[RFC,gdb/contrib] Fix gdb/contrib/gdb-add-index.sh for dwz-m-ed execs

Message ID a9b93e9b-3d9b-844c-4db4-d44c2e1a9e9e@suse.de
State New
Headers show
Series
  • [RFC,gdb/contrib] Fix gdb/contrib/gdb-add-index.sh for dwz-m-ed execs
Related show

Commit Message

Tom de Vries May 7, 2019, 4:13 p.m.
[ was: Re: [PATCH][gdb] Write index for dwz -m file ]


Hi,

This is a follow-up patch for "[gdb] Write index for dwz -m file".

Any comments on the updated gdb/contrib/gdb-add-index.sh script?

In particular, I'd like some advice on whether I should add shell
variables (as I've done for readelf) for grep, tail and sed.

Thanks,
- Tom

Comments

Simon Marchi May 12, 2019, 7:49 p.m. | #1
On 2019-05-07 12:13 p.m., Tom de Vries wrote:
> [ was: Re: [PATCH][gdb] Write index for dwz -m file ]

> 

> 

> Hi,

> 

> This is a follow-up patch for "[gdb] Write index for dwz -m file".

> 

> Any comments on the updated gdb/contrib/gdb-add-index.sh script?

> 

> In particular, I'd like some advice on whether I should add shell

> variables (as I've done for readelf) for grep, tail and sed.

> 

> Thanks,

> - Tom

> 


Hi Tom,

I think it can be useful to override gdb, readelf and objcopy, as it is likely
that people will want to use specific versions of these (either newer, or
specific to their architectures).

But it's not very likely for grep, tail and sed, as long as we make sure that
we are compatible with the BSD versions of these tools.  That would mean making
sure we only use features defined by POSIX.

One case that isn't handled correct by GDB and/or the script (with both my and
your patch applied) is running the script on two executables that share the same
external dwz file.  It will fail adding the index to the dwz file the second time.
This use case is kind of important, because the point of having dwz files is to
share it between multiple executables.

This is what I get the second time:

$ GDB="/home/simark/build/binutils-gdb/gdb/gdb --data-directory=/home/simark/build/binutils-gdb/gdb/data-directory" ~/src/binutils-gdb/gdb/contrib/gdb-add-index.sh b
+ objcopy --add-section .gdb_index=shared-debug-info.dwz.gdb-index --set-section-flags .gdb_index=readonly shared-debug-info.dwz shared-debug-info.dwz
objcopy:std9IdCI: can't add section '.gdb_index': file in wrong format

I haven't looked in more details for this problem.

There's a buglet in the clean up code causing the dwz file's index
(shared-debug-info.dwz.gdb-index in my case) to be left in the current directory after
running the script (even successfully).  This fixes it:

---
diff --git a/gdb/contrib/gdb-add-index.sh b/gdb/contrib/gdb-add-index.sh
index afedce0c848d..2b3af2e84f71 100755
--- a/gdb/contrib/gdb-add-index.sh
+++ b/gdb/contrib/gdb-add-index.sh
@@ -70,7 +70,7 @@ for f in "$file" "$dwz_file"; do
     if [ "$f" = "" ]; then
 	continue
     fi
-    set_files "$file"
+    set_files "$f"
     tmp_files="$tmp_files $index4 $index5 $debugstr $debugstrmerge $debugstrerr"
 done
---

Simon
Tom de Vries May 13, 2019, 10:47 a.m. | #2
On 12-05-19 21:49, Simon Marchi wrote:
> On 2019-05-07 12:13 p.m., Tom de Vries wrote:

>> [ was: Re: [PATCH][gdb] Write index for dwz -m file ]

>>

>>

>> Hi,

>>

>> This is a follow-up patch for "[gdb] Write index for dwz -m file".

>>

>> Any comments on the updated gdb/contrib/gdb-add-index.sh script?

>>

>> In particular, I'd like some advice on whether I should add shell

>> variables (as I've done for readelf) for grep, tail and sed.

>>

>> Thanks,

>> - Tom

>>

> 

> Hi Tom,

> 

> I think it can be useful to override gdb, readelf and objcopy, as it is likely

> that people will want to use specific versions of these (either newer, or

> specific to their architectures).

> 

> But it's not very likely for grep, tail and sed, as long as we make sure that

> we are compatible with the BSD versions of these tools.  That would mean making

> sure we only use features defined by POSIX.

> 


Ack, thanks for the insight.

> One case that isn't handled correct by GDB and/or the script (with both my and

> your patch applied) is running the script on two executables that share the same

> external dwz file.  It will fail adding the index to the dwz file the second time.

> This use case is kind of important, because the point of having dwz files is to

> share it between multiple executables.

> 

> This is what I get the second time:

> 

> $ GDB="/home/simark/build/binutils-gdb/gdb/gdb --data-directory=/home/simark/build/binutils-gdb/gdb/data-directory" ~/src/binutils-gdb/gdb/contrib/gdb-add-index.sh b

> + objcopy --add-section .gdb_index=shared-debug-info.dwz.gdb-index --set-section-flags .gdb_index=readonly shared-debug-info.dwz shared-debug-info.dwz

> objcopy:std9IdCI: can't add section '.gdb_index': file in wrong format

> 

> I haven't looked in more details for this problem.

> 


That's fixed now.

[ In a way, it's a known problem. Running gdb-add-index twice on the
same (regular, non-dwz-ed) executable, gives the same kind of error. ]

> There's a buglet in the clean up code causing the dwz file's index

> (shared-debug-info.dwz.gdb-index in my case) to be left in the current directory after

> running the script (even successfully).  This fixes it:

> 

> ---

> diff --git a/gdb/contrib/gdb-add-index.sh b/gdb/contrib/gdb-add-index.sh

> index afedce0c848d..2b3af2e84f71 100755

> --- a/gdb/contrib/gdb-add-index.sh

> +++ b/gdb/contrib/gdb-add-index.sh

> @@ -70,7 +70,7 @@ for f in "$file" "$dwz_file"; do

>      if [ "$f" = "" ]; then

>  	continue

>      fi

> -    set_files "$file"

> +    set_files "$f"

>      tmp_files="$tmp_files $index4 $index5 $debugstr $debugstrmerge $debugstrerr"

>  done


Ok, I've incorporated that fix, as well as making the handle_file
$dwz_file conditional on $dwz_file != "".

Updated version attached.

Thanks,
- Tom
[gdb/contrib] Fix gdb/contrib/gdb-add-index.sh for dwz-m-ed execs

Atm gdb-add-index.exp fails with target board cc-with-dwz-m.

Fix this by updating gdb/contrib/gdb-add-index.sh to handle a dwz-m-ed
executable.

Tested on x86_64-linux.

gdb/ChangeLog:

2019-05-07  Tom de Vries  <tdevries@suse.de>

	PR gdb/24445
	* contrib/gdb-add-index.sh: Update to handle dwz-m-ed executable.

---
 gdb/contrib/gdb-add-index.sh | 138 ++++++++++++++++++++++++++++---------------
 1 file changed, 91 insertions(+), 47 deletions(-)

diff --git a/gdb/contrib/gdb-add-index.sh b/gdb/contrib/gdb-add-index.sh
index efaad1dce7..5a37020656 100755
--- a/gdb/contrib/gdb-add-index.sh
+++ b/gdb/contrib/gdb-add-index.sh
@@ -20,6 +20,7 @@
 # If not, or you want others, pass the following in the environment
 GDB=${GDB:=gdb}
 OBJCOPY=${OBJCOPY:=objcopy}
+READELF=${READELF:=readelf}
 
 myname="${0##*/}"
 
@@ -43,15 +44,44 @@ fi
 
 dir="${file%/*}"
 test "$dir" = "$file" && dir="."
-index4="${file}.gdb-index"
-index5="${file}.debug_names"
-debugstr="${file}.debug_str"
-debugstrmerge="${file}.debug_str.merge"
-debugstrerr="${file}.debug_str.err"
 
-rm -f $index4 $index5 $debugstr $debugstrmerge $debugstrerr
+dwz_file=""
+if $READELF -S "$file" | grep -q " \.gnu_debugaltlink "; then
+    dwz_file=$($READELF --string-dump=.gnu_debugaltlink "$file" \
+		   | grep -A1  "'\.gnu_debugaltlink':" \
+		   | tail -n +2 \
+		   | sed 's/.*]//')
+    dwz_file=$(echo $dwz_file)
+    if $READELF -S "$dwz_file" | grep -E -q " \.(gdb_index|debug_names) "; then
+	# Already has an index, skip it.
+	dwz_file=""
+    fi
+fi
+
+set_files ()
+{
+    local file="$1"
+
+    index4="${file}.gdb-index"
+    index5="${file}.debug_names"
+    debugstr="${file}.debug_str"
+    debugstrmerge="${file}.debug_str.merge"
+    debugstrerr="${file}.debug_str.err"
+}
+
+tmp_files=
+for f in "$file" "$dwz_file"; do
+    if [ "$f" = "" ]; then
+	continue
+    fi
+    set_files "$f"
+    tmp_files="$tmp_files $index4 $index5 $debugstr $debugstrmerge $debugstrerr"
+done
+
+rm -f $tmp_files
+
 # Ensure intermediate index file is removed when we exit.
-trap "rm -f $index4 $index5 $debugstr $debugstrmerge $debugstrerr" 0
+trap "rm -f $tmp_files" 0
 
 $GDB --batch -nx -iex 'set auto-load no' \
     -ex "file $file" -ex "save gdb-index $dwarf5 $dir" || {
@@ -67,50 +97,64 @@ $GDB --batch -nx -iex 'set auto-load no' \
 # already stripped binary, it's a no-op.
 status=0
 
-if test -f "$index4" -a -f "$index5"; then
-    echo "$myname: Both index types were created for $file" 1>&2
-    status=1
-elif test -f "$index4" -o -f "$index5"; then
-    if test -f "$index4"; then
-	index="$index4"
-	section=".gdb_index"
-    else
-	index="$index5"
-	section=".debug_names"
-    fi
-    debugstradd=false
-    debugstrupdate=false
-    if test -s "$debugstr"; then
-	if ! $OBJCOPY --dump-section .debug_str="$debugstrmerge" "$file" /dev/null \
-		 2>$debugstrerr; then
-	    cat >&2 $debugstrerr
-	    exit 1
-	fi
-	if grep -q "can't dump section '.debug_str' - it does not exist" \
-		  $debugstrerr; then
-	    debugstradd=true
+handle_file ()
+{
+    local file
+    file="$1"
+
+    set_files "$file"
+
+    if test -f "$index4" -a -f "$index5"; then
+	echo "$myname: Both index types were created for $file" 1>&2
+	status=1
+    elif test -f "$index4" -o -f "$index5"; then
+	if test -f "$index4"; then
+	    index="$index4"
+	    section=".gdb_index"
 	else
-	    debugstrupdate=true
-	    cat >&2 $debugstrerr
+	    index="$index5"
+	    section=".debug_names"
+	fi
+	debugstradd=false
+	debugstrupdate=false
+	if test -s "$debugstr"; then
+	    if ! $OBJCOPY --dump-section .debug_str="$debugstrmerge" "$file" \
+		 /dev/null 2>$debugstrerr; then
+		cat >&2 $debugstrerr
+		exit 1
+	    fi
+	    if grep -q "can't dump section '.debug_str' - it does not exist" \
+		    $debugstrerr; then
+		debugstradd=true
+	    else
+		debugstrupdate=true
+		cat >&2 $debugstrerr
+	    fi
+	    cat "$debugstr" >>"$debugstrmerge"
 	fi
-	cat "$debugstr" >>"$debugstrmerge"
-    fi
 
-    $OBJCOPY --add-section $section="$index" \
-	--set-section-flags $section=readonly \
-	$(if $debugstradd; then \
-	      echo --add-section .debug_str="$debugstrmerge"; \
-	      echo --set-section-flags .debug_str=readonly; \
-	  fi; \
-	  if $debugstrupdate; then \
-	      echo --update-section .debug_str="$debugstrmerge"; \
-	  fi) \
-	"$file" "$file"
+	$OBJCOPY --add-section $section="$index" \
+		 --set-section-flags $section=readonly \
+		 $(if $debugstradd; then \
+		       echo --add-section .debug_str="$debugstrmerge"; \
+		       echo --set-section-flags .debug_str=readonly; \
+		   fi; \
+		   if $debugstrupdate; then \
+		       echo --update-section .debug_str="$debugstrmerge"; \
+		   fi) \
+		 "$file" "$file"
+
+	status=$?
+    else
+	echo "$myname: No index was created for $file" 1>&2
+	echo "$myname: [Was there no debuginfo? Was there already an index?]" \
+	     1>&2
+    fi
+}
 
-    status=$?
-else
-    echo "$myname: No index was created for $file" 1>&2
-    echo "$myname: [Was there no debuginfo? Was there already an index?]" 1>&2
+handle_file "$file"
+if [ "$dwz_file" != "" ]; then
+    handle_file "$dwz_file"
 fi
 
 exit $status

Patch

[gdb/contrib] Fix gdb/contrib/gdb-add-index.sh for dwz-m-ed execs

Atm gdb-add-index.exp fails with target board cc-with-dwz-m.

Fix this by updating gdb/contrib/gdb-add-index.sh to handle a dwz-m-ed
executable.

Tested on x86_64-linux.

gdb/ChangeLog:

2019-05-07  Tom de Vries  <tdevries@suse.de>

	PR gdb/24445
	* contrib/gdb-add-index.sh: Update to handle dwz-m-ed executable.

---
 gdb/contrib/gdb-add-index.sh | 134 +++++++++++++++++++++++++++----------------
 1 file changed, 86 insertions(+), 48 deletions(-)

diff --git a/gdb/contrib/gdb-add-index.sh b/gdb/contrib/gdb-add-index.sh
index efaad1dce7..afedce0c84 100755
--- a/gdb/contrib/gdb-add-index.sh
+++ b/gdb/contrib/gdb-add-index.sh
@@ -20,6 +20,7 @@ 
 # If not, or you want others, pass the following in the environment
 GDB=${GDB:=gdb}
 OBJCOPY=${OBJCOPY:=objcopy}
+READELF=${READELF:=readelf}
 
 myname="${0##*/}"
 
@@ -43,15 +44,40 @@  fi
 
 dir="${file%/*}"
 test "$dir" = "$file" && dir="."
-index4="${file}.gdb-index"
-index5="${file}.debug_names"
-debugstr="${file}.debug_str"
-debugstrmerge="${file}.debug_str.merge"
-debugstrerr="${file}.debug_str.err"
 
-rm -f $index4 $index5 $debugstr $debugstrmerge $debugstrerr
+dwz_file=""
+if $READELF -S "$file" | grep -q " \.gnu_debugaltlink "; then
+    dwz_file=$($READELF --string-dump=.gnu_debugaltlink "$file" \
+		   | grep -A1  "'\.gnu_debugaltlink':" \
+		   | tail -n +2 \
+		   | sed 's/.*]//')
+    dwz_file=$(echo $dwz_file)
+fi
+
+set_files ()
+{
+    local file="$1"
+
+    index4="${file}.gdb-index"
+    index5="${file}.debug_names"
+    debugstr="${file}.debug_str"
+    debugstrmerge="${file}.debug_str.merge"
+    debugstrerr="${file}.debug_str.err"
+}
+
+tmp_files=
+for f in "$file" "$dwz_file"; do
+    if [ "$f" = "" ]; then
+	continue
+    fi
+    set_files "$file"
+    tmp_files="$tmp_files $index4 $index5 $debugstr $debugstrmerge $debugstrerr"
+done
+
+rm -f $tmp_files
+
 # Ensure intermediate index file is removed when we exit.
-trap "rm -f $index4 $index5 $debugstr $debugstrmerge $debugstrerr" 0
+trap "rm -f $tmp_files" 0
 
 $GDB --batch -nx -iex 'set auto-load no' \
     -ex "file $file" -ex "save gdb-index $dwarf5 $dir" || {
@@ -67,50 +93,62 @@  $GDB --batch -nx -iex 'set auto-load no' \
 # already stripped binary, it's a no-op.
 status=0
 
-if test -f "$index4" -a -f "$index5"; then
-    echo "$myname: Both index types were created for $file" 1>&2
-    status=1
-elif test -f "$index4" -o -f "$index5"; then
-    if test -f "$index4"; then
-	index="$index4"
-	section=".gdb_index"
-    else
-	index="$index5"
-	section=".debug_names"
-    fi
-    debugstradd=false
-    debugstrupdate=false
-    if test -s "$debugstr"; then
-	if ! $OBJCOPY --dump-section .debug_str="$debugstrmerge" "$file" /dev/null \
-		 2>$debugstrerr; then
-	    cat >&2 $debugstrerr
-	    exit 1
-	fi
-	if grep -q "can't dump section '.debug_str' - it does not exist" \
-		  $debugstrerr; then
-	    debugstradd=true
+handle_file ()
+{
+    local file
+    file="$1"
+
+    set_files "$file"
+
+    if test -f "$index4" -a -f "$index5"; then
+	echo "$myname: Both index types were created for $file" 1>&2
+	status=1
+    elif test -f "$index4" -o -f "$index5"; then
+	if test -f "$index4"; then
+	    index="$index4"
+	    section=".gdb_index"
 	else
-	    debugstrupdate=true
-	    cat >&2 $debugstrerr
+	    index="$index5"
+	    section=".debug_names"
+	fi
+	debugstradd=false
+	debugstrupdate=false
+	if test -s "$debugstr"; then
+	    if ! $OBJCOPY --dump-section .debug_str="$debugstrmerge" "$file" \
+		 /dev/null 2>$debugstrerr; then
+		cat >&2 $debugstrerr
+		exit 1
+	    fi
+	    if grep -q "can't dump section '.debug_str' - it does not exist" \
+		    $debugstrerr; then
+		debugstradd=true
+	    else
+		debugstrupdate=true
+		cat >&2 $debugstrerr
+	    fi
+	    cat "$debugstr" >>"$debugstrmerge"
 	fi
-	cat "$debugstr" >>"$debugstrmerge"
-    fi
 
-    $OBJCOPY --add-section $section="$index" \
-	--set-section-flags $section=readonly \
-	$(if $debugstradd; then \
-	      echo --add-section .debug_str="$debugstrmerge"; \
-	      echo --set-section-flags .debug_str=readonly; \
-	  fi; \
-	  if $debugstrupdate; then \
-	      echo --update-section .debug_str="$debugstrmerge"; \
-	  fi) \
-	"$file" "$file"
+	$OBJCOPY --add-section $section="$index" \
+		 --set-section-flags $section=readonly \
+		 $(if $debugstradd; then \
+		       echo --add-section .debug_str="$debugstrmerge"; \
+		       echo --set-section-flags .debug_str=readonly; \
+		   fi; \
+		   if $debugstrupdate; then \
+		       echo --update-section .debug_str="$debugstrmerge"; \
+		   fi) \
+		 "$file" "$file"
+
+	status=$?
+    else
+	echo "$myname: No index was created for $file" 1>&2
+	echo "$myname: [Was there no debuginfo? Was there already an index?]" \
+	     1>&2
+    fi
+}
 
-    status=$?
-else
-    echo "$myname: No index was created for $file" 1>&2
-    echo "$myname: [Was there no debuginfo? Was there already an index?]" 1>&2
-fi
+handle_file "$file"
+handle_file "$dwz_file"
 
 exit $status