[dpdk-dev] [PATCH] devtools: catch symbol duplicates in version map

[dpdk-dev] [PATCH] devtools: catch symbol duplicates in version map

[David Marchand]

Add a check on versioned symbol duplicates in map files.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
This check could be added to buildtools/check-symbols.sh so that
regular developers catch the issue when building their changes...
Opinions?

---
 devtools/check-symbol-maps.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/devtools/check-symbol-maps.sh b/devtools/check-symbol-maps.sh
index c3cbcaf720..a430de7f64 100755
--- a/devtools/check-symbol-maps.sh
+++ b/devtools/check-symbol-maps.sh
@@ -35,6 +35,21 @@ if [ -n "$orphan_symbols" ] ; then
     ret=1
 fi
 
+find_duplicate_symbols ()
+{
+    for map in $(find lib drivers -name '*.map') ; do
+        ! buildtools/map-list-symbol.sh $map | \
+		sort | uniq -c | grep -v " 1 $map"
+    done
+}
+
+duplicate_symbols=$(find_duplicate_symbols)
+if [ -n "$duplicate_symbols" ] ; then
+    echo "Found duplicates in symbol map file:"
+    echo "$duplicate_symbols"
+    ret=1
+fi
+
 find_orphan_windows_symbols ()
 {
     for def in $(find lib drivers -name '*_exports.def') ; do
-- 
2.23.0

Re: [dpdk-dev] [PATCH] devtools: catch symbol duplicates in version map

[Thomas Monjalon]

25/02/2021 12:14, David Marchand:
> Add a check on versioned symbol duplicates in map files.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> This check could be added to buildtools/check-symbols.sh so that
> regular developers catch the issue when building their changes...
> Opinions?

In general I am against adding developer tools in the build process,
because it could cause more issues for normal users.

Re: [dpdk-dev] [PATCH] devtools: catch symbol duplicates in version map

[Bruce Richardson]

On Thu, Feb 25, 2021 at 12:41:16PM +0100, Thomas Monjalon wrote:
> 25/02/2021 12:14, David Marchand:
> > Add a check on versioned symbol duplicates in map files.
> > 
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> > This check could be added to buildtools/check-symbols.sh so that
> > regular developers catch the issue when building their changes...
> > Opinions?
> 
> In general I am against adding developer tools in the build process,
> because it could cause more issues for normal users.
> 
Is this really likely to cause issues for normal users? If we add this to
the build process any issues will surely be caught be developers before
merge.

Re: [dpdk-dev] [PATCH] devtools: catch symbol duplicates in version map

[Thomas Monjalon]

25/02/2021 12:57, Bruce Richardson:
> On Thu, Feb 25, 2021 at 12:41:16PM +0100, Thomas Monjalon wrote:
> > 25/02/2021 12:14, David Marchand:
> > > Add a check on versioned symbol duplicates in map files.
> > > 
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > ---
> > > This check could be added to buildtools/check-symbols.sh so that
> > > regular developers catch the issue when building their changes...
> > > Opinions?
> > 
> > In general I am against adding developer tools in the build process,
> > because it could cause more issues for normal users.
> > 
> Is this really likely to cause issues for normal users?

Yes because some users will have a different shell,
or other weird setup we don't think about yet.

> If we add this to
> the build process any issues will surely be caught be developers before
> merge.

If we want the checks to be more popular, we should write a script
to help running the right script at the right time,
and ideally help in the contribution process.

Re: [dpdk-dev] [PATCH] devtools: catch symbol duplicates in version map

[Bruce Richardson]

On Thu, Feb 25, 2021 at 01:01:10PM +0100, Thomas Monjalon wrote:
> 25/02/2021 12:57, Bruce Richardson:
> > On Thu, Feb 25, 2021 at 12:41:16PM +0100, Thomas Monjalon wrote:
> > > 25/02/2021 12:14, David Marchand:
> > > > Add a check on versioned symbol duplicates in map files.
> > > > 
> > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > ---
> > > > This check could be added to buildtools/check-symbols.sh so that
> > > > regular developers catch the issue when building their changes...
> > > > Opinions?
> > > 
> > > In general I am against adding developer tools in the build process,
> > > because it could cause more issues for normal users.
> > > 
> > Is this really likely to cause issues for normal users?
> 
> Yes because some users will have a different shell,
> or other weird setup we don't think about yet.
> 
I think it unlikely, but ok.

Now that meson (from version 0.53 onwards) has a "filesystem" module, with
an "exists" function, we can perhaps look to introduce a "developer mode"
build again, based off the presence of the .git folder. Alternatively, we
could make "developer mode" a regular meson option rather than trying to be
too smart about it. [Or combine both and have developer mode option with
"enabled"/"disabled"/"auto-detect" values]

/Bruce

Re: [dpdk-dev] [PATCH] devtools: catch symbol duplicates in version map

[Thomas Monjalon]

25/02/2021 13:05, Bruce Richardson:
> On Thu, Feb 25, 2021 at 01:01:10PM +0100, Thomas Monjalon wrote:
> > 25/02/2021 12:57, Bruce Richardson:
> > > On Thu, Feb 25, 2021 at 12:41:16PM +0100, Thomas Monjalon wrote:
> > > > 25/02/2021 12:14, David Marchand:
> > > > > Add a check on versioned symbol duplicates in map files.
> > > > > 
> > > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > > ---
> > > > > This check could be added to buildtools/check-symbols.sh so that
> > > > > regular developers catch the issue when building their changes...
> > > > > Opinions?
> > > > 
> > > > In general I am against adding developer tools in the build process,
> > > > because it could cause more issues for normal users.
> > > > 
> > > Is this really likely to cause issues for normal users?
> > 
> > Yes because some users will have a different shell,
> > or other weird setup we don't think about yet.
> > 
> I think it unlikely, but ok.

You would be surprised.
I'm going to send a fix for running buildtools/check-symbols.sh
on busybox.

> Now that meson (from version 0.53 onwards) has a "filesystem" module, with
> an "exists" function, we can perhaps look to introduce a "developer mode"
> build again, based off the presence of the .git folder. Alternatively, we
> could make "developer mode" a regular meson option rather than trying to be
> too smart about it. [Or combine both and have developer mode option with
> "enabled"/"disabled"/"auto-detect" values]

Yes, a developer mode is a good idea.

[dpdk-dev] [PATCH v2 1/2] devtools: catch symbol duplicates in version map

[David Marchand]

Add a check on versioned symbol duplicates in map files.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- rebased,

---
 devtools/check-symbol-maps.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/devtools/check-symbol-maps.sh b/devtools/check-symbol-maps.sh
index f06353fc75..f2c7d29663 100755
--- a/devtools/check-symbol-maps.sh
+++ b/devtools/check-symbol-maps.sh
@@ -35,4 +35,19 @@ if [ -n "$orphan_symbols" ] ; then
     ret=1
 fi
 
+find_duplicate_symbols ()
+{
+    for map in $(find lib drivers -name '*.map') ; do
+        buildtools/map-list-symbol.sh $map | \
+            sort | uniq -c | grep -v " 1 $map" || true
+    done
+}
+
+duplicate_symbols=$(find_duplicate_symbols)
+if [ -n "$duplicate_symbols" ] ; then
+    echo "Found duplicates in symbol map file:"
+    echo "$duplicate_symbols"
+    ret=1
+fi
+
 exit $ret
-- 
2.23.0

[dpdk-dev] [PATCH v2 2/2] buildtools: check symbol maps in developer mode

[David Marchand]

Hook check-symbol-maps.sh in the symbol check when in developer mode to
help developers catch issues before submitting their changes.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- now that we have the developer mode, added this check in the build
  process,

---
 buildtools/check-symbols.sh   |  9 ++++++++-
 devtools/check-symbol-maps.sh | 12 ++++++++----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/buildtools/check-symbols.sh b/buildtools/check-symbols.sh
index 83b3a0182f..e458c0af72 100755
--- a/buildtools/check-symbols.sh
+++ b/buildtools/check-symbols.sh
@@ -5,7 +5,9 @@
 MAPFILE=$1
 OBJFILE=$2
 
-LIST_SYMBOL=$(dirname $(readlink -f $0))/map-list-symbol.sh
+ROOTDIR=$(readlink -f $(dirname $(readlink -f $0))/..)
+LIST_SYMBOL=$ROOTDIR/buildtools/map-list-symbol.sh
+CHECK_SYMBOL_MAPS=$ROOTDIR/devtools/check-symbol-maps.sh
 
 # added check for "make -C test/" usage
 if [ ! -e $MAPFILE ] || [ ! -f $OBJFILE ]
@@ -23,6 +25,11 @@ trap 'rm -f "$DUMPFILE"' EXIT
 objdump -t $OBJFILE >$DUMPFILE
 
 ret=0
+
+if ! $CHECK_SYMBOL_MAPS $MAPFILE; then
+	ret=1
+fi
+
 for SYM in `$LIST_SYMBOL -S EXPERIMENTAL $MAPFILE |cut -d ' ' -f 3`
 do
 	if grep -q "\.text.*[[:space:]]$SYM$" $DUMPFILE &&
diff --git a/devtools/check-symbol-maps.sh b/devtools/check-symbol-maps.sh
index f2c7d29663..c52e28bd69 100755
--- a/devtools/check-symbol-maps.sh
+++ b/devtools/check-symbol-maps.sh
@@ -7,11 +7,15 @@ cd $(dirname $0)/..
 # speed up by ignoring Unicode details
 export LC_ALL=C
 
+if [ $# = 0 ] ; then
+    set -- $(find lib drivers -name '*.map')
+fi
+
 ret=0
 
 find_orphan_symbols ()
 {
-    for map in $(find lib drivers -name '*.map') ; do
+    for map in $@ ; do
         for sym in $(sed -rn 's,^([^}]*_.*);.*$,\1,p' $map) ; do
             if echo $sym | grep -q '^per_lcore_' ; then
                 symsrc=${sym#per_lcore_}
@@ -28,7 +32,7 @@ find_orphan_symbols ()
     done
 }
 
-orphan_symbols=$(find_orphan_symbols)
+orphan_symbols=$(find_orphan_symbols $@)
 if [ -n "$orphan_symbols" ] ; then
     echo "Found only in symbol map file:"
     echo "$orphan_symbols" | sed 's,^,\t,'
@@ -37,13 +41,13 @@ fi
 
 find_duplicate_symbols ()
 {
-    for map in $(find lib drivers -name '*.map') ; do
+    for map in $@ ; do
         buildtools/map-list-symbol.sh $map | \
             sort | uniq -c | grep -v " 1 $map" || true
     done
 }
 
-duplicate_symbols=$(find_duplicate_symbols)
+duplicate_symbols=$(find_duplicate_symbols $@)
 if [ -n "$duplicate_symbols" ] ; then
     echo "Found duplicates in symbol map file:"
     echo "$duplicate_symbols"
-- 
2.23.0

Re: [dpdk-dev] [PATCH v2 2/2] buildtools: check symbol maps in developer mode

[Kinsella, Ray]

On 05/05/2021 15:30, David Marchand wrote:
> Hook check-symbol-maps.sh in the symbol check when in developer mode to
> help developers catch issues before submitting their changes.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v1:
> - now that we have the developer mode, added this check in the build
>   process,
> 
> ---
>  buildtools/check-symbols.sh   |  9 ++++++++-
>  devtools/check-symbol-maps.sh | 12 ++++++++----
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/buildtools/check-symbols.sh b/buildtools/check-symbols.sh
> index 83b3a0182f..e458c0af72 100755
> --- a/buildtools/check-symbols.sh
> +++ b/buildtools/check-symbols.sh
> @@ -5,7 +5,9 @@
>  MAPFILE=$1
>  OBJFILE=$2
>  
> -LIST_SYMBOL=$(dirname $(readlink -f $0))/map-list-symbol.sh
> +ROOTDIR=$(readlink -f $(dirname $(readlink -f $0))/..)
> +LIST_SYMBOL=$ROOTDIR/buildtools/map-list-symbol.sh
> +CHECK_SYMBOL_MAPS=$ROOTDIR/devtools/check-symbol-maps.sh
>  
>  # added check for "make -C test/" usage
>  if [ ! -e $MAPFILE ] || [ ! -f $OBJFILE ]
> @@ -23,6 +25,11 @@ trap 'rm -f "$DUMPFILE"' EXIT
>  objdump -t $OBJFILE >$DUMPFILE
>  
>  ret=0
> +
> +if ! $CHECK_SYMBOL_MAPS $MAPFILE; then
> +	ret=1
> +fi
> +
>  for SYM in `$LIST_SYMBOL -S EXPERIMENTAL $MAPFILE |cut -d ' ' -f 3`
>  do
>  	if grep -q "\.text.*[[:space:]]$SYM$" $DUMPFILE &&
> diff --git a/devtools/check-symbol-maps.sh b/devtools/check-symbol-maps.sh
> index f2c7d29663..c52e28bd69 100755
> --- a/devtools/check-symbol-maps.sh
> +++ b/devtools/check-symbol-maps.sh
> @@ -7,11 +7,15 @@ cd $(dirname $0)/..
>  # speed up by ignoring Unicode details
>  export LC_ALL=C
>  
> +if [ $# = 0 ] ; then
> +    set -- $(find lib drivers -name '*.map')
> +fi
> +
>  ret=0
>  
>  find_orphan_symbols ()
>  {
> -    for map in $(find lib drivers -name '*.map') ; do
> +    for map in $@ ; do
>          for sym in $(sed -rn 's,^([^}]*_.*);.*$,\1,p' $map) ; do
>              if echo $sym | grep -q '^per_lcore_' ; then
>                  symsrc=${sym#per_lcore_}
> @@ -28,7 +32,7 @@ find_orphan_symbols ()
>      done
>  }
>  
> -orphan_symbols=$(find_orphan_symbols)
> +orphan_symbols=$(find_orphan_symbols $@)
>  if [ -n "$orphan_symbols" ] ; then
>      echo "Found only in symbol map file:"
>      echo "$orphan_symbols" | sed 's,^,\t,'
> @@ -37,13 +41,13 @@ fi
>  
>  find_duplicate_symbols ()
>  {
> -    for map in $(find lib drivers -name '*.map') ; do
> +    for map in $@ ; do
>          buildtools/map-list-symbol.sh $map | \
>              sort | uniq -c | grep -v " 1 $map" || true
>      done
>  }
>  
> -duplicate_symbols=$(find_duplicate_symbols)
> +duplicate_symbols=$(find_duplicate_symbols $@)
>  if [ -n "$duplicate_symbols" ] ; then
>      echo "Found duplicates in symbol map file:"
>      echo "$duplicate_symbols"
> 
+1

Reviewed-by: Ray Kinsella <mdr@ashroe.eu>

Re: [dpdk-dev] [PATCH v2 2/2] buildtools: check symbol maps in developer mode

[Thomas Monjalon]

> > Hook check-symbol-maps.sh in the symbol check when in developer mode to
> > help developers catch issues before submitting their changes.
> > 
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> +1
> 
> Reviewed-by: Ray Kinsella <mdr@ashroe.eu>

Series applied, thanks

Re: [dpdk-dev] [PATCH v2 2/2] buildtools: check symbol maps in developer mode

[Thomas Monjalon]

12/05/2021 22:24, Thomas Monjalon:
> > > Hook check-symbol-maps.sh in the symbol check when in developer mode to
> > > help developers catch issues before submitting their changes.
> > > 
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > +1
> > 
> > Reviewed-by: Ray Kinsella <mdr@ashroe.eu>
> 
> Series applied, thanks

Oops, sorry no it cannot be merged as-is because of an incompatibility
with Busybox grep: unrecognized option: exclude=version.map

I think it is because of the old function find_orphan_symbols()
using the option --exclude.

Re: [dpdk-dev] [PATCH v2 2/2] buildtools: check symbol maps in developer mode

[David Marchand]

On Wed, May 12, 2021 at 10:38 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 12/05/2021 22:24, Thomas Monjalon:
> > > > Hook check-symbol-maps.sh in the symbol check when in developer mode to
> > > > help developers catch issues before submitting their changes.
> > > >
> > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > +1
> > >
> > > Reviewed-by: Ray Kinsella <mdr@ashroe.eu>
> >
> > Series applied, thanks
>
> Oops, sorry no it cannot be merged as-is because of an incompatibility
> with Busybox grep: unrecognized option: exclude=version.map
>
> I think it is because of the old function find_orphan_symbols()
> using the option --exclude.

This could be fixed with:
-            if ! grep -q -r --exclude=$(basename $map) \
-                    -w $symsrc $(dirname $map) ; then
+            if [ -z "$(grep -rlw $symsrc $(dirname $map) |grep -v
$map)" ] ; then

Do you want a v3 with this change?


-- 
David Marchand

[dpdk-dev] [PATCH v3 1/3] devtools: fix orphan symbols check with busybox

[David Marchand]

Avoid relying on GNU grep --exclude option.

Fixes: f8ad40dc998c ("devtools: check orphan symbols in map files")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 devtools/check-symbol-maps.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/devtools/check-symbol-maps.sh b/devtools/check-symbol-maps.sh
index f06353fc75..08451ef5aa 100755
--- a/devtools/check-symbol-maps.sh
+++ b/devtools/check-symbol-maps.sh
@@ -20,8 +20,7 @@ find_orphan_symbols ()
             else
                 symsrc=$sym
             fi
-            if ! grep -q -r --exclude=$(basename $map) \
-                    -w $symsrc $(dirname $map) ; then
+            if [ -z "$(grep -rlw $symsrc $(dirname $map) |grep -v $map)" ] ; then
                 echo "$map: $sym"
             fi
         done
-- 
2.23.0

[dpdk-dev] [PATCH v3 2/3] devtools: catch symbol duplicates in version map

[David Marchand]

Add a check on versioned symbol duplicates in map files.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- rebased,

---
 devtools/check-symbol-maps.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/devtools/check-symbol-maps.sh b/devtools/check-symbol-maps.sh
index 08451ef5aa..8a976dc97f 100755
--- a/devtools/check-symbol-maps.sh
+++ b/devtools/check-symbol-maps.sh
@@ -34,4 +34,19 @@ if [ -n "$orphan_symbols" ] ; then
     ret=1
 fi
 
+find_duplicate_symbols ()
+{
+    for map in $(find lib drivers -name '*.map') ; do
+        buildtools/map-list-symbol.sh $map | \
+            sort | uniq -c | grep -v " 1 $map" || true
+    done
+}
+
+duplicate_symbols=$(find_duplicate_symbols)
+if [ -n "$duplicate_symbols" ] ; then
+    echo "Found duplicates in symbol map file:"
+    echo "$duplicate_symbols"
+    ret=1
+fi
+
 exit $ret
-- 
2.23.0

[dpdk-dev] [PATCH v3 3/3] build: check symbol maps in developer mode

[David Marchand]

Hook check-symbol-maps.sh in the symbol check when in developer mode to
help developers catch issues before submitting their changes.

Signed-off-by: David Marchand <david.marchand@redhat.com>
Reviewed-by: Ray Kinsella <mdr@ashroe.eu>
---
Changes since v1:
- now that we have the developer mode, added this check in the build
  process,
---
 buildtools/check-symbols.sh   |  9 ++++++++-
 devtools/check-symbol-maps.sh | 12 ++++++++----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/buildtools/check-symbols.sh b/buildtools/check-symbols.sh
index 83b3a0182f..e458c0af72 100755
--- a/buildtools/check-symbols.sh
+++ b/buildtools/check-symbols.sh
@@ -5,7 +5,9 @@
 MAPFILE=$1
 OBJFILE=$2
 
-LIST_SYMBOL=$(dirname $(readlink -f $0))/map-list-symbol.sh
+ROOTDIR=$(readlink -f $(dirname $(readlink -f $0))/..)
+LIST_SYMBOL=$ROOTDIR/buildtools/map-list-symbol.sh
+CHECK_SYMBOL_MAPS=$ROOTDIR/devtools/check-symbol-maps.sh
 
 # added check for "make -C test/" usage
 if [ ! -e $MAPFILE ] || [ ! -f $OBJFILE ]
@@ -23,6 +25,11 @@ trap 'rm -f "$DUMPFILE"' EXIT
 objdump -t $OBJFILE >$DUMPFILE
 
 ret=0
+
+if ! $CHECK_SYMBOL_MAPS $MAPFILE; then
+	ret=1
+fi
+
 for SYM in `$LIST_SYMBOL -S EXPERIMENTAL $MAPFILE |cut -d ' ' -f 3`
 do
 	if grep -q "\.text.*[[:space:]]$SYM$" $DUMPFILE &&
diff --git a/devtools/check-symbol-maps.sh b/devtools/check-symbol-maps.sh
index 8a976dc97f..23b0a05f78 100755
--- a/devtools/check-symbol-maps.sh
+++ b/devtools/check-symbol-maps.sh
@@ -7,11 +7,15 @@ cd $(dirname $0)/..
 # speed up by ignoring Unicode details
 export LC_ALL=C
 
+if [ $# = 0 ] ; then
+    set -- $(find lib drivers -name '*.map')
+fi
+
 ret=0
 
 find_orphan_symbols ()
 {
-    for map in $(find lib drivers -name '*.map') ; do
+    for map in $@ ; do
         for sym in $(sed -rn 's,^([^}]*_.*);.*$,\1,p' $map) ; do
             if echo $sym | grep -q '^per_lcore_' ; then
                 symsrc=${sym#per_lcore_}
@@ -27,7 +31,7 @@ find_orphan_symbols ()
     done
 }
 
-orphan_symbols=$(find_orphan_symbols)
+orphan_symbols=$(find_orphan_symbols $@)
 if [ -n "$orphan_symbols" ] ; then
     echo "Found only in symbol map file:"
     echo "$orphan_symbols" | sed 's,^,\t,'
@@ -36,13 +40,13 @@ fi
 
 find_duplicate_symbols ()
 {
-    for map in $(find lib drivers -name '*.map') ; do
+    for map in $@ ; do
         buildtools/map-list-symbol.sh $map | \
             sort | uniq -c | grep -v " 1 $map" || true
     done
 }
 
-duplicate_symbols=$(find_duplicate_symbols)
+duplicate_symbols=$(find_duplicate_symbols $@)
 if [ -n "$duplicate_symbols" ] ; then
     echo "Found duplicates in symbol map file:"
     echo "$duplicate_symbols"
-- 
2.23.0

Re: [dpdk-dev] [PATCH v3 3/3] build: check symbol maps in developer mode

[Thomas Monjalon]

13/05/2021 10:34, David Marchand:
> Hook check-symbol-maps.sh in the symbol check when in developer mode to
> help developers catch issues before submitting their changes.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Reviewed-by: Ray Kinsella <mdr@ashroe.eu>

Series applied, thanks.