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

  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(-)
  

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.
>
  

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
  

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
  

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