On Mon, 25 Jan 2021 10:11:07 +0000, Kinsella, Ray wrote:
> On 25/01/2021 10:05, Dmitry Kozlyuk wrote:
> > On Mon, 25 Jan 2021 09:25:51 +0000, Kinsella, Ray wrote:
> >> On 23/01/2021 11:38, Thomas Monjalon wrote:
> >>> 22/01/2021 23:24, Dmitry Kozlyuk:
> >>>> On Fri, 22 Jan 2021 21:57:15 +0100, Thomas Monjalon wrote:
> >>>>> 22/01/2021 21:31, Dmitry Kozlyuk:
> >>>>>> On Wed, 20 Jan 2021 11:24:21 +0100, Thomas Monjalon wrote:
> >>>>>>> 20/01/2021 08:23, Dmitry Kozlyuk:
> >>>>>>>> On Wed, 20 Jan 2021 01:05:59 +0100, Thomas Monjalon wrote:
> >>>>>>>>> This is now the right timeframe to introduce this change
> >>>>>>>>> with the new Python module dependency.
> >>>>>>>>> Unfortunately, the ABI check is returning an issue:
> >>>>>>>>>
> >>>>>>>>> 'const char mlx5_common_pci_pmd_info[62]' was changed
> >>>>>>>>> to 'const char mlx5_common_pci_pmd_info[60]' at
> >>>>>>>>> rte_common_mlx5.pmd.c
> >>>>>>>>
> >>>>>>>> Will investigate and fix ASAP.
> >>>>>>
> >>>>>> Now that I think of it: strings like this change every time new PCI
> >>>>>> IDs are
> >>>>>> added to a PMD, but AFAIK adding PCI IDs is not considered an ABI
> >>>>>> breakage,
> >>>>>> is it? One example is 28c9a7d7b48e ("net/mlx5: add ConnectX-6 Lx
> >>>>>> device ID")
> >>>>>> added 2020-07-08, i.e. clearly outside of ABI change window.
> >>>>>
> >>>>> You're right.
> >>>>>
> >>>>>> "xxx_pmd_info" changes are due to JSON formatting (new is more
> >>>>>> canonical),
> >>>>>> which can be worked around easily, if the above is wrong.
> >>>>>
> >>>>> If the new format is better, please keep it.
> >>>>> What we need is an exception for the pmdinfo symbols
> >>>>> in the file devtools/libabigail.abignore.
> >>>>> You can probably use a regex for these symbols.
> >>>>
> >>>> This would allow real breakages to pass ABI check, abidiff doesn't
> >>>> analyze
> >>>> variable content and it's not easy to compare. Maybe later a script can
> >>>> be
> >>>> added that checks lines with RTE_DEVICE_IN in patches. There are at most
> >>>> 32 of
> >>>> 5494 relevant commits between 19.11 and 20.11, though.
> >>>>
> >>>> To verify there are no meaningful changes I ensured empty diff between
> >>>> results of the following command for "main" and the branch:
> >>>>
> >>>> find build/drivers -name '*.so' -exec usertools/dpdk-pmdinfo.py
> >>>
> >>> For now we cannot do such check as part of the ABI checker.
> >>> And we cannot merge this patch if the ABI check fails.
> >>> I think the only solution is to allow any change in the pmdinfo variables.
> >>>
> >>
> >> So my 2c on this is that this is an acceptable work-around for the v21
> >> (DPDK v20.11) ABI.
> >> However we are going to end up carrying this rule in libabigail.ignore
> >> indefinitely.
> >>
> >> Would it make sense to just fix the size of _pmd_info to some reasonably
> >> large value -
> >> say 128 bytes, to allow us to drop the rule in the DPDK 21.11 v22 release?
> >>
> >
> > I don't think so. This is a JSON *string to be parsed;* considering its size
> > as part of application *binary* interface is wrong in the first place.
>
> Right - then is belongs in INTERNAL, I would say.
>
> > As for
> > content, checking that no PCI IDs are removed is out of scope for libabigail
> > anyway.
>
> Lets be clear PCI IDs - are _nothing_ to do with ABI.
Technically, yes, but they're referred to in abi_policy.rst, because DPDK
behavior depends on them. Same issue as with as return values: no formats
change, yet compatibility is broken.
> > Technically we could fix _pmd_info size, but this still allows
> > breaking changes to pass the check with no benefit.
>
> ABI changes or other, please explain?
Behavioral changes via PCI ID removal, see above.
