diff mbox

Make 'symbol-file' not care about the position of command line arguments

Message ID 20171129214451.14257-1-sergiodj@redhat.com
State New, archived
Headers

Commit Message

Sergio Durigan Junior Nov. 29, 2017, 9:44 p.m. UTC 
  This is a bug that's been detected while doing the readnever work.
Currently if you use the 'symbol-file' command you have to be careful
about the position of each argument you pass on the command line.
This is because while parsing its arguments, if the command detects a
filename, it promptly calls 'symbol_file_add_main_1' without waiting
to see if there are other args on the line.  This only affects the
'-readnow' argument so far, but while implementing the '-readnever'
command it also affected it.

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.
---
 gdb/symfile.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Pedro Alves Nov. 29, 2017, 10:26 p.m. UTC | #1 
On 11/29/2017 09:44 PM, Sergio Durigan Junior wrote:
> This is a bug that's been detected while doing the readnever work.
> Currently if you use the 'symbol-file' command you have to be careful
> about the position of each argument you pass on the command line.
> This is because while parsing its arguments, if the command detects a
> filename, it promptly calls 'symbol_file_add_main_1' without waiting
> to see if there are other args on the line.  This only affects the
> '-readnow' argument so far, but while implementing the '-readnever'
> command it also affected it.
> 

Testcase or it didn't happen?  :-)

I hadn't really understood what this was about in the other thread.
(Now I do.)  I wonder whether it's really desirable to make this
work.  It seems to me that it's much more usual in GDB for option
processing to stop at the first argument that doesn't start
with '-'?  I.e., like getopt on most platforms.  (The related
add-symbol-file command stands out as quite odd to me for
explicitly wanting '-'-options after non-'-' options...)

Thanks,
Pedro Alves
Sergio Durigan Junior Nov. 29, 2017, 10:42 p.m. UTC | #2 
On Wednesday, November 29 2017, Pedro Alves wrote:

> On 11/29/2017 09:44 PM, Sergio Durigan Junior wrote:
>> This is a bug that's been detected while doing the readnever work.
>> Currently if you use the 'symbol-file' command you have to be careful
>> about the position of each argument you pass on the command line.
>> This is because while parsing its arguments, if the command detects a
>> filename, it promptly calls 'symbol_file_add_main_1' without waiting
>> to see if there are other args on the line.  This only affects the
>> '-readnow' argument so far, but while implementing the '-readnever'
>> command it also affected it.
>> 
>
> Testcase or it didn't happen?  :-)

Can we negotiate this?  :-)

I absolutely agree (and should have written about it in the commit log),
but it's easier to write a testcase for the -readnever.  Actually, I
have one already written.  So I'd like to "postpone" the testcase until
the readnever feature is in.  OK?

> I hadn't really understood what this was about in the other thread.
> (Now I do.)  I wonder whether it's really desirable to make this
> work.  It seems to me that it's much more usual in GDB for option
> processing to stop at the first argument that doesn't start
> with '-'?  I.e., like getopt on most platforms.  (The related
> add-symbol-file command stands out as quite odd to me for
> explicitly wanting '-'-options after non-'-' options...)

I didn't know getopt stopped processing after the first non-'-'
argument.  I've always considered that passing '--' is the de facto way
of telling getopt (or argp) to stop processing.

I find it very confusing to have positional arguments in a command line.
This is not intuitive, and there's usually no indication that the
command expects a fixed position for its arguments (as is the case with
'symbol-file', for example).  I consider this to be a bug, if you ask
me.
Pedro Alves Nov. 29, 2017, 11:15 p.m. UTC | #3 
On 11/29/2017 10:42 PM, Sergio Durigan Junior wrote:
> On Wednesday, November 29 2017, Pedro Alves wrote:
> 
>> On 11/29/2017 09:44 PM, Sergio Durigan Junior wrote:
>>> This is a bug that's been detected while doing the readnever work.
>>> Currently if you use the 'symbol-file' command you have to be careful
>>> about the position of each argument you pass on the command line.
>>> This is because while parsing its arguments, if the command detects a
>>> filename, it promptly calls 'symbol_file_add_main_1' without waiting
>>> to see if there are other args on the line.  This only affects the
>>> '-readnow' argument so far, but while implementing the '-readnever'
>>> command it also affected it.
>>>
>>
>> Testcase or it didn't happen?  :-)
> 
> Can we negotiate this?  :-)
> 
> I absolutely agree (and should have written about it in the commit log),
> but it's easier to write a testcase for the -readnever.  Actually, I
> have one already written.  So I'd like to "postpone" the testcase until
> the readnever feature is in.  OK?

Doesn't -readnow work for that just the same?

>> I hadn't really understood what this was about in the other thread.
>> (Now I do.)  I wonder whether it's really desirable to make this
>> work.  It seems to me that it's much more usual in GDB for option
>> processing to stop at the first argument that doesn't start
>> with '-'?  I.e., like getopt on most platforms.  (The related
>> add-symbol-file command stands out as quite odd to me for
>> explicitly wanting '-'-options after non-'-' options...)
> 
> I didn't know getopt stopped processing after the first non-'-'
> argument.

Most implementations of getopt do.  GNU getopt is known for
reordering non-'-' arguments.  You can override that by
setting POSIXLY_CORRECT in the environment, for example.

> I've always considered that passing '--' is the de facto way
> of telling getopt (or argp) to stop processing.

"--" is used to stop processing options when you want to pass
a non-option argument that starts with "-", like imagine if you
wanted to pass a filename that starts with "-" to symbol-file
(the filename is not an option.)

> 
> I find it very confusing to have positional arguments in a command line.
> This is not intuitive, and there's usually no indication that the
> command expects a fixed position for its arguments (as is the case with
> 'symbol-file', for example).  I consider this to be a bug, if you ask
> me.

In this case it's right there in the online help (since Tom's patch):

 (gdb) help symbol-file
 Load symbol table from executable file FILE.
 Usage: symbol-file [-readnow] FILE

Note that supporting non-option arguments before options
is impossible for commands that need to handle raw arguments.
I.e., commands that want to supporting passing arguments
arguments with spaces, without forcing the user to wrap it
all in quotes.  Like "break -q function (int)", or
"print /x EXPRESSION_WITH_SPACES", etc.  (I have a WIP series
that adds '-'-options to "print", btw.)
It's good to keep the possibility of the command being extended
in that direction in mind.  It doesn't seem to be the case
here, but that's the angle I was coming from.

Thanks,
Pedro Alves
Sergio Durigan Junior Nov. 30, 2017, 12:08 a.m. UTC | #4 
On Wednesday, November 29 2017, Pedro Alves wrote:

> On 11/29/2017 10:42 PM, Sergio Durigan Junior wrote:
>> On Wednesday, November 29 2017, Pedro Alves wrote:
>> 
>>> On 11/29/2017 09:44 PM, Sergio Durigan Junior wrote:
>>>> This is a bug that's been detected while doing the readnever work.
>>>> Currently if you use the 'symbol-file' command you have to be careful
>>>> about the position of each argument you pass on the command line.
>>>> This is because while parsing its arguments, if the command detects a
>>>> filename, it promptly calls 'symbol_file_add_main_1' without waiting
>>>> to see if there are other args on the line.  This only affects the
>>>> '-readnow' argument so far, but while implementing the '-readnever'
>>>> command it also affected it.
>>>>
>>>
>>> Testcase or it didn't happen?  :-)
>> 
>> Can we negotiate this?  :-)
>> 
>> I absolutely agree (and should have written about it in the commit log),
>> but it's easier to write a testcase for the -readnever.  Actually, I
>> have one already written.  So I'd like to "postpone" the testcase until
>> the readnever feature is in.  OK?
>
> Doesn't -readnow work for that just the same?

Well, yeah, I've just confirmed that it's possible to match
"...expanding to full symbols..." when -readnow is used, so it's
possible to provide a testcase even now, indeed.

I'll wait until we reach a conclusion on whether this patch is useful or
not, and then submit the testcase along with v2.

>>> I hadn't really understood what this was about in the other thread.
>>> (Now I do.)  I wonder whether it's really desirable to make this
>>> work.  It seems to me that it's much more usual in GDB for option
>>> processing to stop at the first argument that doesn't start
>>> with '-'?  I.e., like getopt on most platforms.  (The related
>>> add-symbol-file command stands out as quite odd to me for
>>> explicitly wanting '-'-options after non-'-' options...)
>> 
>> I didn't know getopt stopped processing after the first non-'-'
>> argument.
>
> Most implementations of getopt do.  GNU getopt is known for
> reordering non-'-' arguments.  You can override that by
> setting POSIXLY_CORRECT in the environment, for example.
>
>> I've always considered that passing '--' is the de facto way
>> of telling getopt (or argp) to stop processing.
>
> "--" is used to stop processing options when you want to pass
> a non-option argument that starts with "-", like imagine if you
> wanted to pass a filename that starts with "-" to symbol-file
> (the filename is not an option.)

Ah, true, that makes sense.

>> 
>> I find it very confusing to have positional arguments in a command line.
>> This is not intuitive, and there's usually no indication that the
>> command expects a fixed position for its arguments (as is the case with
>> 'symbol-file', for example).  I consider this to be a bug, if you ask
>> me.
>
> In this case it's right there in the online help (since Tom's patch):
>
>  (gdb) help symbol-file
>  Load symbol table from executable file FILE.
>  Usage: symbol-file [-readnow] FILE

Sorry, but to me this doesn't say much.  I read this help and think "Oh,
-readnow can be passed as an option, OK."  Maybe that's my fault, but if
I was writing a command like "symbol-file FILE" and just remembered that
I wanted -readnow, I would include it in the end of the line.  In fact,
that's exactly what happened when I was testing -readnever.  I saw that
the flag had no effect when the last parameter, which made me go after
the cause.

> Note that supporting non-option arguments before options
> is impossible for commands that need to handle raw arguments.
> I.e., commands that want to supporting passing arguments
> arguments with spaces, without forcing the user to wrap it
> all in quotes.  Like "break -q function (int)", or
> "print /x EXPRESSION_WITH_SPACES", etc.  (I have a WIP series
> that adds '-'-options to "print", btw.)
> It's good to keep the possibility of the command being extended
> in that direction in mind.  It doesn't seem to be the case
> here, but that's the angle I was coming from.

I agree, but that's not the case with 'symbol-file' or
'add-symbol-file', right?  I mean, their only non-option argument is the
FILE, which won't have spaces (or will, but they'll be quoted like they
should).

Sorry for being so persistent, but I was bit by this bug and it wasn't
nice.
Pedro Alves Nov. 30, 2017, 12:34 a.m. UTC | #5 
On 11/30/2017 12:08 AM, Sergio Durigan Junior wrote:
> On Wednesday, November 29 2017, Pedro Alves wrote:
> 
> I'll wait until we reach a conclusion on whether this patch is useful or
> not, and then submit the testcase along with v2.

I don't mind this going in, provided it comes with a testcase.

>>> I've always considered that passing '--' is the de facto way
>>> of telling getopt (or argp) to stop processing.
>>
>> "--" is used to stop processing options when you want to pass
>> a non-option argument that starts with "-", like imagine if you
>> wanted to pass a filename that starts with "-" to symbol-file
>> (the filename is not an option.)
>
> Ah, true, that makes sense.
>

... which points out that we should really handle "--", for
the possibility of such filenames.

(At some point we should really come up with some getopt-like
API that can be used by all the commands that also use gdb_argv,
to help with normalizing the interface, and also to make it
easier to add new options to commands.)

>> Note that supporting non-option arguments before options
>> is impossible for commands that need to handle raw arguments.
>> I.e., commands that want to supporting passing arguments
>> arguments with spaces, without forcing the user to wrap it
>> all in quotes.  Like "break -q function (int)", or
>> "print /x EXPRESSION_WITH_SPACES", etc.  (I have a WIP series
>> that adds '-'-options to "print", btw.)
>> It's good to keep the possibility of the command being extended
>> in that direction in mind.  It doesn't seem to be the case
>> here, but that's the angle I was coming from.
> 
> I agree, but that's not the case with 'symbol-file' or
> 'add-symbol-file', right?  

Yes, that's what I said - "it doesn't seem to be the case here".

Are we still negotiating?  Can I get you to make
"add-symbol-file" process options in any position too?  :-)

> I mean, their only non-option argument is the
> FILE, which won't have spaces (or will, but they'll be quoted like they
> should).
> 
> Sorry for being so persistent, but I was bit by this bug and it wasn't
> nice.

Thanks,
Pedro Alves
Sergio Durigan Junior Nov. 30, 2017, 4:07 a.m. UTC | #6 
On Wednesday, November 29 2017, Pedro Alves wrote:

> On 11/30/2017 12:08 AM, Sergio Durigan Junior wrote:
>> On Wednesday, November 29 2017, Pedro Alves wrote:
>> 
>> I'll wait until we reach a conclusion on whether this patch is useful or
>> not, and then submit the testcase along with v2.
>
> I don't mind this going in, provided it comes with a testcase.

I'm working on it right now.

>>>> I've always considered that passing '--' is the de facto way
>>>> of telling getopt (or argp) to stop processing.
>>>
>>> "--" is used to stop processing options when you want to pass
>>> a non-option argument that starts with "-", like imagine if you
>>> wanted to pass a filename that starts with "-" to symbol-file
>>> (the filename is not an option.)
>>
>> Ah, true, that makes sense.
>>
>
> ... which points out that we should really handle "--", for
> the possibility of such filenames.
>
> (At some point we should really come up with some getopt-like
> API that can be used by all the commands that also use gdb_argv,
> to help with normalizing the interface, and also to make it
> easier to add new options to commands.)

That'd be super.  It's been on my mind for quite some time, too.  I'm
impressed such interface doesn't exist yet.

>>> Note that supporting non-option arguments before options
>>> is impossible for commands that need to handle raw arguments.
>>> I.e., commands that want to supporting passing arguments
>>> arguments with spaces, without forcing the user to wrap it
>>> all in quotes.  Like "break -q function (int)", or
>>> "print /x EXPRESSION_WITH_SPACES", etc.  (I have a WIP series
>>> that adds '-'-options to "print", btw.)
>>> It's good to keep the possibility of the command being extended
>>> in that direction in mind.  It doesn't seem to be the case
>>> here, but that's the angle I was coming from.
>> 
>> I agree, but that's not the case with 'symbol-file' or
>> 'add-symbol-file', right?  
>
> Yes, that's what I said - "it doesn't seem to be the case here".
>
> Are we still negotiating?  Can I get you to make
> "add-symbol-file" process options in any position too?  :-)

Sure thing!  I'll give it a try.

Thanks,
diff mbox

Patch

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 4bbe0b5a62..9565570734 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 (arg, add_flags, flags);
     }
 }