Re: [PATCH v2 2/4] Make add-symbol-file's address argument optional

Simon Marchi
2018-06-26T02:14:38Z

Hi Petr,

The patch LGTM, with some nits to address before pushing.

On 2018-06-11 08:08, Petr Tesarik wrote:
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 973365574f..84600bfe5f 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -18917,18 +18917,21 @@ the program is running. To do this, use the
> @code{kill} command
>
> @kindex add-symbol-file
> @cindex dynamic linking
> -@item add-symbol-file @var{filename} @var{address}
> -@itemx add-symbol-file @var{filename} @var{address} @r{[} -readnow
> @r{|} -readnever @r{]}
> -@itemx add-symbol-file @var{filename} @var{address} -s @var{section}
> @var{address} @dots{}
> +@item add-symbol-file @var{filename} @r{[} @var{address} @r{]}
> +@itemx add-symbol-file @var{filename} @r{[} @var{address} @r{]} @r{[}
> -readnow @r{|} -readnever @r{]}
> +@itemx add-symbol-file @var{filename} @r{[} @var{address} @r{]} -s
> @var{section} @var{address} @dots{}
> The @code{add-symbol-file} command reads additional symbol table
> information from the file @var{filename}. You would use this command
> when @var{filename} has been dynamically loaded (by some other means)
> into the program that is running. The @var{address} should give the
> memory
> -address at which the file has been loaded; @value{GDBN} cannot figure
> -this out for itself. You can additionally specify an arbitrary number
> +address at which the file has been loaded.
> +You can additionally specify an arbitrary number
> of @samp{-s @var{section} @var{address}} pairs, to give an explicit
> section name and base address for that section. You can specify any
> @var{address} as an expression.
> +If @var{address} is omitted, @value{GDBN} will use the section
> +addresses found in @var{filename}. You can use @samp{-s} to
> +override this default and load a section at a different address.

I really think that this section could use some improvements:

- There are two arguments named "address", so it's not clear what the
text refers to.
- I don't think it's useful to have the synopsis on three different
lines, since the options are not mutually exclusive.
- It should be made clear that the positional "address" argument
specifies the start of the .text section. Since it is now optional, I
also think that this positional argument should be deprecated in favor
of using "-s .text ..."...

But none of this is a direct consequence of your patch, so your patch
looks ok to me.

>
> The symbol table of the file @var{filename} is added to the symbol
> table
> originally read with the @code{symbol-file} command. You can use the
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 461f60d074..3e3ab20412 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -2161,29 +2161,26 @@ add_symbol_file_command (const char *args, int
> from_tty)

There might be the error message:

error (_("add-symbol-file takes a file name and an address"));

that would need to be updated, now that only the file name is mandatory.

> diff --git a/gdb/testsuite/gdb.base/relocate.exp
> b/gdb/testsuite/gdb.base/relocate.exp
> index 77f6a88159..a3af8cea61 100644
> --- a/gdb/testsuite/gdb.base/relocate.exp
> +++ b/gdb/testsuite/gdb.base/relocate.exp
> @@ -73,6 +73,21 @@ gdb_test_multiple "add-symbol-file -s .text 0x200
> $binfile 0x100" $test {
> gdb_test "n" "Not confirmed\." $test
> }
> }
> +# Check that passing a single "-s .text" is equivallent to passing

equivallent -> equivalent.

Thanks,

Simon