On 25/01/2021 10:29, David Marchand wrote:
> On Mon, Jan 25, 2021 at 11:01 AM Kinsella, Ray <m...@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
