[PATCH 1/2] gdb/testsuite: make gdb_gnu_strip_debug consistent
Andrew Burgess
aburgess@redhat.com
Sun May 26 23:08:04 GMT 2024
Tom de Vries <tdevries@suse.de> writes:
> On 5/20/24 16:14, Andrew Burgess wrote:
>> While writing a test I realised that the default behaviour of
>> gdb_gnu_strip_debug doesn't match its comment.
>>
>> The comment says that the function takes a FILENAME, and splits the
>> file into FILENAME.stripped and FILENAME.debug, leaving FILENAME
>> unchanged. The comment says that a .gnu_debuglink will be added to
>> FILENAME.stripped.
>>
>> However, this is not true, FILENAME.stripped is created, with no debug
>> information. FILENAME.debug is created containing the debug
>> information.
>>
>> But, when adding the .gnu_debuglink we take FILENAME.stripped as the
>> input, and then overwrite FILENAME with the output. As a result,
>> FILENAME.stripped does not include a .gnu_debuglink, while FILENAME
>> contains the .gnu_debuglink and no debug information!
>>
>> The users of gdb_gnu_strip_debug can be split into two groups, those
>> who are using the .gnu_debuglink, these tests are all written assuming
>> that FILENAME is updated.
>>
>> Then there are some tests that rely on gdb_gnu_strip_debug's ability
>> to split out the debug information,
>
> I got confused a bit here. Doesn't the first group also rely on this?
>
>> these tests are then going to do a
>> lookup based on the build-id. These tests use the FILENAME.stripped
>> output file.
>>
>> This all seems too confused to me.
>>
>> As most used of gdb_gnu_strip_debug assume that FILENAME is updated, I
>
> used -> uses
>
>> propose that we just make that the actual, advertised behaviour of
>> this proc.
>>
>
> Make sense.
>
> [ FWIW, the overwriting behaviour is not great for the case where the
> exec is used in the initial part of a test-case, and then modified by
> gdb_gnu_strip_debug (I don't know whether that occurs though). When
> trying to reproduce the initial part of the tests using say -x gdb.in.1
> the behaviour may be different because the exec has changed in the mean
> time. So going from that point of view we could have gone for the
> syntax of gdb_gnu_strip_debug returning the filename of the stripped
> exec, if successful. Come to think of it, doing so might also allow us
> to name the stripped exec differently, say foo.stripped and
> foo.stripped-with-link, which will make it more clear in gdb.log what
> exec is used. ]
I see what you're saying here. But I'm really not super keen to start
rewriting all the tests that use gdb_gnu_strip_debug right now :-/ As
most of the tests that call this proc are already written assuming the
overwrite behaviour.
Unless you strongly object then I'm planning to just go with this as it
is right now. But if it really bothers you I can tackle fixing all the
tests. Let me know.
Anyway, updated patch with the minor type fixes, and slightly clearer
commit message is below, just in case you wanted another look.
Thanks,
Andrew
---
commit b888aea514e5eb968ef0e2e30f8042848f88fc90
Author: Andrew Burgess <aburgess@redhat.com>
Date: Sat May 18 11:05:49 2024 +0100
gdb/testsuite: make gdb_gnu_strip_debug consistent
While writing a test I realised that the default behaviour of
gdb_gnu_strip_debug doesn't match its comment.
The comment says that the function takes a FILENAME, and splits the
file into FILENAME.stripped and FILENAME.debug, leaving FILENAME
unchanged. The comment says that a .gnu_debuglink will be added to
FILENAME.stripped.
However, this is not true, FILENAME.stripped is created, with no debug
information. FILENAME.debug is created containing the debug
information.
But, when adding the .gnu_debuglink we take FILENAME.stripped as the
input, and then overwrite FILENAME with the output. As a result,
FILENAME.stripped does not include a .gnu_debuglink, while FILENAME
contains the .gnu_debuglink and no debug information!
The users of gdb_gnu_strip_debug can be split into two groups, those
who are using the .gnu_debuglink, these tests are all written assuming
that FILENAME is updated.
Then there are some tests that only rely on gdb_gnu_strip_debug's
ability to split out the debug information, these tests are then going
to do a lookup based on the build-id, these tests don't require the
.gnu_debuglink. These tests use the FILENAME.stripped output file.
This all seems too confused to me.
As most uses of gdb_gnu_strip_debug assume that FILENAME is updated, I
propose that we just make that the actual, advertised behaviour of
this proc.
So now, gdb_gnu_strip_debug will take FILENAME, and will split the
debug information out into FILENAME.debug. The debug information will
then be stripped from FILENAME, and by default a .gnu_debuglink will
be added to FILENAME pointing to FILENAME.debug.
I've updated the two tests that actually relied on FILENAME.stripped
to instead just use FILENAME.
One of the tests was doing a build-id based lookup, but was still
allowing the .gnu_debuglink to be added to FILENAME, I've updated this
test to pass the no-debuglink flag to gdb_gnu_strip_debug, which stops
the .gnu_debuglink from being added.
All of the tests that call gdb_gnu_strip_debug still pass for me.
Acked-By: Tom de Vries <tdevries@suse.de>
diff --git a/gdb/testsuite/gdb.base/corefile-buildid.exp b/gdb/testsuite/gdb.base/corefile-buildid.exp
index 130198611ec..e1b9804d891 100644
--- a/gdb/testsuite/gdb.base/corefile-buildid.exp
+++ b/gdb/testsuite/gdb.base/corefile-buildid.exp
@@ -172,11 +172,9 @@ proc locate_exec_from_core_build_id {corefile buildid suffix \
"mkdir -p [file join $debugdir [file dirname $buildid]]"
set files_list {}
+ lappend files_list $binfile $buildid
if {$sepdebug} {
- lappend files_list "$binfile.stripped" $buildid
lappend files_list "$binfile.debug" "$buildid.debug"
- } else {
- lappend files_list $binfile $buildid
}
if {$shared} {
global sharedir
@@ -200,12 +198,7 @@ proc locate_exec_from_core_build_id {corefile buildid suffix \
gdb_test "core-file $corefile" "Program terminated with .*" \
"load core file"
if {$symlink} {
- if {$sepdebug} {
- set expected_file [file join $builddir \
- [file tail "$binfile.stripped"]]
- } else {
- set expected_file [file join $builddir [file tail $binfile]]
- }
+ set expected_file [file join $builddir [file tail $binfile]]
} else {
set expected_file $buildid
}
@@ -245,15 +238,12 @@ proc do_corefile_buildid_tests {args} {
if {$sepdebug} {
# Strip debuginfo into its own file.
- if {[gdb_gnu_strip_debug [standard_output_file $program_to_run]] \
- != 0} {
+ if {[gdb_gnu_strip_debug [standard_output_file $program_to_run] \
+ no-debuglink] != 0} {
untested "could not strip executable for [join $suffix \ ]"
return
}
- # Run the stripped program instead of the original.
- set program_to_run [file join $builddir \
- [file tail "$binfile.stripped"]]
lappend suffix "sepdebug"
}
diff --git a/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp b/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp
index b86622543ef..aa1c263e800 100644
--- a/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp
+++ b/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp
@@ -30,14 +30,11 @@ if {[build_executable "build executable" ${testfile} ${srcfile} \
return -1
}
-# Split BINFILE into BINFILE.stripped and BINFILE.debug, the first is
-# the executable with the debug information removed, and the second is
-# the debug information.
+# Split debug information from BINFILE into BINFILE.debug.
#
-# However, by passing the "no-debuglink" flag we prevent this proc
-# from adding a .gnu_debuglink section to the executable. Any lookup
-# of the debug information by GDB will need to be done based on the
-# build-id.
+# By passing the "no-debuglink" flag we prevent this proc from adding
+# a .gnu_debuglink section to BINFILE. Any lookup of the debug
+# information by GDB will need to be done based on the build-id.
if {[gdb_gnu_strip_debug $binfile no-debuglink]} {
unsupported "cannot produce separate debug info files"
return -1
@@ -59,12 +56,6 @@ set debuginfod_debugdir [standard_output_file "debug"]
remote_exec build "mkdir $debuginfod_debugdir"
remote_exec build "mv $debugfile $debuginfod_debugdir"
-# This is BINFILE with the debug information removed. We are going to
-# place this in the BUILD_ID_DEBUG_FILE location, this would usually
-# represent a mistake by the user, and will trigger a warning from
-# GDB, this is the warning we are checking for.
-set stripped_binfile [standard_output_file "${binfile}.stripped"]
-
# Create the .build-id/PREFIX directory name from
# .build-id/PREFIX/SUFFIX.debug filename.
set debugdir [file dirname ${build_id_debug_file}]
@@ -76,7 +67,7 @@ remote_exec build "mkdir -p $debugdir"
# information, which will point back at this file, which also doesn't
# have debug information, which could cause a loop. But GDB will spot
# this and give a warning.
-remote_exec build "mv ${stripped_binfile} ${build_id_debug_file}"
+remote_exec build "mv ${binfile} ${build_id_debug_file}"
# Now start GDB.
clean_restart
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 0d78691c381..a63c13f9cbc 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -8017,10 +8017,11 @@ proc build_id_debug_filename_get { filename } {
}
# DEST should be a file compiled with debug information. This proc
-# creates two new files DEST.debug which contains the debug
-# information extracted from DEST, and DEST.stripped, which is a copy
-# of DEST with the debug information removed. A '.gnu_debuglink'
-# section will be added to DEST.stripped that points to DEST.debug.
+# creates DEST.debug which contains the debug information extracted
+# from DEST, and DEST is updated with the debug information removed.
+#
+# By default a '.gnu_debuglink' section will be added to DEST that
+# points to DEST.debug.
#
# If ARGS is passed, it is a list of optional flags. The currently
# supported flags are:
@@ -8028,7 +8029,7 @@ proc build_id_debug_filename_get { filename } {
# - no-main : remove the symbol entry for main from the separate
# debug file DEST.debug,
# - no-debuglink : don't add the '.gnu_debuglink' section to
-# DEST.stripped.
+# DEST.
#
# Function returns zero on success. Function will return non-zero failure code
# on some targets not supporting separate debug info (such as i386-msdos).
@@ -8087,20 +8088,26 @@ proc gdb_gnu_strip_debug { dest args } {
# Unless the "no-debuglink" flag is passed, then link the two
# previous output files together, adding the .gnu_debuglink
# section to the stripped_file, containing a pointer to the
- # debug_file, save the new file in dest.
+ # debug_file.
if {[lsearch -exact $args "no-debuglink"] == -1} {
- set result [catch "exec $objcopy_program --add-gnu-debuglink=${debug_file} ${stripped_file} ${dest}" output]
+ set result [catch "exec $objcopy_program --add-gnu-debuglink=${debug_file} ${stripped_file} ${stripped_file}-tmp" output]
verbose "result is $result"
verbose "output is $output"
if {$result == 1} {
return 1
}
+ file delete "${stripped_file}"
+ file rename "${stripped_file}-tmp" "${stripped_file}"
}
# Workaround PR binutils/10802:
# Preserve the 'x' bit also for PIEs (Position Independent Executables).
- set perm [file attributes ${stripped_file} -permissions]
- file attributes ${dest} -permissions $perm
+ set perm [file attributes ${dest} -permissions]
+ file attributes ${stripped_file} -permissions $perm
+
+ # Move the stripped_file back into dest.
+ file delete ${dest}
+ file rename ${stripped_file} ${dest}
return 0
}
More information about the Gdb-patches
mailing list