Patchwork [v2,2/4] Make add-symbol-file's address argument optional

login
register
mail settings
Submitter Petr Tesarik
Date June 11, 2018, 12:08 p.m.
Message ID <20180611120835.27343-3-ptesarik@suse.cz>
Download mbox | patch
Permalink /patch/27741/
State New
Headers show

Comments

Petr Tesarik - June 11, 2018, 12:08 p.m.
The (first) .text section must be always specified as the second
non-option argument.  The documentation states that GDB cannot
figure out this address by itself.  This is true if the object file
was indeed relocated, but it is also confusing, because all other
sections can be omitted and will use the address provided by BFD.

gdb/ChangeLog:
2018-06-11  Petr Tesarik  <ptesarik@suse.com>

	* symfile.c (add_symbol_file_command, _initialize_symfile): Do not
	require the second argument.  If omitted, load sections at the
	addresses specified in the file.

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

	* gdb.texinfo (Files): The address argument for "add-symbol-file"
	is no longer mandatory.

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

	* gdb.base/relocate.exp: Test add-symbol-file behavior when the
	address argument is omitted.
---
 gdb/ChangeLog                       |  6 ++++++
 gdb/NEWS                            |  3 +++
 gdb/doc/ChangeLog                   |  5 +++++
 gdb/doc/gdb.texinfo                 | 13 ++++++++-----
 gdb/symfile.c                       | 25 ++++++++++++-------------
 gdb/testsuite/ChangeLog             |  5 +++++
 gdb/testsuite/gdb.base/relocate.exp | 15 +++++++++++++++
 7 files changed, 54 insertions(+), 18 deletions(-)
Eli Zaretskii - June 11, 2018, 3:25 p.m.
> From: Petr Tesarik <ptesarik@suse.cz>
> Cc: Petr Tesarik <ptesarik@suse.cz>,	Jeff Mahoney <jeffm@suse.com>
> Date: Mon, 11 Jun 2018 14:08:33 +0200
> 
> The (first) .text section must be always specified as the second
> non-option argument.  The documentation states that GDB cannot
> figure out this address by itself.  This is true if the object file
> was indeed relocated, but it is also confusing, because all other
> sections can be omitted and will use the address provided by BFD.
> 
> gdb/ChangeLog:
> 2018-06-11  Petr Tesarik  <ptesarik@suse.com>
> 
> 	* symfile.c (add_symbol_file_command, _initialize_symfile): Do not
> 	require the second argument.  If omitted, load sections at the
> 	addresses specified in the file.
> 
> gdb/doc/ChangeLog:
> 2018-06-11  Petr Tesarik  <ptesarik@suse.com>
> 
> 	* gdb.texinfo (Files): The address argument for "add-symbol-file"
> 	is no longer mandatory.
> 
> gdb/testsuite/ChangeLog:
> 2018-06-11  Petr Tesarik  <ptesarik@suse.com>
> 
> 	* gdb.base/relocate.exp: Test add-symbol-file behavior when the
> 	address argument is omitted.

I have a feeling I already reviewed and approved this, but in case I
didn't, I approve it now.

Thanks.
Petr Tesarik - June 11, 2018, 4:50 p.m.
On Mon, 11 Jun 2018 18:25:02 +0300
Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Petr Tesarik <ptesarik@suse.cz>
> > Cc: Petr Tesarik <ptesarik@suse.cz>,	Jeff Mahoney <jeffm@suse.com>
> > Date: Mon, 11 Jun 2018 14:08:33 +0200
> > 
> > The (first) .text section must be always specified as the second
> > non-option argument.  The documentation states that GDB cannot
> > figure out this address by itself.  This is true if the object file
> > was indeed relocated, but it is also confusing, because all other
> > sections can be omitted and will use the address provided by BFD.
> > 
> > gdb/ChangeLog:
> > 2018-06-11  Petr Tesarik  <ptesarik@suse.com>
> > 
> > 	* symfile.c (add_symbol_file_command, _initialize_symfile): Do not
> > 	require the second argument.  If omitted, load sections at the
> > 	addresses specified in the file.
> > 
> > gdb/doc/ChangeLog:
> > 2018-06-11  Petr Tesarik  <ptesarik@suse.com>
> > 
> > 	* gdb.texinfo (Files): The address argument for "add-symbol-file"
> > 	is no longer mandatory.
> > 
> > gdb/testsuite/ChangeLog:
> > 2018-06-11  Petr Tesarik  <ptesarik@suse.com>
> > 
> > 	* gdb.base/relocate.exp: Test add-symbol-file behavior when the
> > 	address argument is omitted.  
> 
> I have a feeling I already reviewed and approved this, but in case I
> didn't, I approve it now.

You did, but I resent the whole patch series again, improving the
changelog style and adding the help texts where necessary. I found it
better than fixing it up with a separate patch.

I'm sorry if that was confusing.

Petr T
Eli Zaretskii - June 11, 2018, 5:25 p.m.
> Date: Mon, 11 Jun 2018 18:50:48 +0200
> From: Petr Tesarik <ptesarik@suse.cz>
> Cc: gdb-patches@sourceware.org, jeffm@suse.com
> 
> > I have a feeling I already reviewed and approved this, but in case I
> > didn't, I approve it now.
> 
> You did, but I resent the whole patch series again, improving the
> changelog style and adding the help texts where necessary. I found it
> better than fixing it up with a separate patch.

Well, in the future just say in the text that precedes the patch that
the documentation parts were already approved, that should be enough.

Thanks.
Simon Marchi - June 26, 2018, 2:14 a.m.
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

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1b789d00f4..1c5a1f6bfb 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,11 @@ 
 2018-06-11  Petr Tesarik  <ptesarik@suse.com>
 
+	* symfile.c (add_symbol_file_command, _initialize_symfile): Do not
+	require the second argument.  If omitted, load sections at the
+	addresses specified in the file.
+
+2018-06-11  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.
diff --git a/gdb/NEWS b/gdb/NEWS
index 101746567a..54c0f1d19b 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -6,6 +6,9 @@ 
 * The 'symbol-file' command now accepts an '-o' option to add a relative
   offset to all sections.
 
+* The 'add-symbol-file' command no longer requires the second argument
+  (address of the text section).
+
 * The endianness used with the 'set endian auto' mode in the absence of
   an executable selected for debugging is now the last endianness chosen
   either by one of the 'set endian big' and 'set endian little' commands
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index b31c1ac324..b8f48733f9 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,5 +1,10 @@ 
 2018-06-11  Petr Tesarik  <ptesarik@suse.com>
 
+	* gdb.texinfo (Files): The address argument for "add-symbol-file"
+	is no longer mandatory.
+
+2018-06-11  Petr Tesarik  <ptesarik@suse.com>
+
 	* gdb.texinfo (Files): Document "symbol-file -o offset".
 
 2018-06-08  Gary Benson <gbenson@redhat.com>
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.
 
 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)
 
   validate_readnow_readnever (flags);
 
-  /* This command takes at least two arguments.  The first one is a
-     filename, and the second is the address where this file has been
-     loaded.  Abort now if this address hasn't been provided by the
-     user.  */
-  if (!seen_addr)
-    error (_("The address where %s has been loaded is missing"),
-	   filename.get ());
-
   /* Print the prompt for the query below.  And save the arguments into
      a sect_addr_info structure to be passed around to other
      functions.  We have to split this up into separate print
      statements because hex_string returns a local static
      string.  */
 
-  printf_unfiltered (_("add symbol table from file \"%s\" at\n"),
+  printf_unfiltered (_("add symbol table from file \"%s\""),
 		     filename.get ());
   section_addr_info section_addrs;
-  for (sect_opt &sect : sect_opts)
+  std::vector<sect_opt>::const_iterator it = sect_opts.begin ();
+  if (!seen_addr)
+    ++it;
+  for (; it != sect_opts.end (); ++it)
     {
       CORE_ADDR addr;
-      const char *val = sect.value;
-      const char *sec = sect.name;
+      const char *val = it->value;
+      const char *sec = it->name;
 
+      if (section_addrs.size () == 0)
+	printf_unfiltered (_(" at\n"));
       addr = parse_and_eval_address (val);
 
       /* Here we store the section offsets in the order they were
@@ -2198,6 +2195,8 @@  add_symbol_file_command (const char *args, int from_tty)
 	 At this point, we don't know what file type this is,
 	 so we can't determine what section names are valid.  */
     }
+  if (section_addrs.size () == 0)
+    printf_unfiltered ("\n");
 
   if (from_tty && (!query ("%s", "")))
     error (_("Not confirmed."));
@@ -3793,7 +3792,7 @@  to execute.\n" READNOW_READNEVER_HELP), &cmdlist);
 
   c = add_cmd ("add-symbol-file", class_files, add_symbol_file_command, _("\
 Load symbols from FILE, assuming FILE has been dynamically loaded.\n\
-Usage: add-symbol-file FILE ADDR [-readnow | -readnever | \
+Usage: add-symbol-file FILE [ADDR] [-readnow | -readnever | \
 -s SECT-NAME SECT-ADDR]...\n\
 ADDR is the starting address of the file's text.\n\
 Each '-s' argument provides a section name and address, and\n\
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b29a2bfab3..f2e7aa98a8 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,10 @@ 
 2018-06-11  Petr Tesarik  <ptesarik@suse.com>
 
+	* gdb.base/relocate.exp: Test add-symbol-file behavior when the
+	address argument is omitted.
+
+2018-06-11  Petr Tesarik  <ptesarik@suse.com>
+
 	* gdb.base/relocate.exp: Add test for "symbol-file -o ".
 
 2018-06-08  Gary Benson <gbenson@redhat.com>
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
+# the text address in a positional argument.
+set test "add-symbol-file -s .text, no address"
+gdb_test_multiple "add-symbol-file $binfile -s .text 0x100" $test {
+    -re "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = 0x100\r\n\\(y or n\\) " {
+	gdb_test "n" "Not confirmed\." $test
+    }
+}
+# Check section addresses can be omitted.
+set test "add-symbol-file no address"
+gdb_test_multiple "add-symbol-file $binfile" $test {
+    -re "add symbol table from file \"${binfile}\"\r\n\\(y or n\\) " {
+	gdb_test "n" "Not confirmed\." $test
+    }
+}
 # Test that passing "--" disables option processing.
 gdb_test "add-symbol-file -- $binfile 0x100 -s .bss 0x3" \
     "Unrecognized argument \"-s\"" \