[PATCH] buildtools: fix invalid symbols

[PATCH] buildtools: fix invalid symbols

[Mingjin Ye]

ELF files generated by higher version compilers wrap multiple
symbols prefixed with "this_pmd_name".

This patch fixes the issue by filtering invalid symbols.

Bugzilla ID: 1466
Fixes: 6c4bf8f42432 ("buildtools: add Python pmdinfogen")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
 buildtools/pmdinfogen.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/buildtools/pmdinfogen.py b/buildtools/pmdinfogen.py
index 2a44f17bda..6ea97caec7 100755
--- a/buildtools/pmdinfogen.py
+++ b/buildtools/pmdinfogen.py
@@ -200,7 +200,8 @@ def dump(self, file):
 def load_drivers(image):
     drivers = []
     for symbol in image.find_by_prefix("this_pmd_name"):
-        drivers.append(Driver.load(image, symbol))
+        if len(symbol.string_value) != 0:
+            drivers.append(Driver.load(image, symbol))
     return drivers
 
 
-- 
2.25.1

Re: [PATCH] buildtools: fix invalid symbols

[Bruce Richardson]

On Thu, Jun 27, 2024 at 10:11:44AM +0000, Mingjin Ye wrote:
> ELF files generated by higher version compilers wrap multiple
> symbols prefixed with "this_pmd_name".
> 
> This patch fixes the issue by filtering invalid symbols.
> 
> Bugzilla ID: 1466
> Fixes: 6c4bf8f42432 ("buildtools: add Python pmdinfogen")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---
>  buildtools/pmdinfogen.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/buildtools/pmdinfogen.py b/buildtools/pmdinfogen.py
> index 2a44f17bda..6ea97caec7 100755
> --- a/buildtools/pmdinfogen.py
> +++ b/buildtools/pmdinfogen.py
> @@ -200,7 +200,8 @@ def dump(self, file):
>  def load_drivers(image):
>      drivers = []
>      for symbol in image.find_by_prefix("this_pmd_name"):
> -        drivers.append(Driver.load(image, symbol))
> +        if len(symbol.string_value) != 0:
> +            drivers.append(Driver.load(image, symbol))

One small suggestion. Empty strings evaluate to boolean false, so the
condition can just be simplified to:

	if symbol.string_value:
	    drivers.append(....)

/Bruce

Re: [PATCH] buildtools: fix invalid symbols

[David Marchand]

On Thu, Jun 27, 2024 at 12:57 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Thu, Jun 27, 2024 at 10:11:44AM +0000, Mingjin Ye wrote:
> > ELF files generated by higher version compilers wrap multiple
> > symbols prefixed with "this_pmd_name".
> >
> > This patch fixes the issue by filtering invalid symbols.
> >
> > Bugzilla ID: 1466
> > Fixes: 6c4bf8f42432 ("buildtools: add Python pmdinfogen")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> > ---
> >  buildtools/pmdinfogen.py | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/buildtools/pmdinfogen.py b/buildtools/pmdinfogen.py
> > index 2a44f17bda..6ea97caec7 100755
> > --- a/buildtools/pmdinfogen.py
> > +++ b/buildtools/pmdinfogen.py
> > @@ -200,7 +200,8 @@ def dump(self, file):
> >  def load_drivers(image):
> >      drivers = []
> >      for symbol in image.find_by_prefix("this_pmd_name"):
> > -        drivers.append(Driver.load(image, symbol))
> > +        if len(symbol.string_value) != 0:
> > +            drivers.append(Driver.load(image, symbol))
>
> One small suggestion. Empty strings evaluate to boolean false, so the
> condition can just be simplified to:
>
>         if symbol.string_value:
>             drivers.append(....)

I have the same comment than what Ali tried with:
https://patchwork.dpdk.org/project/dpdk/patch/20240320155814.617220-1-alialnu@nvidia.com/

I would prefer we don't rely on the content of symbols (that we don't
know anything about) when we can filter on the symbol names exactly
which is something DPDK controls.

My suggestion is to filter symbol *names* with regex ^this_pmd_name[0-9]+$.


-- 
David Marchand

Re: [PATCH] buildtools: fix invalid symbols

[David Marchand]

On Thu, Jun 27, 2024 at 12:36 PM Mingjin Ye <mingjinx.ye@intel.com> wrote:
>
> ELF files generated by higher version compilers wrap multiple
> symbols prefixed with "this_pmd_name".
>
> This patch fixes the issue by filtering invalid symbols.
>
> Bugzilla ID: 1466
> Fixes: 6c4bf8f42432 ("buildtools: add Python pmdinfogen")
> Cc: stable@dpdk.org
>
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

Is it the same issue as:
https://patchwork.dpdk.org/project/dpdk/patch/20240320155814.617220-1-alialnu@nvidia.com/
?


-- 
David Marchand

[PATCH v2] buildtools: fix invalid symbols

[Mingjin Ye]

Elf files generated by higher version compilers wrap multiple
symbols prefixed with "this_pmd_name".

The patch uses the regex "^this_pmd_name[0-9]+$" to match the
symbol name.

Bugzilla ID: 1466
Fixes: 6c4bf8f42432 ("buildtools: add Python pmdinfogen")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
v2: Use regex ^this_pmd_name[0-9]+$ to filter symbols *names*
---
 buildtools/pmdinfogen.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/buildtools/pmdinfogen.py b/buildtools/pmdinfogen.py
index 2a44f17bda..0fbcc697ed 100755
--- a/buildtools/pmdinfogen.py
+++ b/buildtools/pmdinfogen.py
@@ -6,6 +6,7 @@
 import argparse
 import ctypes
 import json
+import re
 import sys
 import tempfile
 
@@ -70,7 +71,7 @@ def find_by_prefix(self, prefix):
         prefix = prefix.encode("utf-8") if self._legacy_elftools else prefix
         for i in range(self._symtab.num_symbols()):
             symbol = self._symtab.get_symbol(i)
-            if symbol.name.startswith(prefix):
+            if re.match(prefix, symbol.name):
                 yield ELFSymbol(self._image, symbol)
 
 
@@ -199,7 +200,7 @@ def dump(self, file):
 
 def load_drivers(image):
     drivers = []
-    for symbol in image.find_by_prefix("this_pmd_name"):
+    for symbol in image.find_by_prefix("^this_pmd_name[0-9]+$"):
         drivers.append(Driver.load(image, symbol))
     return drivers
 
-- 
2.25.1

Re: [PATCH v2] buildtools: fix invalid symbols

[David Marchand]

Adding Ali in the loop, as he was working on a similar patch.

On Mon, Jul 1, 2024 at 12:56 PM Mingjin Ye <mingjinx.ye@intel.com> wrote:
>
> Elf files generated by higher version compilers wrap multiple
> symbols prefixed with "this_pmd_name".
>
> The patch uses the regex "^this_pmd_name[0-9]+$" to match the
> symbol name.
>
> Bugzilla ID: 1466
> Fixes: 6c4bf8f42432 ("buildtools: add Python pmdinfogen")
> Cc: stable@dpdk.org
>
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---
> v2: Use regex ^this_pmd_name[0-9]+$ to filter symbols *names*
> ---
>  buildtools/pmdinfogen.py | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/buildtools/pmdinfogen.py b/buildtools/pmdinfogen.py
> index 2a44f17bda..0fbcc697ed 100755
> --- a/buildtools/pmdinfogen.py
> +++ b/buildtools/pmdinfogen.py
> @@ -6,6 +6,7 @@
>  import argparse
>  import ctypes
>  import json
> +import re
>  import sys
>  import tempfile
>
> @@ -70,7 +71,7 @@ def find_by_prefix(self, prefix):

This function does not find symbols with a prefix anymore...
Please rename.


>          prefix = prefix.encode("utf-8") if self._legacy_elftools else prefix
>          for i in range(self._symtab.num_symbols()):
>              symbol = self._symtab.get_symbol(i)
> -            if symbol.name.startswith(prefix):
> +            if re.match(prefix, symbol.name):
>                  yield ELFSymbol(self._image, symbol)
>
>
> @@ -199,7 +200,7 @@ def dump(self, file):
>
>  def load_drivers(image):
>      drivers = []
> -    for symbol in image.find_by_prefix("this_pmd_name"):
> +    for symbol in image.find_by_prefix("^this_pmd_name[0-9]+$"):
>          drivers.append(Driver.load(image, symbol))
>      return drivers

The COFF implementation is probably broken by this change as it is
still filtering symbols with a prefix.
Please update.


-- 
David Marchand

[PATCH v3] buildtools: fix invalid symbols

[Mingjin Ye]

In scenarios where a higher clang compiler is used and ASAN is enabled,
the generated ELF file will additionally insert undefined debug symbols
with the same prefix. This causes duplicate C code to be generated.

This patch fixes this issue by skipping the unspecified symbol type.

Fixes: 6c4bf8f42432 ("buildtools: add Python pmdinfogen")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
 buildtools/pmdinfogen.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/buildtools/pmdinfogen.py b/buildtools/pmdinfogen.py
index 2a44f17bda..9896f107dc 100755
--- a/buildtools/pmdinfogen.py
+++ b/buildtools/pmdinfogen.py
@@ -70,6 +70,9 @@ def find_by_prefix(self, prefix):
         prefix = prefix.encode("utf-8") if self._legacy_elftools else prefix
         for i in range(self._symtab.num_symbols()):
             symbol = self._symtab.get_symbol(i)
+            # Skip unspecified symbol type
+            if symbol.entry.st_info['type'] == "STT_NOTYPE":
+                continue
             if symbol.name.startswith(prefix):
                 yield ELFSymbol(self._image, symbol)
 
-- 
2.25.1

RE: [PATCH v3] buildtools: fix invalid symbols

[Ali Alnubani]

> -----Original Message-----
> From: Mingjin Ye <mingjinx.ye@intel.com>
> Sent: Friday, July 5, 2024 11:26 AM
> To: dev@dpdk.org
> Cc: david.marchand@redhat.com; Ali Alnubani <alialnu@nvidia.com>; Mingjin Ye
> <mingjinx.ye@intel.com>; stable@dpdk.org; Dmitry Kozlyuk
> <dmitry.kozliuk@gmail.com>
> Subject: [PATCH v3] buildtools: fix invalid symbols
> 
> In scenarios where a higher clang compiler is used and ASAN is enabled,
> the generated ELF file will additionally insert undefined debug symbols
> with the same prefix. This causes duplicate C code to be generated.
> 
> This patch fixes this issue by skipping the unspecified symbol type.
> 
> Fixes: 6c4bf8f42432 ("buildtools: add Python pmdinfogen")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---

Can confirm it fixes the build failure in my environment, thanks Mingjin.

Tested-by: Ali Alnubani <alialnu@nvidia.com>

Regards,
Ali

Re: [PATCH v3] buildtools: fix invalid symbols

[David Marchand]

On Fri, Jul 5, 2024 at 10:49 AM Mingjin Ye <mingjinx.ye@intel.com> wrote:
>
> In scenarios where a higher clang compiler is used and ASAN is enabled,
> the generated ELF file will additionally insert undefined debug symbols
> with the same prefix. This causes duplicate C code to be generated.
>
> This patch fixes this issue by skipping the unspecified symbol type.

You did not reply to my comments on v3 and here we have a new hack.
This hack is ugly and not future proof (I can imagine some other
symbols may appear in the future. If those symbols are not of type
"STT_NOTYPE" we will have to find another filter at this point...).

Please have a try with:
https://patchwork.dpdk.org/project/dpdk/patch/20240711113851.975368-1-david.marchand@redhat.com/


Thanks.

-- 
David Marchand