[PATCH] Add an optional offset option to the "symbol-file" command

[PATCH] Add an optional offset option to the "symbol-file" command

[Petr Tesarik]

If the main file is relocated at runtime, all symbols are offset by
a fixed amount.  Let the user specify this offset when loading a
symbol file.  The Linux kernel with kASLR is one such example.

gdb/ChangeLog:
2018-04-27  Petr Tesarik  <ptesarik@suse.com>

	* symfile.c (symbol_file_command, symbol_file_add_main_1)
	(_initialize_symfile): Add option "-o" to symbol-file to add an
	offset to each section of the symbol file.

gdb/doc/ChangeLog:
2018-04-27  Petr Tesarik  <ptesarik@suse.com>

	* gdb.texinfo (Files): Document "symbol-file -o offset".

gdb/testsuite/ChangeLog:
2018-04-27  Petr Tesarik  <ptesarik@suse.com>

	* gdb.base/relocate.exp: Add test for "symbol-file -o ".
---
 gdb/ChangeLog                       |  6 ++++++
 gdb/doc/ChangeLog                   |  4 ++++
 gdb/doc/gdb.texinfo                 |  7 ++++++-
 gdb/symfile.c                       | 28 +++++++++++++++++++++-------
 gdb/testsuite/ChangeLog             |  4 ++++
 gdb/testsuite/gdb.base/relocate.exp | 33 +++++++++++++++++++++++++++++++++
 6 files changed, 74 insertions(+), 8 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index cd86be7fb3..56423e7044 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2018-04-27  Petr Tesarik  <ptesarik@suse.com>
+
+	* symfile.c (symbol_file_command, symbol_file_add_main_1)
+	(_initialize_symfile): Add option "-o" to symbol-file to add an
+	offset to each section of the symbol file.
+
 2018-04-26  Andrzej Kaczmarek  <andrzej.kaczmarek@codecoup.pl>
 
 	PR remote/9665
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 83d48781f9..7a67b80cda 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,7 @@
+2018-04-27  Petr Tesarik  <ptesarik@suse.com>
+
+	* gdb.texinfo (Files): Document "symbol-file -o offset".
+
 2018-04-13  Andreas Arnez  <arnez@linux.vnet.ibm.com>
 
 	* gdb.texinfo (Symbols): Mention the fact that "info
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 28f083f96e..56a36d8225 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -18819,11 +18819,16 @@ if necessary to locate your program.  Omitting @var{filename} means to
 discard information on the executable file.
 
 @kindex symbol-file
-@item symbol-file @r{[} @var{filename} @r{]}
+@item symbol-file @r{[} -o @var{offset} @r{]} @r{[} @var{filename} @r{]}
 Read symbol table information from file @var{filename}.  @code{PATH} is
 searched when necessary.  Use the @code{file} command to get both symbol
 table and program to run from the same file.
 
+If an optional @var{offset} is specified, it is added to the start
+address of each section in the symbol file.  This is useful if the
+program is relocated at runtime, such as the Linux kernel with kASLR
+enabled.
+
 @code{symbol-file} with no argument clears out @value{GDBN} information on your
 program's symbol table.
 
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 1e5297ee29..41788f0a67 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -87,7 +87,7 @@ int readnever_symbol_files;	/* Never read full symbols.  */
 /* Functions this file defines.  */
 
 static void symbol_file_add_main_1 (const char *args, symfile_add_flags add_flags,
-				    objfile_flags flags);
+				    objfile_flags flags, CORE_ADDR offset);
 
 static const struct sym_fns *find_sym_fns (bfd *);
 
@@ -1222,16 +1222,20 @@ symbol_file_add (const char *name, symfile_add_flags add_flags,
 void
 symbol_file_add_main (const char *args, symfile_add_flags add_flags)
 {
-  symbol_file_add_main_1 (args, add_flags, 0);
+  symbol_file_add_main_1 (args, add_flags, 0, 0);
 }
 
 static void
 symbol_file_add_main_1 (const char *args, symfile_add_flags add_flags,
-			objfile_flags flags)
+			objfile_flags flags, CORE_ADDR offset)
 {
+  struct objfile *objfile;
+
   add_flags |= current_inferior ()->symfile_flags | SYMFILE_MAINLINE;
 
-  symbol_file_add (args, add_flags, NULL, flags);
+  objfile = symbol_file_add (args, add_flags, NULL, flags);
+  if (offset != 0)
+    objfile_rebase (objfile, offset);
 
   /* Getting new symbols may change our opinion about
      what is frameless.  */
@@ -1548,6 +1552,8 @@ symbol_file_command (const char *args, int from_tty)
       symfile_add_flags add_flags = 0;
       char *name = NULL;
       bool stop_processing_options = false;
+      bool expecting_offset = false;
+      CORE_ADDR offset = 0;
       int idx;
       char *arg;
 
@@ -1559,7 +1565,12 @@ symbol_file_command (const char *args, int from_tty)
 	{
 	  if (stop_processing_options || *arg != '-')
 	    {
-	      if (name == NULL)
+	      if (expecting_offset)
+		{
+		  offset = parse_and_eval_address (arg);
+		  expecting_offset = false;
+		}
+	      else if (name == NULL)
 		name = arg;
 	      else
 		error (_("Unrecognized argument \"%s\""), arg);
@@ -1568,6 +1579,8 @@ symbol_file_command (const char *args, int from_tty)
 	    flags |= OBJF_READNOW;
 	  else if (strcmp (arg, "-readnever") == 0)
 	    flags |= OBJF_READNEVER;
+	  else if (strcmp (arg, "-o") == 0)
+	    expecting_offset = true;
 	  else if (strcmp (arg, "--") == 0)
 	    stop_processing_options = true;
 	  else
@@ -1579,7 +1592,7 @@ symbol_file_command (const char *args, int from_tty)
 
       validate_readnow_readnever (flags);
 
-      symbol_file_add_main_1 (name, add_flags, flags);
+      symbol_file_add_main_1 (name, add_flags, flags, offset);
     }
 }
 
@@ -3772,7 +3785,8 @@ symbolic debug information."
 
   c = add_cmd ("symbol-file", class_files, symbol_file_command, _("\
 Load symbol table from executable file FILE.\n\
-Usage: symbol-file [-readnow | -readnever] FILE\n\
+Usage: symbol-file [-readnow | -readnever] [-o <OFF>] FILE\n\
+OFF is an optional offset which is added to each section address.\n\
 The `file' command can also load symbol tables, as well as setting the file\n\
 to execute.\n" READNOW_READNEVER_HELP), &cmdlist);
   set_cmd_completer (c, filename_completer);
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 34da102c62..68431cb035 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2018-04-27  Petr Tesarik  <ptesarik@suse.com>
+
+	* gdb.base/relocate.exp: Add test for "symbol-file -o ".
+
 2018-04-26  Pedro Alves  <palves@redhat.com>
 
 	* gdb.base/gnu-ifunc.exp (set-break): Test that GDB resolves
diff --git a/gdb/testsuite/gdb.base/relocate.exp b/gdb/testsuite/gdb.base/relocate.exp
index 89f2fffcd9..4383e79cb2 100644
--- a/gdb/testsuite/gdb.base/relocate.exp
+++ b/gdb/testsuite/gdb.base/relocate.exp
@@ -196,6 +196,39 @@ if { "${function_foo_addr}" == "${new_function_foo_addr}" } {
   pass "function foo has a different address"
 }
 
+# Load the object using symbol-file with an offset and check that
+# all addresses are moved by that offset.
+
+set offset 0x10000
+clean_restart
+gdb_test "symbol-file -o $offset $binfile" \
+    "Reading symbols from ${binfile}\.\.\.done\." \
+    "symbol-file with offset"
+
+# Make sure the address of a static variable is moved by offset.
+set new_static_foo_addr [get_var_address static_foo]
+if { "${new_static_foo_addr}" == "${static_foo_addr}" + $offset } {
+  pass "static variable foo is moved by offset"
+} else {
+  fail "static variable foo is moved by offset"
+}
+
+# Make sure the address of a global variable is moved by offset.
+set new_global_foo_addr [get_var_address global_foo]
+if { "${new_global_foo_addr}" == "${global_foo_addr}" + $offset } {
+  pass "global variable foo is moved by offset"
+} else {
+  fail "global variable foo is moved by offset"
+}
+
+# Make sure the address of a functaion is moved by offset.
+set new_function_foo_addr [get_var_address function_foo]
+if { "${new_function_foo_addr}" == "${function_foo_addr}" + $offset } {
+  pass "function foo is moved by offset"
+} else {
+  fail "function foo is moved by offset"
+}
+
 # Now try loading the object as an exec-file; we should be able to print
 # the values of variables after we do this.
 
-- 
2.13.6

Re: [PATCH] Add an optional offset option to the "symbol-file" command

[Petr Tesarik]

Hi all,

any comment on my patch? If it's not good, can you elaborate on what
needs improvement, please?

Petr T

On Fri, 27 Apr 2018 11:24:49 +0200
Petr Tesarik <ptesarik@suse.cz> wrote:

> If the main file is relocated at runtime, all symbols are offset by
> a fixed amount.  Let the user specify this offset when loading a
> symbol file.  The Linux kernel with kASLR is one such example.
> 
> gdb/ChangeLog:
> 2018-04-27  Petr Tesarik  <ptesarik@suse.com>
> 
> 	* symfile.c (symbol_file_command, symbol_file_add_main_1)
> 	(_initialize_symfile): Add option "-o" to symbol-file to add an
> 	offset to each section of the symbol file.
> 
> gdb/doc/ChangeLog:
> 2018-04-27  Petr Tesarik  <ptesarik@suse.com>
> 
> 	* gdb.texinfo (Files): Document "symbol-file -o offset".
> 
> gdb/testsuite/ChangeLog:
> 2018-04-27  Petr Tesarik  <ptesarik@suse.com>
> 
> 	* gdb.base/relocate.exp: Add test for "symbol-file -o ".
> ---
>  gdb/ChangeLog                       |  6 ++++++
>  gdb/doc/ChangeLog                   |  4 ++++
>  gdb/doc/gdb.texinfo                 |  7 ++++++-
>  gdb/symfile.c                       | 28 +++++++++++++++++++++-------
>  gdb/testsuite/ChangeLog             |  4 ++++
>  gdb/testsuite/gdb.base/relocate.exp | 33 +++++++++++++++++++++++++++++++++
>  6 files changed, 74 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index cd86be7fb3..56423e7044 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2018-04-27  Petr Tesarik  <ptesarik@suse.com>
> +
> +	* symfile.c (symbol_file_command, symbol_file_add_main_1)
> +	(_initialize_symfile): Add option "-o" to symbol-file to add an
> +	offset to each section of the symbol file.
> +
>  2018-04-26  Andrzej Kaczmarek  <andrzej.kaczmarek@codecoup.pl>
>  
>  	PR remote/9665
> diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
> index 83d48781f9..7a67b80cda 100644
> --- a/gdb/doc/ChangeLog
> +++ b/gdb/doc/ChangeLog
> @@ -1,3 +1,7 @@
> +2018-04-27  Petr Tesarik  <ptesarik@suse.com>
> +
> +	* gdb.texinfo (Files): Document "symbol-file -o offset".
> +
>  2018-04-13  Andreas Arnez  <arnez@linux.vnet.ibm.com>
>  
>  	* gdb.texinfo (Symbols): Mention the fact that "info
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 28f083f96e..56a36d8225 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -18819,11 +18819,16 @@ if necessary to locate your program.  Omitting @var{filename} means to
>  discard information on the executable file.
>  
>  @kindex symbol-file
> -@item symbol-file @r{[} @var{filename} @r{]}
> +@item symbol-file @r{[} -o @var{offset} @r{]} @r{[} @var{filename} @r{]}
>  Read symbol table information from file @var{filename}.  @code{PATH} is
>  searched when necessary.  Use the @code{file} command to get both symbol
>  table and program to run from the same file.
>  
> +If an optional @var{offset} is specified, it is added to the start
> +address of each section in the symbol file.  This is useful if the
> +program is relocated at runtime, such as the Linux kernel with kASLR
> +enabled.
> +
>  @code{symbol-file} with no argument clears out @value{GDBN} information on your
>  program's symbol table.
>  
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 1e5297ee29..41788f0a67 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -87,7 +87,7 @@ int readnever_symbol_files;	/* Never read full symbols.  */
>  /* Functions this file defines.  */
>  
>  static void symbol_file_add_main_1 (const char *args, symfile_add_flags add_flags,
> -				    objfile_flags flags);
> +				    objfile_flags flags, CORE_ADDR offset);
>  
>  static const struct sym_fns *find_sym_fns (bfd *);
>  
> @@ -1222,16 +1222,20 @@ symbol_file_add (const char *name, symfile_add_flags add_flags,
>  void
>  symbol_file_add_main (const char *args, symfile_add_flags add_flags)
>  {
> -  symbol_file_add_main_1 (args, add_flags, 0);
> +  symbol_file_add_main_1 (args, add_flags, 0, 0);
>  }
>  
>  static void
>  symbol_file_add_main_1 (const char *args, symfile_add_flags add_flags,
> -			objfile_flags flags)
> +			objfile_flags flags, CORE_ADDR offset)
>  {
> +  struct objfile *objfile;
> +
>    add_flags |= current_inferior ()->symfile_flags | SYMFILE_MAINLINE;
>  
> -  symbol_file_add (args, add_flags, NULL, flags);
> +  objfile = symbol_file_add (args, add_flags, NULL, flags);
> +  if (offset != 0)
> +    objfile_rebase (objfile, offset);
>  
>    /* Getting new symbols may change our opinion about
>       what is frameless.  */
> @@ -1548,6 +1552,8 @@ symbol_file_command (const char *args, int from_tty)
>        symfile_add_flags add_flags = 0;
>        char *name = NULL;
>        bool stop_processing_options = false;
> +      bool expecting_offset = false;
> +      CORE_ADDR offset = 0;
>        int idx;
>        char *arg;
>  
> @@ -1559,7 +1565,12 @@ symbol_file_command (const char *args, int from_tty)
>  	{
>  	  if (stop_processing_options || *arg != '-')
>  	    {
> -	      if (name == NULL)
> +	      if (expecting_offset)
> +		{
> +		  offset = parse_and_eval_address (arg);
> +		  expecting_offset = false;
> +		}
> +	      else if (name == NULL)
>  		name = arg;
>  	      else
>  		error (_("Unrecognized argument \"%s\""), arg);
> @@ -1568,6 +1579,8 @@ symbol_file_command (const char *args, int from_tty)
>  	    flags |= OBJF_READNOW;
>  	  else if (strcmp (arg, "-readnever") == 0)
>  	    flags |= OBJF_READNEVER;
> +	  else if (strcmp (arg, "-o") == 0)
> +	    expecting_offset = true;
>  	  else if (strcmp (arg, "--") == 0)
>  	    stop_processing_options = true;
>  	  else
> @@ -1579,7 +1592,7 @@ symbol_file_command (const char *args, int from_tty)
>  
>        validate_readnow_readnever (flags);
>  
> -      symbol_file_add_main_1 (name, add_flags, flags);
> +      symbol_file_add_main_1 (name, add_flags, flags, offset);
>      }
>  }
>  
> @@ -3772,7 +3785,8 @@ symbolic debug information."
>  
>    c = add_cmd ("symbol-file", class_files, symbol_file_command, _("\
>  Load symbol table from executable file FILE.\n\
> -Usage: symbol-file [-readnow | -readnever] FILE\n\
> +Usage: symbol-file [-readnow | -readnever] [-o <OFF>] FILE\n\
> +OFF is an optional offset which is added to each section address.\n\
>  The `file' command can also load symbol tables, as well as setting the file\n\
>  to execute.\n" READNOW_READNEVER_HELP), &cmdlist);
>    set_cmd_completer (c, filename_completer);
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index 34da102c62..68431cb035 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,7 @@
> +2018-04-27  Petr Tesarik  <ptesarik@suse.com>
> +
> +	* gdb.base/relocate.exp: Add test for "symbol-file -o ".
> +
>  2018-04-26  Pedro Alves  <palves@redhat.com>
>  
>  	* gdb.base/gnu-ifunc.exp (set-break): Test that GDB resolves
> diff --git a/gdb/testsuite/gdb.base/relocate.exp b/gdb/testsuite/gdb.base/relocate.exp
> index 89f2fffcd9..4383e79cb2 100644
> --- a/gdb/testsuite/gdb.base/relocate.exp
> +++ b/gdb/testsuite/gdb.base/relocate.exp
> @@ -196,6 +196,39 @@ if { "${function_foo_addr}" == "${new_function_foo_addr}" } {
>    pass "function foo has a different address"
>  }
>  
> +# Load the object using symbol-file with an offset and check that
> +# all addresses are moved by that offset.
> +
> +set offset 0x10000
> +clean_restart
> +gdb_test "symbol-file -o $offset $binfile" \
> +    "Reading symbols from ${binfile}\.\.\.done\." \
> +    "symbol-file with offset"
> +
> +# Make sure the address of a static variable is moved by offset.
> +set new_static_foo_addr [get_var_address static_foo]
> +if { "${new_static_foo_addr}" == "${static_foo_addr}" + $offset } {
> +  pass "static variable foo is moved by offset"
> +} else {
> +  fail "static variable foo is moved by offset"
> +}
> +
> +# Make sure the address of a global variable is moved by offset.
> +set new_global_foo_addr [get_var_address global_foo]
> +if { "${new_global_foo_addr}" == "${global_foo_addr}" + $offset } {
> +  pass "global variable foo is moved by offset"
> +} else {
> +  fail "global variable foo is moved by offset"
> +}
> +
> +# Make sure the address of a functaion is moved by offset.
> +set new_function_foo_addr [get_var_address function_foo]
> +if { "${new_function_foo_addr}" == "${function_foo_addr}" + $offset } {
> +  pass "function foo is moved by offset"
> +} else {
> +  fail "function foo is moved by offset"
> +}
> +
>  # Now try loading the object as an exec-file; we should be able to print
>  # the values of variables after we do this.
>  

Re: [PATCH] Add an optional offset option to the "symbol-file" command

[Simon Marchi]

On 2018-05-23 04:49, Petr Tesarik wrote:
> Hi all,
> 
> any comment on my patch? If it's not good, can you elaborate on what
> needs improvement, please?
> 
> Petr T

Hi Petr,

Sorry about that, with the volume on the list patches fall through the 
cracks some times, it is perfectly appropriate to ping them after a 
while as you did.

I am not against adding that new feature to "symbol-file" (it seems 
useful), but I am just wondering first if you can achieve the same thing 
using "add-symbol-file" instead.  add-symbol-file doesn't take an 
offset, but the beginning address of the .text section.  Other sections 
(e.g. .data and .bss) need to be specified separately though.  I'd then 
like to know if it would be possible to make symbol-file similar to 
add-symbol-file, and if that would be practical/easy to use.  On one 
side, it would be weird if symbol-file and add-symbol-file had different 
syntaxes to achieve the same thing, but on the other hand having to 
specify a single offset for all sections of the object file (probably 
enough 99.9% of the time) sounds much easier than having to specify the 
base addresses of multiple sections...

I don't have big comments on the patch itself, just nits here and there.

>> @@ -1222,16 +1222,20 @@ symbol_file_add (const char *name, 
>> symfile_add_flags add_flags,
>>  void
>>  symbol_file_add_main (const char *args, symfile_add_flags add_flags)
>>  {
>> -  symbol_file_add_main_1 (args, add_flags, 0);
>> +  symbol_file_add_main_1 (args, add_flags, 0, 0);
>>  }
>> 
>>  static void
>>  symbol_file_add_main_1 (const char *args, symfile_add_flags 
>> add_flags,
>> -			objfile_flags flags)
>> +			objfile_flags flags, CORE_ADDR offset)
>>  {
>> +  struct objfile *objfile;
>> +
>>    add_flags |= current_inferior ()->symfile_flags | SYMFILE_MAINLINE;
>> 
>> -  symbol_file_add (args, add_flags, NULL, flags);
>> +  objfile = symbol_file_add (args, add_flags, NULL, flags);

You can declare and assign the variable on the same line.


>> @@ -1568,6 +1579,8 @@ symbol_file_command (const char *args, int 
>> from_tty)
>>  	    flags |= OBJF_READNOW;
>>  	  else if (strcmp (arg, "-readnever") == 0)
>>  	    flags |= OBJF_READNEVER;
>> +	  else if (strcmp (arg, "-o") == 0)
>> +	    expecting_offset = true;

This doesn't handle correctly (IMO) "symbol-file foo -o", which should 
be rejected with an error message.  I think it would be simpler to do 
something like this:

	  else if (strcmp (arg, "-o") == 0)
	    {
	      arg = built_argv[++idx];
	      if (arg == NULL)
		error (_("Missing argument to -o"));

	      offset = parse_and_eval_address (arg);
	    }



>> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
>> index 34da102c62..68431cb035 100644
>> --- a/gdb/testsuite/ChangeLog
>> +++ b/gdb/testsuite/ChangeLog
>> @@ -1,3 +1,7 @@
>> +2018-04-27  Petr Tesarik  <ptesarik@suse.com>
>> +
>> +	* gdb.base/relocate.exp: Add test for "symbol-file -o ".
>> +
>>  2018-04-26  Pedro Alves  <palves@redhat.com>
>> 
>>  	* gdb.base/gnu-ifunc.exp (set-break): Test that GDB resolves
>> diff --git a/gdb/testsuite/gdb.base/relocate.exp 
>> b/gdb/testsuite/gdb.base/relocate.exp
>> index 89f2fffcd9..4383e79cb2 100644
>> --- a/gdb/testsuite/gdb.base/relocate.exp
>> +++ b/gdb/testsuite/gdb.base/relocate.exp
>> @@ -196,6 +196,39 @@ if { "${function_foo_addr}" == 
>> "${new_function_foo_addr}" } {
>>    pass "function foo has a different address"
>>  }
>> 
>> +# Load the object using symbol-file with an offset and check that
>> +# all addresses are moved by that offset.
>> +
>> +set offset 0x10000
>> +clean_restart
>> +gdb_test "symbol-file -o $offset $binfile" \
>> +    "Reading symbols from ${binfile}\.\.\.done\." \
>> +    "symbol-file with offset"
>> +
>> +# Make sure the address of a static variable is moved by offset.
>> +set new_static_foo_addr [get_var_address static_foo]
>> +if { "${new_static_foo_addr}" == "${static_foo_addr}" + $offset } {
>> +  pass "static variable foo is moved by offset"
>> +} else {
>> +  fail "static variable foo is moved by offset"
>> +}

You can simplify these using gdb_assert:

gdb_assert "${new_static_foo_addr} == ${static_foo_addr} + $offset" \
     "static variable foo is moved by offset"

Thanks,

Simon

Re: [PATCH] Add an optional offset option to the "symbol-file" command

[Petr Tesarik]

Hi Simon,

On Thu, 24 May 2018 09:33:30 -0400
Simon Marchi <simon.marchi@polymtl.ca> wrote:

>[...]
> Sorry about that, with the volume on the list patches fall through the 
> cracks some times, it is perfectly appropriate to ping them after a 
> while as you did.

No problem.

> I am not against adding that new feature to "symbol-file" (it seems 
> useful), but I am just wondering first if you can achieve the same thing 
> using "add-symbol-file" instead.  add-symbol-file doesn't take an 
> offset, but the beginning address of the .text section.  Other sections 
> (e.g. .data and .bss) need to be specified separately though.  I'd then 
> like to know if it would be possible to make symbol-file similar to 
> add-symbol-file, and if that would be practical/easy to use.  On one 
> side, it would be weird if symbol-file and add-symbol-file had different 
> syntaxes to achieve the same thing, but on the other hand having to 
> specify a single offset for all sections of the object file (probably 
> enough 99.9% of the time) sounds much easier than having to specify the 
> base addresses of multiple sections...

You bet! In fact, I was kind of expecting this question.

Yes, technically, add-symbol-file can be used for the same purpose, but
it is very inconvenient. Most notably, it requires listing all ELF
sections explicitly, and the Linux kernel has a lot of them (typically
a few dozen).

So, my current solution is:

 1. exec-file vmlinux
 2. info target
 3. # parse the output, adding an offset to each section's start
 4. add-symbol-file vmlinux $textaddr -s ... # a very long list
 5. exec-file  # to make sure that only target memory is used

Although I have already written a Python script to initialise the
session, it's ugly, especially when it comes to parsing the output of
"info target".

Regarding consistency, add-symbol-file does not currently have any
conflicting "-o" option, so I can add one for the same purpose.

Shall I do that?

> I don't have big comments on the patch itself, just nits here and
> there.
>[...]
> >> -			objfile_flags flags)
> >> +			objfile_flags flags, CORE_ADDR offset)
> >>  {
> >> +  struct objfile *objfile;
> >> +
> >>    add_flags |= current_inferior ()->symfile_flags |
> >> SYMFILE_MAINLINE;
> >> 
> >> -  symbol_file_add (args, add_flags, NULL, flags);
> >> +  objfile = symbol_file_add (args, add_flags, NULL, flags);  
> 
> You can declare and assign the variable on the same line.

Indeed. I tend to forget that gdb has switched to C++.

> >> @@ -1568,6 +1579,8 @@ symbol_file_command (const char *args, int 
> >> from_tty)
> >>  	    flags |= OBJF_READNOW;
> >>  	  else if (strcmp (arg, "-readnever") == 0)
> >>  	    flags |= OBJF_READNEVER;
> >> +	  else if (strcmp (arg, "-o") == 0)
> >> +	    expecting_offset = true;  
> 
> This doesn't handle correctly (IMO) "symbol-file foo -o", which
> should be rejected with an error message.  I think it would be
> simpler to do something like this:
> 
> 	  else if (strcmp (arg, "-o") == 0)
> 	    {
> 	      arg = built_argv[++idx];
> 	      if (arg == NULL)
> 		error (_("Missing argument to -o"));
> 
> 	      offset = parse_and_eval_address (arg);
> 	    }

Ah, so that's how it's done. Honestly, I was quite surprised to find no
variant of getopt() here...

>[...]
> >> +# Make sure the address of a static variable is moved by offset.
> >> +set new_static_foo_addr [get_var_address static_foo]
> >> +if { "${new_static_foo_addr}" == "${static_foo_addr}" + $offset }
> >> {
> >> +  pass "static variable foo is moved by offset"
> >> +} else {
> >> +  fail "static variable foo is moved by offset"
> >> +}  
> 
> You can simplify these using gdb_assert:
> 
> gdb_assert "${new_static_foo_addr} == ${static_foo_addr} + $offset" \
>      "static variable foo is moved by offset"

Will do. I should probably simplify other similar stanzas in the test
suite in a separate patch.

Thank you for your review! I will send an improved patch soon (but
maybe not today, as I'm at a conference).

Petr T

Re: [PATCH] Add an optional offset option to the "symbol-file" command

[Simon Marchi]

On 2018-05-25 01:23, Petr Tesarik wrote:
> Yes, technically, add-symbol-file can be used for the same purpose, but
> it is very inconvenient. Most notably, it requires listing all ELF
> sections explicitly, and the Linux kernel has a lot of them (typically
> a few dozen).
> 
> So, my current solution is:
> 
>  1. exec-file vmlinux
>  2. info target
>  3. # parse the output, adding an offset to each section's start
>  4. add-symbol-file vmlinux $textaddr -s ... # a very long list
>  5. exec-file  # to make sure that only target memory is used
> 
> Although I have already written a Python script to initialise the
> session, it's ugly, especially when it comes to parsing the output of
> "info target".

Thanks for confirming.

> Regarding consistency, add-symbol-file does not currently have any
> conflicting "-o" option, so I can add one for the same purpose.
> 
> Shall I do that?

I think it would be a good idea.  It would improve the usability of 
add-symbol-file and keep both commands more or less in sync, so it 
sounds like a win-win to me.

I assume the second positional argument of add-symbol-file (which gives 
the start address of .text) would become optional?  You just need to 
think carefully about what should happen when mixing the different 
arguments.  The .text positional argument and -o would probably be 
mutually exclusive?  -o applies to all sections, but you could use -s to 
override the address of a particular section?

> Indeed. I tend to forget that gdb has switched to C++.

Yep, you can get wild!

>> >> @@ -1568,6 +1579,8 @@ symbol_file_command (const char *args, int
>> >> from_tty)
>> >>  	    flags |= OBJF_READNOW;
>> >>  	  else if (strcmp (arg, "-readnever") == 0)
>> >>  	    flags |= OBJF_READNEVER;
>> >> +	  else if (strcmp (arg, "-o") == 0)
>> >> +	    expecting_offset = true;
>> 
>> This doesn't handle correctly (IMO) "symbol-file foo -o", which
>> should be rejected with an error message.  I think it would be
>> simpler to do something like this:
>> 
>> 	  else if (strcmp (arg, "-o") == 0)
>> 	    {
>> 	      arg = built_argv[++idx];
>> 	      if (arg == NULL)
>> 		error (_("Missing argument to -o"));
>> 
>> 	      offset = parse_and_eval_address (arg);
>> 	    }
> 
> Ah, so that's how it's done. Honestly, I was quite surprised to find no
> variant of getopt() here...

I think GDB would benefit of having such a variant of getopt to 
standardize how arguments are parsed throughout the CLI.  And to avoid 
having to do manual parsing like this, which often gives a fragile 
result.

>> You can simplify these using gdb_assert:
>> 
>> gdb_assert "${new_static_foo_addr} == ${static_foo_addr} + $offset" \
>>      "static variable foo is moved by offset"
> 
> Will do. I should probably simplify other similar stanzas in the test
> suite in a separate patch.

If you are up for it, it would be appreciated!

> Thank you for your review! I will send an improved patch soon (but
> maybe not today, as I'm at a conference).

But but... it's the ideal time to work on side-quest patches! ;)

Thanks,

Simon

Re: [PATCH] Add an optional offset option to the "symbol-file" command

[John Baldwin]

On Friday, May 25, 2018 10:14:35 AM Simon Marchi wrote:
> On 2018-05-25 01:23, Petr Tesarik wrote:
> > Yes, technically, add-symbol-file can be used for the same purpose, but
> > it is very inconvenient. Most notably, it requires listing all ELF
> > sections explicitly, and the Linux kernel has a lot of them (typically
> > a few dozen).
> > 
> > So, my current solution is:
> > 
> >  1. exec-file vmlinux
> >  2. info target
> >  3. # parse the output, adding an offset to each section's start
> >  4. add-symbol-file vmlinux $textaddr -s ... # a very long list
> >  5. exec-file  # to make sure that only target memory is used
> > 
> > Although I have already written a Python script to initialise the
> > session, it's ugly, especially when it comes to parsing the output of
> > "info target".
> 
> Thanks for confirming.
> 
> > Regarding consistency, add-symbol-file does not currently have any
> > conflicting "-o" option, so I can add one for the same purpose.
> > 
> > Shall I do that?
> 
> I think it would be a good idea.  It would improve the usability of 
> add-symbol-file and keep both commands more or less in sync, so it 
> sounds like a win-win to me.
> 
> I assume the second positional argument of add-symbol-file (which gives 
> the start address of .text) would become optional?  You just need to 
> think carefully about what should happen when mixing the different 
> arguments.  The .text positional argument and -o would probably be 
> mutually exclusive?  -o applies to all sections, but you could use -s to 
> override the address of a particular section?

Please do this.  I have an old branch with an 'add_symbol_file_rel' that
tried to do this (but the syntax was a bit clumsy).  My use case was debugging
an EFI appliation via the GDB stub in qemu.  I've included the patch below
in case it is useful.

commit d4ae4cb5271baacc2754e0ee89415c66a5296133
Author: John Baldwin <jhb@FreeBSD.org>
Date:   Fri Mar 6 15:09:01 2015 -0500

    Add a variant of 'add-symbol-file' that accepts a single base address
    of the image and relocates all sections in the file by adding the
    base address.

diff --git a/gdb/symfile.c b/gdb/symfile.c
index c2a71eca59..6f86fd4345 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -89,6 +89,8 @@ static void symbol_file_add_main_1 (const char *args, int from_tty, int flags);
 
 static void add_symbol_file_command (char *, int);
 
+static void add_symbol_file_rel_command (char *, int);
+
 static const struct sym_fns *find_sym_fns (bfd *);
 
 static void decrement_reading_symtab (void *);
@@ -2406,6 +2408,74 @@ add_symbol_file_command (char *args, int from_tty)
 }
 \f
 
+static void
+add_symbol_file_rel_command (char *args, int from_tty)
+{
+  struct gdbarch *gdbarch = get_current_arch ();
+  int flags = OBJF_USERLOADED | OBJF_SHARED;
+  CORE_ADDR base_addr;
+  char *arg;
+  int argcnt = 0;
+  int i;
+  char **argv;
+  struct objfile *objf;
+  bfd *bfd;
+
+  struct section_addr_info *section_addrs;
+  struct cleanup *my_cleanups = make_cleanup (null_cleanup, NULL);
+
+  dont_repeat ();
+
+  if (args == NULL)
+    error (_("add-symbol-file-rel takes a file name and an address"));
+
+  argv = gdb_buildargv (args);
+  make_cleanup_freeargv (argv);
+
+  if (argv[0] == NULL || argv[1] == NULL)
+    error (_("add-symbol-file-rel takes a file name and an address"));
+
+  /* The first argument is the file name.  */
+  bfd = symfile_bfd_open (argv[0]);
+  make_cleanup_bfd_unref (bfd);
+
+  /* The second argument is the base address. */
+  base_addr = parse_and_eval_address (argv[1]);
+
+  /* Handle optional arguments. */
+  for (arg = argv[2], argcnt = 2; arg != NULL; arg = argv[++argcnt])
+    {
+      if (strcmp (arg, "-readnow") == 0)
+	flags |= OBJF_READNOW;
+      else
+	error (_("USAGE: add-symbol-file-rel <filename> <baseaddress>"
+		 " [-readnow]"));
+    }
+
+  section_addrs = build_section_addr_info_from_bfd (bfd);
+  make_cleanup_free_section_addr_info (section_addrs);
+  for (i = 0; i < section_addrs->num_sections; i++)
+    {
+      section_addrs->other[i].addr += base_addr;
+      printf_unfiltered ("\t%s_addr = %s\n", section_addrs->other[i].name,
+			 paddress (gdbarch, section_addrs->other[i].addr));
+    }
+  
+  if (from_tty && (!query ("%s", "")))
+    error (_("Not confirmed."));
+
+  objf = symbol_file_add_from_bfd (bfd, argv[0], from_tty ? SYMFILE_VERBOSE : 0,
+				   section_addrs, flags, NULL);
+
+  add_target_sections_of_objfile (objf);
+
+  /* Getting new symbols may change our opinion about what is
+     frameless.  */
+  reinit_frame_cache ();
+  do_cleanups (my_cleanups);
+}
+\f
+
 /* This function removes a symbol file that was added via add-symbol-file.  */
 
 static void
@@ -3998,6 +4068,15 @@ with the text.  SECT is a section name to be loaded at SECT_ADDR."),
 	       &cmdlist);
   set_cmd_completer (c, filename_completer);
 
+  c = add_cmd ("add-symbol-file-rel", class_files, add_symbol_file_rel_command,
+	       _("\
+Load symbols from FILE, assuming FILE has been dynamically loaded.\n\
+Usage: add-symbol-file-rel FILE ADDR\n\
+ADDR is the base address of the file.  The base address is added to the\n\
+address of each loaded section."),
+	       &cmdlist);
+  set_cmd_completer (c, filename_completer);
+  
   c = add_cmd ("remove-symbol-file", class_files,
 	       remove_symbol_file_command, _("\
 Remove a symbol file added via the add-symbol-file command.\n\


-- 
John Baldwin