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

[Pedro Alves <palves@redhat.com>]

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