Patchwork [v2] Make '{add-,}symbol-file' not care about the position of command line arguments

login
register
mail settings
Submitter Sergio Durigan Junior
Date Nov. 30, 2017, 4:24 a.m.
Message ID <20171130042448.30882-1-sergiodj@redhat.com>
Download mbox | patch
Permalink /patch/24622/
State New
Headers show

Comments

Sergio Durigan Junior - Nov. 30, 2017, 4:24 a.m.
Changes from v1:

- Commit message has been rewritten.

- Implemented position-independent argument parsing for
  'add-symbol-file'.

- Added testcases.


This is a bug that's been detected while doing the readnever work.

If you use 'symbol-file' or 'add-symbol-file', the position of each
argument passed to the command matters.  This means that if you do:

  (gdb) symbol-file -readnow /foo/bar

The symbol file specified will (correctly) have all of its symbols
read by GDB (because of the -readnow flag).  However, if you do:

  (gdb) symbol-file /foo/bar -readnow

GDB will silently ignore the -readnow flag, because it was specified
after the filename.  This is not a good thing to do and may confuse
the user.

To address that, I've modified the argument parsing mechanisms of
symbol_file_command and add_symbol_file_command to be
"position-independent".  I have also added one error call at the end
of add_symbol_file_command's argument parsing logic, which now clearly
complains if no filename has been specified.

This patch provides a testcase for both commands, in order to make
sure that the argument order does not matter.  It has been
regression-tested on BuildBot.

gdb/ChangeLog:

2017-11-29  Sergio Durigan Junior  <sergiodj@redhat.com>

	* symfile.c (symbol_file_command): Call
	'symbol_file_add_main_1' only after processing all command
	line options.
	(add_symbol_file_command): Modify logic to make arguments
	position-independent.

gdb/testsuite/ChangeLog:

2017-11-29  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.base/relocate.exp: Add tests to guarantee that arguments
	to 'symbol-file' and 'add-symbol-file' can be
	position-independent.
---
 gdb/symfile.c                       | 61 +++++++++++++++++++------------------
 gdb/testsuite/gdb.base/relocate.exp | 38 +++++++++++++++++++++++
 2 files changed, 70 insertions(+), 29 deletions(-)
Pedro Alves - Nov. 30, 2017, 10:57 a.m.
On 11/30/2017 04:24 AM, Sergio Durigan Junior wrote:
> Changes from v1:
> 
> - Commit message has been rewritten.
> 
> - Implemented position-independent argument parsing for
>   'add-symbol-file'.
> 
> - Added testcases.

Looks like you missed the comment about "--".  Take a look at
maintenance_print_symbols for an example of a command
that supports ending options with "--".  Can you add that
while you're at it, please?  For a test, I'd suggest
e.g., "symbol-file -- -non-existent-file" and confirming
gdb errors out.  That's simpler than actually creating a file.


> +      if (*arg != '-')
>  	{
> -	  /* It's an option (starting with '-') or it's an argument
> -	     to an option.  */
>  	  if (expecting_sec_name)
>  	    {
>  	      sect_opt sect = { arg, NULL };
>  	      sect_opts.push_back (sect);
> -	      expecting_sec_name = 0;
> +	      expecting_sec_name = false;
>  	    }
>  	  else if (expecting_sec_addr)
>  	    {
>  	      sect_opts.back ().value = arg;
> -	      expecting_sec_addr = 0;
> +	      expecting_sec_addr = false;
>  	    }
> -	  else if (strcmp (arg, "-readnow") == 0)
> -	    flags |= OBJF_READNOW;
> -	  else if (strcmp (arg, "-s") == 0)
> +	  else if (filename == NULL)
> +	    {
> +	      /* First non-option argument is always the filename.  */
> +	      filename.reset (tilde_expand (arg));
> +	    }
> +	  else if (!seen_addr)
>  	    {
> -	      expecting_sec_name = 1;
> -	      expecting_sec_addr = 1;
> +	      /* The second non-option argument is always the text
> +		 address at which to load the program.  */
> +	      sect_opt sect = { ".text", arg };
> +	      sect_opts.push_back (sect);
> +	      seen_addr = true;

Does this push_back directly here mean that these
two commands end up with different semantics?

 (gdb) add-symbol-file FILE 0 -s .text 0x1000
 (gdb) add-symbol-file -s .text 0x1000 FILE 0

Not sure that's a good idea.

Please add a test with "-s .text"...

> +# Check that we can pass parameters using any position in the command
> +# line.
> +gdb_test "add-symbol-file -readnow $binfile 0x0 -s .bss 0x3" \
> +    "Not confirmed\." \
> +    "add-symbol-file positionless -readnow" \
> +    "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = 0x0\r\n\t\.bss_addr = 0x3\r\n\\(y or n\\) " \
> +    "n"
> +# When we use -s as the first argument, the section will be printed
> +# first as well.
> +gdb_test "add-symbol-file -s .bss 0x3 -readnow $binfile 0x0" \
> +    "Not confirmed\." \
> +    "add-symbol-file positionless -s" \
> +    "add symbol table from file \"${binfile}\" at\r\n\t\.bss_addr = 0x3\r\n\t\.text_addr = 0x0\r\n\\(y or n\\) " \
> +    "n"
> +gdb_test "add-symbol-file $binfile 0x0 -s .bss 0x3" \
> +    "Not confirmed\." \
> +    "add-symbol-file positionless -s, no -readnow" \
> +    "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = 0x0\r\n\t\.bss_addr = 0x3\r\n\\(y or n\\) " \
> +    "n"

Using a number != 0x0 is a little better, since its easy for
a variable to end up always zero-initialized / zero-propagated
by mistake, and the test wouldn't notice.

> +# Since we're here, might as well test the 'symbol-file' command and
> +# if its arguments can also be passed at any position.
> +gdb_test "symbol-file -readnow $binfile" \
> +    "Reading symbols from ${binfile}\.\.\.expanding to full symbols\.\.\.done\." \
> +    "symbol-file with -readnow first"
> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir

Just use clean_restart with no argument.

> +gdb_test "symbol-file $binfile -readnow" \
> +    "Reading symbols from ${binfile}\.\.\.expanding to full symbols\.\.\.done\." \
> +    "symbol-file with -readnow second"
> +gdb_test "symbol-file -readnow" \
> +    "no symbol file name was specified" \
> +    "symbol-file without filename"
> +
> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir

Ditto.

> +
>  gdb_test "add-symbol-file ${binfile} 0 -s" \
>      "Missing section name after .-s." \
>      "add-symbol-file bare -s"
> 

Thanks,
Pedro Alves
Sergio Durigan Junior - Nov. 30, 2017, 12:38 p.m.
On Thursday, November 30 2017, Pedro Alves wrote:

> On 11/30/2017 04:24 AM, Sergio Durigan Junior wrote:
>> Changes from v1:
>> 
>> - Commit message has been rewritten.
>> 
>> - Implemented position-independent argument parsing for
>>   'add-symbol-file'.
>> 
>> - Added testcases.
>
> Looks like you missed the comment about "--".  Take a look at
> maintenance_print_symbols for an example of a command
> that supports ending options with "--".  Can you add that
> while you're at it, please?  For a test, I'd suggest
> e.g., "symbol-file -- -non-existent-file" and confirming
> gdb errors out.  That's simpler than actually creating a file.

Oh, I didn't understand that you were asking me to implement it.  I
thought it was a comment for "something to be done later".  I'll do it,
then.

>> +      if (*arg != '-')
>>  	{
>> -	  /* It's an option (starting with '-') or it's an argument
>> -	     to an option.  */
>>  	  if (expecting_sec_name)
>>  	    {
>>  	      sect_opt sect = { arg, NULL };
>>  	      sect_opts.push_back (sect);
>> -	      expecting_sec_name = 0;
>> +	      expecting_sec_name = false;
>>  	    }
>>  	  else if (expecting_sec_addr)
>>  	    {
>>  	      sect_opts.back ().value = arg;
>> -	      expecting_sec_addr = 0;
>> +	      expecting_sec_addr = false;
>>  	    }
>> -	  else if (strcmp (arg, "-readnow") == 0)
>> -	    flags |= OBJF_READNOW;
>> -	  else if (strcmp (arg, "-s") == 0)
>> +	  else if (filename == NULL)
>> +	    {
>> +	      /* First non-option argument is always the filename.  */
>> +	      filename.reset (tilde_expand (arg));
>> +	    }
>> +	  else if (!seen_addr)
>>  	    {
>> -	      expecting_sec_name = 1;
>> -	      expecting_sec_addr = 1;
>> +	      /* The second non-option argument is always the text
>> +		 address at which to load the program.  */
>> +	      sect_opt sect = { ".text", arg };
>> +	      sect_opts.push_back (sect);
>> +	      seen_addr = true;
>
> Does this push_back directly here mean that these
> two commands end up with different semantics?
>
>  (gdb) add-symbol-file FILE 0 -s .text 0x1000
>  (gdb) add-symbol-file -s .text 0x1000 FILE 0

The arguments are printed in a different order, yes:

(gdb) add-symbol-file -s .text 0x100 ./gdb/testsuite/outputs/gdb.base/relocate/relocate.o 0
add symbol table from file "./gdb/testsuite/outputs/gdb.base/relocate/relocate.o" at
        .text_addr = 0x100
        .text_addr = 0x0
(y or n) n
Not confirmed.
(gdb) add-symbol-file  ./gdb/testsuite/outputs/gdb.base/relocate/relocate.o 0 -s .text 0x100
add symbol table from file "./gdb/testsuite/outputs/gdb.base/relocate/relocate.o" at
        .text_addr = 0x0
        .text_addr = 0x100
(y or n) n
Not confirmed.

> Not sure that's a good idea.

I'll work on it and make sure they're always printed in the same way.

> Please add a test with "-s .text"...

Will do.

>> +# Check that we can pass parameters using any position in the command
>> +# line.
>> +gdb_test "add-symbol-file -readnow $binfile 0x0 -s .bss 0x3" \
>> +    "Not confirmed\." \
>> +    "add-symbol-file positionless -readnow" \
>> +    "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = 0x0\r\n\t\.bss_addr = 0x3\r\n\\(y or n\\) " \
>> +    "n"
>> +# When we use -s as the first argument, the section will be printed
>> +# first as well.
>> +gdb_test "add-symbol-file -s .bss 0x3 -readnow $binfile 0x0" \
>> +    "Not confirmed\." \
>> +    "add-symbol-file positionless -s" \
>> +    "add symbol table from file \"${binfile}\" at\r\n\t\.bss_addr = 0x3\r\n\t\.text_addr = 0x0\r\n\\(y or n\\) " \
>> +    "n"
>> +gdb_test "add-symbol-file $binfile 0x0 -s .bss 0x3" \
>> +    "Not confirmed\." \
>> +    "add-symbol-file positionless -s, no -readnow" \
>> +    "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = 0x0\r\n\t\.bss_addr = 0x3\r\n\\(y or n\\) " \
>> +    "n"
>
> Using a number != 0x0 is a little better, since its easy for
> a variable to end up always zero-initialized / zero-propagated
> by mistake, and the test wouldn't notice.

OK.

>> +# Since we're here, might as well test the 'symbol-file' command and
>> +# if its arguments can also be passed at any position.
>> +gdb_test "symbol-file -readnow $binfile" \
>> +    "Reading symbols from ${binfile}\.\.\.expanding to full symbols\.\.\.done\." \
>> +    "symbol-file with -readnow first"
>> +gdb_exit
>> +gdb_start
>> +gdb_reinitialize_dir $srcdir/$subdir
>
> Just use clean_restart with no argument.

Done.

>> +gdb_test "symbol-file $binfile -readnow" \
>> +    "Reading symbols from ${binfile}\.\.\.expanding to full symbols\.\.\.done\." \
>> +    "symbol-file with -readnow second"
>> +gdb_test "symbol-file -readnow" \
>> +    "no symbol file name was specified" \
>> +    "symbol-file without filename"
>> +
>> +gdb_exit
>> +gdb_start
>> +gdb_reinitialize_dir $srcdir/$subdir
>
> Ditto.

Done.

>> +
>>  gdb_test "add-symbol-file ${binfile} 0 -s" \
>>      "Missing section name after .-s." \
>>      "add-symbol-file bare -s"
>> 
>
> Thanks,
> Pedro Alves

Thanks,
Pedro Alves - Nov. 30, 2017, 12:48 p.m.
On 11/30/2017 12:38 PM, Sergio Durigan Junior wrote:
> On Thursday, November 30 2017, Pedro Alves wrote:
> 
>> On 11/30/2017 04:24 AM, Sergio Durigan Junior wrote:
>>> Changes from v1:
>>>
>>> - Commit message has been rewritten.
>>>
>>> - Implemented position-independent argument parsing for
>>>   'add-symbol-file'.
>>>
>>> - Added testcases.
>>
>> Looks like you missed the comment about "--".  Take a look at
>> maintenance_print_symbols for an example of a command
>> that supports ending options with "--".  Can you add that
>> while you're at it, please?  For a test, I'd suggest
>> e.g., "symbol-file -- -non-existent-file" and confirming
>> gdb errors out.  That's simpler than actually creating a file.
> 
> Oh, I didn't understand that you were asking me to implement it.  I
> thought it was a comment for "something to be done later".  I'll do it,
> then.

I'd rather do it at the same time because there's the possibility
that implementing it suggests implementing any-position arguments
differently.  Or maybe not.  But I'd rather that we didn't have
to redo any-position support later on again.

>>
>> Does this push_back directly here mean that these
>> two commands end up with different semantics?
>>
>>  (gdb) add-symbol-file FILE 0 -s .text 0x1000
>>  (gdb) add-symbol-file -s .text 0x1000 FILE 0
> 
> The arguments are printed in a different order, yes:
> 
> (gdb) add-symbol-file -s .text 0x100 ./gdb/testsuite/outputs/gdb.base/relocate/relocate.o 0
> add symbol table from file "./gdb/testsuite/outputs/gdb.base/relocate/relocate.o" at
>         .text_addr = 0x100
>         .text_addr = 0x0
> (y or n) n
> Not confirmed.
> (gdb) add-symbol-file  ./gdb/testsuite/outputs/gdb.base/relocate/relocate.o 0 -s .text 0x100
> add symbol table from file "./gdb/testsuite/outputs/gdb.base/relocate/relocate.o" at
>         .text_addr = 0x0
>         .text_addr = 0x100
> (y or n) n
> Not confirmed.
> 
>> Not sure that's a good idea.
> 
> I'll work on it and make sure they're always printed in the same way.

TBC, the worry was not really about the printing (that's minor),
but what is the actual resulting .text address that the symbol file
it loaded at in the end.  But sounds like the printing order can
be used as proxy for checking that.  It's not as robust as
actually loading the symbols and then checking what happened
(with e.g., printing a symbol's address), but it's good
enough.

Thanks,
Pedro Alves
Sergio Durigan Junior - Nov. 30, 2017, 1:06 p.m.
On Thursday, November 30 2017, Pedro Alves wrote:

>>> Does this push_back directly here mean that these
>>> two commands end up with different semantics?
>>>
>>>  (gdb) add-symbol-file FILE 0 -s .text 0x1000
>>>  (gdb) add-symbol-file -s .text 0x1000 FILE 0
>> 
>> The arguments are printed in a different order, yes:
>> 
>> (gdb) add-symbol-file -s .text 0x100 ./gdb/testsuite/outputs/gdb.base/relocate/relocate.o 0
>> add symbol table from file "./gdb/testsuite/outputs/gdb.base/relocate/relocate.o" at
>>         .text_addr = 0x100
>>         .text_addr = 0x0
>> (y or n) n
>> Not confirmed.
>> (gdb) add-symbol-file  ./gdb/testsuite/outputs/gdb.base/relocate/relocate.o 0 -s .text 0x100
>> add symbol table from file "./gdb/testsuite/outputs/gdb.base/relocate/relocate.o" at
>>         .text_addr = 0x0
>>         .text_addr = 0x100
>> (y or n) n
>> Not confirmed.
>> 
>>> Not sure that's a good idea.
>> 
>> I'll work on it and make sure they're always printed in the same way.
>
> TBC, the worry was not really about the printing (that's minor),
> but what is the actual resulting .text address that the symbol file
> it loaded at in the end.  But sounds like the printing order can
> be used as proxy for checking that.  It's not as robust as
> actually loading the symbols and then checking what happened
> (with e.g., printing a symbol's address), but it's good
> enough.

Right, I got it.

The way I made it now, the address provided as .text will always be the
first to be set/printed, even if the user provides a "-s .text" later.
This maintains compatibility with the current way of doing things.

Patch

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 4bbe0b5a62..1d6989c1b9 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1634,14 +1634,13 @@  symbol_file_command (const char *args, int from_tty)
 	  else if (*arg == '-')
 	    error (_("unknown option `%s'"), arg);
 	  else
-	    {
-	      symbol_file_add_main_1 (arg, add_flags, flags);
-	      name = arg;
-	    }
+	    name = arg;
 	}
 
       if (name == NULL)
 	error (_("no symbol file name was specified"));
+
+      symbol_file_add_main_1 (name, add_flags, flags);
     }
 }
 
@@ -2180,8 +2179,9 @@  add_symbol_file_command (const char *args, int from_tty)
   char *arg;
   int argcnt = 0;
   int sec_num = 0;
-  int expecting_sec_name = 0;
-  int expecting_sec_addr = 0;
+  bool expecting_sec_name = false;
+  bool expecting_sec_addr = false;
+  bool seen_addr = false;
   struct objfile *objf;
   objfile_flags flags = OBJF_USERLOADED | OBJF_SHARED;
   symfile_add_flags add_flags = 0;
@@ -2208,46 +2208,49 @@  add_symbol_file_command (const char *args, int from_tty)
 
   for (arg = argv[0], argcnt = 0; arg != NULL; arg = argv[++argcnt])
     {
-      /* Process the argument.  */
-      if (argcnt == 0)
-	{
-	  /* The first argument is the file name.  */
-	  filename.reset (tilde_expand (arg));
-	}
-      else if (argcnt == 1)
-	{
-	  /* The second argument is always the text address at which
-	     to load the program.  */
-	  sect_opt sect = { ".text", arg };
-	  sect_opts.push_back (sect);
-	}
-      else
+      if (*arg != '-')
 	{
-	  /* It's an option (starting with '-') or it's an argument
-	     to an option.  */
 	  if (expecting_sec_name)
 	    {
 	      sect_opt sect = { arg, NULL };
 	      sect_opts.push_back (sect);
-	      expecting_sec_name = 0;
+	      expecting_sec_name = false;
 	    }
 	  else if (expecting_sec_addr)
 	    {
 	      sect_opts.back ().value = arg;
-	      expecting_sec_addr = 0;
+	      expecting_sec_addr = false;
 	    }
-	  else if (strcmp (arg, "-readnow") == 0)
-	    flags |= OBJF_READNOW;
-	  else if (strcmp (arg, "-s") == 0)
+	  else if (filename == NULL)
+	    {
+	      /* First non-option argument is always the filename.  */
+	      filename.reset (tilde_expand (arg));
+	    }
+	  else if (!seen_addr)
 	    {
-	      expecting_sec_name = 1;
-	      expecting_sec_addr = 1;
+	      /* The second non-option argument is always the text
+		 address at which to load the program.  */
+	      sect_opt sect = { ".text", arg };
+	      sect_opts.push_back (sect);
+	      seen_addr = true;
 	    }
 	  else
 	    error (_("Unrecognized argument \"%s\""), arg);
 	}
+      else if (strcmp (arg, "-readnow") == 0)
+	flags |= OBJF_READNOW;
+      else if (strcmp (arg, "-s") == 0)
+	{
+	  expecting_sec_name = true;
+	  expecting_sec_addr = true;
+	}
+      else
+	error (_("Unrecognized argument \"%s\""), arg);
     }
 
+  if (filename == NULL)
+    error (_("You must provide a filename to be loaded."));
+
   /* 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
diff --git a/gdb/testsuite/gdb.base/relocate.exp b/gdb/testsuite/gdb.base/relocate.exp
index 6eef15fb20..6a5ec5e201 100644
--- a/gdb/testsuite/gdb.base/relocate.exp
+++ b/gdb/testsuite/gdb.base/relocate.exp
@@ -37,6 +37,44 @@  foreach x {"-raednow" "readnow" "foo" "-readnow s"} {
 	"add-symbol-file: unknown option $word"
 }
 
+# Check that we can pass parameters using any position in the command
+# line.
+gdb_test "add-symbol-file -readnow $binfile 0x0 -s .bss 0x3" \
+    "Not confirmed\." \
+    "add-symbol-file positionless -readnow" \
+    "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = 0x0\r\n\t\.bss_addr = 0x3\r\n\\(y or n\\) " \
+    "n"
+# When we use -s as the first argument, the section will be printed
+# first as well.
+gdb_test "add-symbol-file -s .bss 0x3 -readnow $binfile 0x0" \
+    "Not confirmed\." \
+    "add-symbol-file positionless -s" \
+    "add symbol table from file \"${binfile}\" at\r\n\t\.bss_addr = 0x3\r\n\t\.text_addr = 0x0\r\n\\(y or n\\) " \
+    "n"
+gdb_test "add-symbol-file $binfile 0x0 -s .bss 0x3" \
+    "Not confirmed\." \
+    "add-symbol-file positionless -s, no -readnow" \
+    "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = 0x0\r\n\t\.bss_addr = 0x3\r\n\\(y or n\\) " \
+    "n"
+# Since we're here, might as well test the 'symbol-file' command and
+# if its arguments can also be passed at any position.
+gdb_test "symbol-file -readnow $binfile" \
+    "Reading symbols from ${binfile}\.\.\.expanding to full symbols\.\.\.done\." \
+    "symbol-file with -readnow first"
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_test "symbol-file $binfile -readnow" \
+    "Reading symbols from ${binfile}\.\.\.expanding to full symbols\.\.\.done\." \
+    "symbol-file with -readnow second"
+gdb_test "symbol-file -readnow" \
+    "no symbol file name was specified" \
+    "symbol-file without filename"
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
 gdb_test "add-symbol-file ${binfile} 0 -s" \
     "Missing section name after .-s." \
     "add-symbol-file bare -s"