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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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