Re: [dpdk-dev] [PATCH v8 2/3] build: use Python pmdinfogen

On Mon, 25 Jan 2021 09:25:51 +0000, Kinsella, Ray wrote:

On 23/01/2021 11:38, Thomas Monjalon wrote:

quoted

22/01/2021 23:24, Dmitry Kozlyuk:  

quoted

On Fri, 22 Jan 2021 21:57:15 +0100, Thomas Monjalon wrote:  

quoted

22/01/2021 21:31, Dmitry Kozlyuk:  

quoted

On Wed, 20 Jan 2021 11:24:21 +0100, Thomas Monjalon wrote:    

quoted

20/01/2021 08:23, Dmitry Kozlyuk:    

quoted

On Wed, 20 Jan 2021 01:05:59 +0100, Thomas Monjalon wrote:      

quoted

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.
 

quoted

"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.