[v8,2/3] build: use Python pmdinfogen
Checks
Commit Message
Use the same interpreter to run pmdinfogen as for other build scripts.
Adjust wrapper script accordingly and also don't suppress stderr from ar
and pmdinfogen.
Add configure-time check for elftools Python module for Unix hosts.
Add python3-pyelftools to CI configuration. The package is available on
all major distributions. FreeBSD has no system requirements section in
its GSG. Currently neither Windows uses pmdinfogen, nor is COFF (PE)
supported.
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
.travis.yml | 2 +-
buildtools/gen-pmdinfo-cfile.sh | 6 +++---
buildtools/meson.build | 15 +++++++++++++++
doc/guides/linux_gsg/sys_reqs.rst | 6 ++++++
drivers/meson.build | 2 +-
meson.build | 1 -
6 files changed, 26 insertions(+), 6 deletions(-)
Comments
On Tue, Oct 20, 2020 at 08:44:02PM +0300, Dmitry Kozlyuk wrote:
> Use the same interpreter to run pmdinfogen as for other build scripts.
> Adjust wrapper script accordingly and also don't suppress stderr from ar
> and pmdinfogen.
>
> Add configure-time check for elftools Python module for Unix hosts.
>
> Add python3-pyelftools to CI configuration. The package is available on
> all major distributions. FreeBSD has no system requirements section in
> its GSG. Currently neither Windows uses pmdinfogen, nor is COFF (PE)
> supported.
>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
From a meson viewpoint, this all looks ok. I'd hope in future that we could
merge the shell script wrapper into the python script directly, rather than
having two levels of scripting, but it's not something with any urgency.
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> .travis.yml | 2 +-
> buildtools/gen-pmdinfo-cfile.sh | 6 +++---
> buildtools/meson.build | 15 +++++++++++++++
> doc/guides/linux_gsg/sys_reqs.rst | 6 ++++++
> drivers/meson.build | 2 +-
> meson.build | 1 -
> 6 files changed, 26 insertions(+), 6 deletions(-)
>
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
Few more comments below:
20/10/2020 19:44, Dmitry Kozlyuk:
> --- a/buildtools/meson.build
> +++ b/buildtools/meson.build
> +if host_machine.system() != 'windows'
You can use "is_windows".
> --- a/doc/guides/linux_gsg/sys_reqs.rst
> +++ b/doc/guides/linux_gsg/sys_reqs.rst
> +* ``pyelftools`` (version 0.22+)
This requirement is missing in doc/guides/freebsd_gsg/build_dpdk.rst
> --- a/meson.build
> +++ b/meson.build
> -subdir('buildtools/pmdinfogen')
This could be in patch 3 (removing the code).
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.
> Few more comments below:
>
> 20/10/2020 19:44, Dmitry Kozlyuk:
> > --- a/buildtools/meson.build
> > +++ b/buildtools/meson.build
> > +if host_machine.system() != 'windows'
>
> You can use "is_windows".
It's defined by config/meson.build, which is processed after
buidtools/meson.build, because of the dependency, if swapped:
config/x86/meson.build:6:1: ERROR: Unknown variable
"binutils_avx512_check".
> > --- a/doc/guides/linux_gsg/sys_reqs.rst
> > +++ b/doc/guides/linux_gsg/sys_reqs.rst
> > +* ``pyelftools`` (version 0.22+)
>
> This requirement is missing in doc/guides/freebsd_gsg/build_dpdk.rst
OK.
> > --- a/meson.build
> > +++ b/meson.build
> > -subdir('buildtools/pmdinfogen')
>
> This could be in patch 3 (removing the code).
It would redefine "pmdinfogen" variable to old pmdinfogen.
Besides, why build what's not used at this patch already?
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.
>
> > Few more comments below:
> >
> > 20/10/2020 19:44, Dmitry Kozlyuk:
> > > --- a/buildtools/meson.build
> > > +++ b/buildtools/meson.build
> > > +if host_machine.system() != 'windows'
> >
> > You can use "is_windows".
>
> It's defined by config/meson.build, which is processed after
> buidtools/meson.build, because of the dependency, if swapped:
>
> config/x86/meson.build:6:1: ERROR: Unknown variable
> "binutils_avx512_check".
OK
> > > --- a/doc/guides/linux_gsg/sys_reqs.rst
> > > +++ b/doc/guides/linux_gsg/sys_reqs.rst
> > > +* ``pyelftools`` (version 0.22+)
> >
> > This requirement is missing in doc/guides/freebsd_gsg/build_dpdk.rst
>
> OK.
>
> > > --- a/meson.build
> > > +++ b/meson.build
> > > -subdir('buildtools/pmdinfogen')
> >
> > This could be in patch 3 (removing the code).
>
> It would redefine "pmdinfogen" variable to old pmdinfogen.
> Besides, why build what's not used at this patch already?
Just trying to find the best patch split.
If needed, OK to keep as is.
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.
"xxx_pmd_info" changes are due to JSON formatting (new is more canonical),
which can be worked around easily, if the above is wrong.
> > > > --- a/meson.build
> > > > +++ b/meson.build
> > > > -subdir('buildtools/pmdinfogen')
> > >
> > > This could be in patch 3 (removing the code).
> >
> > It would redefine "pmdinfogen" variable to old pmdinfogen.
> > Besides, why build what's not used at this patch already?
>
> Just trying to find the best patch split.
> If needed, OK to keep as is.
I even don't mind squashing all three commits into one.
The split is done to ease the review.
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.
> > > > > --- a/meson.build
> > > > > +++ b/meson.build
> > > > > -subdir('buildtools/pmdinfogen')
> > > >
> > > > This could be in patch 3 (removing the code).
> > >
> > > It would redefine "pmdinfogen" variable to old pmdinfogen.
> > > Besides, why build what's not used at this patch already?
> >
> > Just trying to find the best patch split.
> > If needed, OK to keep as is.
>
> I even don't mind squashing all three commits into one.
> The split is done to ease the review.
I think the split is good as is.
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
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.
On Sat, 23 Jan 2021 12:38:45 +0100, 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.
Send v10 with suppression.
Such check, however, *can* be implemented: at ABI check stage we have two
install directories that dpdk-pmdinfo.py can inspect. Then a script can check
that diff contains only additions, i.e. no device support being removed.
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?
Ray K
On 25/01/2021 09:25, 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?
>
> Ray K
Another point is - shouldn't _pmd_info probably live in "INTERNAL" is anycase?
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. As for
content, checking that no PCI IDs are removed is out of scope for libabigail
anyway. Technically we could fix _pmd_info size, but this still allows
breaking changes to pass the check with no benefit.
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 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?
On Mon, Jan 25, 2021 at 11:01 AM Kinsella, Ray <mdr@ashroe.eu> wrote:
>
>
>
> On 25/01/2021 09:25, 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?
> >
> > Ray K
>
>
> Another point is - shouldn't _pmd_info probably live in "INTERNAL" is anycase?
The symbol itself can be hidden from the ABeyes.
It is only a placeholder for the PMD_INFO_STRING= string used by
usertools/dpdk-pmdinfo.py and maybe some other parsing tool.
I guess a static symbol would be enough:
diff --git a/buildtools/pmdinfogen/pmdinfogen.c
b/buildtools/pmdinfogen/pmdinfogen.c
index a68d1ea999..14bf7d9f42 100644
--- a/buildtools/pmdinfogen/pmdinfogen.c
+++ b/buildtools/pmdinfogen/pmdinfogen.c
@@ -393,7 +393,7 @@ static void output_pmd_info_string(struct elf_info
*info, char *outfile)
drv = info->drivers;
while (drv) {
- fprintf(ofd, "const char %s_pmd_info[] __attribute__((used)) = "
+ fprintf(ofd, "static const char %s_pmd_info[]
__attribute__((used)) = "
"\"PMD_INFO_STRING= {",
drv->name);
fprintf(ofd, "\\\"name\\\" : \\\"%s\\\", ", drv->name);
We will need an exception for the v21 ABI though.
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.
On 25/01/2021 10:29, David Marchand wrote:
> On Mon, Jan 25, 2021 at 11:01 AM Kinsella, Ray <mdr@ashroe.eu> wrote:
>>
>>
>>
>> On 25/01/2021 09:25, 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?
>>>
>>> Ray K
>>
>>
>> Another point is - shouldn't _pmd_info probably live in "INTERNAL" is anycase?
>
> The symbol itself can be hidden from the ABeyes.
> It is only a placeholder for the PMD_INFO_STRING= string used by
> usertools/dpdk-pmdinfo.py and maybe some other parsing tool.
>
> I guess a static symbol would be enough:
>
> diff --git a/buildtools/pmdinfogen/pmdinfogen.c
> b/buildtools/pmdinfogen/pmdinfogen.c
> index a68d1ea999..14bf7d9f42 100644
> --- a/buildtools/pmdinfogen/pmdinfogen.c
> +++ b/buildtools/pmdinfogen/pmdinfogen.c
> @@ -393,7 +393,7 @@ static void output_pmd_info_string(struct elf_info
> *info, char *outfile)
> drv = info->drivers;
>
> while (drv) {
> - fprintf(ofd, "const char %s_pmd_info[] __attribute__((used)) = "
> + fprintf(ofd, "static const char %s_pmd_info[]
> __attribute__((used)) = "
> "\"PMD_INFO_STRING= {",
> drv->name);
> fprintf(ofd, "\\\"name\\\" : \\\"%s\\\", ", drv->name);
>
>
> We will need an exception for the v21 ABI though.
>
Good suggestion +1
25/01/2021 11:46, Kinsella, Ray:
> On 25/01/2021 10:29, David Marchand wrote:
> > The symbol itself can be hidden from the ABeyes.
> > It is only a placeholder for the PMD_INFO_STRING= string used by
> > usertools/dpdk-pmdinfo.py and maybe some other parsing tool.
> >
> > I guess a static symbol would be enough:
> >
> > diff --git a/buildtools/pmdinfogen/pmdinfogen.c
> > b/buildtools/pmdinfogen/pmdinfogen.c
> > index a68d1ea999..14bf7d9f42 100644
> > --- a/buildtools/pmdinfogen/pmdinfogen.c
> > +++ b/buildtools/pmdinfogen/pmdinfogen.c
> > @@ -393,7 +393,7 @@ static void output_pmd_info_string(struct elf_info
> > *info, char *outfile)
> > drv = info->drivers;
> >
> > while (drv) {
> > - fprintf(ofd, "const char %s_pmd_info[] __attribute__((used)) = "
> > + fprintf(ofd, "static const char %s_pmd_info[]
> > __attribute__((used)) = "
> > "\"PMD_INFO_STRING= {",
> > drv->name);
> > fprintf(ofd, "\\\"name\\\" : \\\"%s\\\", ", drv->name);
> >
> >
> > We will need an exception for the v21 ABI though.
> >
>
> Good suggestion +1
Yes +1 for adding static on *_pmd_info
@@ -11,7 +11,7 @@ addons:
apt:
update: true
packages: &required_packages
- - [libnuma-dev, python3-setuptools, python3-wheel, python3-pip, ninja-build]
+ - [libnuma-dev, python3-setuptools, python3-wheel, python3-pip, python3-pyelftools, ninja-build]
- [libbsd-dev, libpcap-dev, libibverbs-dev, libcrypto++-dev, libfdt-dev, libjansson-dev]
_aarch64_packages: &aarch64_packages
@@ -4,11 +4,11 @@
arfile=$1
output=$2
-pmdinfogen=$3
+shift 2
+pmdinfogen=$*
# The generated file must not be empty if compiled in pedantic mode
echo 'static __attribute__((unused)) const char *generator = "'$0'";' > $output
for ofile in `ar t $arfile` ; do
- ar p $arfile $ofile | $pmdinfogen - - >> $output 2> /dev/null
+ ar p $arfile $ofile | $pmdinfogen - - >> $output
done
-exit 0
@@ -17,3 +17,18 @@ else
endif
map_to_win_cmd = py3 + files('map_to_win.py')
sphinx_wrapper = py3 + files('call-sphinx-build.py')
+pmdinfogen = py3 + files('pmdinfogen.py')
+
+# TODO: starting from Meson 0.51.0 use
+# python3 = import('python').find_installation('python',
+# modules : python3_required_modules)
+python3_required_modules = []
+if host_machine.system() != 'windows'
+ python3_required_modules = ['elftools']
+endif
+foreach module : python3_required_modules
+ script = 'import importlib.util; import sys; exit(importlib.util.find_spec("@0@") is None)'
+ if run_command(py3, '-c', script.format(module)).returncode() != 0
+ error('missing python module: @0@'.format(module))
+ endif
+endforeach
@@ -52,6 +52,12 @@ Compilation of the DPDK
* If the packaged version is below the minimum version, the latest versions
can be installed from Python's "pip" repository: ``pip3 install meson ninja``
+* ``pyelftools`` (version 0.22+)
+
+ * For RHEL/Fedora systems it can be installed using ``dnf install python-pyelftools``
+
+ * For Ubuntu/Debian it can be installed using ``apt install python3-pyelftools``
+
* Library for handling NUMA (Non Uniform Memory Access).
* ``numactl-devel`` in RHEL/Fedora;
@@ -152,7 +152,7 @@ foreach subpath:subdirs
command: [pmdinfo, tmp_lib.full_path(),
'@OUTPUT@', pmdinfogen],
output: out_filename,
- depends: [pmdinfogen, tmp_lib])
+ depends: [tmp_lib])
endif
# now build the static driver
@@ -45,7 +45,6 @@ subdir('buildtools')
subdir('config')
# build libs and drivers
-subdir('buildtools/pmdinfogen')
subdir('lib')
subdir('drivers')