Page MenuHomePhabricator

[lldb/Driver] Fix handling on positional arguments
ClosedPublic

Authored by JDevlieghere on May 18 2020, 3:13 PM.

Details

Summary

Before the transition to libOption it was possible to specify arguments for the inferior without -- as long as they didn't start with a dash.

For example, the following invocations should all behave the same:

$ lldb inferior inferior-arg
$ lldb inferior -- inferior-arg
$ lldb -- inferior inferior-arg

This patch fixes that behavior and adds a test to cover the different combinations.

Diff Detail

Event Timeline

JDevlieghere created this revision.May 18 2020, 3:13 PM

One thing the current approach allows is foo bar -o 'run' baz which will set target.run-args to "bar" "baz" . I guess that's not really desirable, but I'm not sure it outweighs the added complexity of tracking the beginning and thee end.

So when talking about "prior to libOption", are we talking getopt()? I believe that getopt would have an issue with:

$ lldb inferior --inferior-arg 

As it would not understand the argument. Or was there an intermediate llvm option parser we were using after getopt()?

And BTW, getopt allows arguments and options to be interspersed:

$ lldb --lldb-option1 inferior --lldb-option2=123 inferior-arg1 -- --inferior-opt=444 inferior-arg2

Not sure why you need to track "beginning and end". Does libOption not pull out all the options (and their option values if they have them) before handing you the command line? If not, then this probably is not worth doing. But if it does, this should be straightforward since the only arguments lldb takes (prior to seeing a --) are meant to be arguments to the program when you run it.

JDevlieghere updated this revision to Diff 264745.EditedMay 18 2020, 4:09 PM
JDevlieghere edited the summary of this revision. (Show Details)

Apparently I misremembered the old behavior. Jim refreshed my memory about what he expects the behavior to be link. I've reworked the patch to match that. The result is actually less complex than it was before.

Please ignore the old patch & description.

Add test for lldb inferior -b inferior-arg -- inferior-arg2 as Greg pointed out.

Document the behavior in the help and man page.

jingham accepted this revision.May 18 2020, 5:15 PM

LGTM. These were useful behaviors, thanks for restoring them!

This revision is now accepted and ready to land.May 18 2020, 5:15 PM
clayborg accepted this revision.May 18 2020, 5:42 PM

Much better!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2020, 6:59 PM

Thanks for fixing this. I remember running into this a couple of times, but I never got around to do something about it...

lldb/test/Shell/Driver/TestPositionalArgs.test
27–28

Unrelated to this patch, but.. do we actually want this behavior? I don't think these were just "warnings" before libOption. Given that clang treats (at least some) unknown options as errors, I'm guessing there must be some libOption knob for controlling the behavior of this...

JDevlieghere marked 2 inline comments as done.May 19 2020, 8:49 AM
JDevlieghere added inline comments.
lldb/test/Shell/Driver/TestPositionalArgs.test
27–28

What behavior do you have in mind? Make these errors and exit early? It's just us reporting the warning, we can do anything we want with it.

labath added inline comments.May 19 2020, 9:58 AM
lldb/test/Shell/Driver/TestPositionalArgs.test
27–28

Yes, I think it would be more reasonable to report an error and exit in case we encounter an argument we don't recognise.

I agree with Pavel, reporting an error and exiting seems like a good behavior. When I mistype a command line option, I generally immediately quit lldb, up-arrow and fix and resubmit, so this would save keystrokes.

We should make sure if we do exit that we don't output any other text that would obscure the error message. It should be easy to spot the error both so you can easily fix it and to prevent you from typing lldb commands into your shell. If libOption had a "nearest option name to the one you typed" facility that would be useful in this case as well.

Does anybody remember what gdb does when you mistype a command-line option? We're not at all required to model their behavior, but it would be interesting to consider.

We should make sure if we do exit that we don't output any other text that would obscure the error message. It should be easy to spot the error both so you can easily fix it and to prevent you from typing lldb commands into your shell. If libOption had a "nearest option name to the one you typed" facility that would be useful in this case as well.

That facility exists. Clang uses it for command-line "fix-its". I am not sure what it takes to make use of it.

Does anybody remember what gdb does when you mistype a command-line option? We're not at all required to model their behavior, but it would be interesting to consider.

$ gdb -foobar
gdb: unrecognized option '-foobar'
Use `gdb --help' for a complete list of options.

We should make sure if we do exit that we don't output any other text that would obscure the error message. It should be easy to spot the error both so you can easily fix it and to prevent you from typing lldb commands into your shell. If libOption had a "nearest option name to the one you typed" facility that would be useful in this case as well.

That facility exists. Clang uses it for command-line "fix-its". I am not sure what it takes to make use of it.

Jonas said this wasn't part of libOption, but was done by hand in clang.

Does anybody remember what gdb does when you mistype a command-line option? We're not at all required to model their behavior, but it would be interesting to consider.

$ gdb -foobar
gdb: unrecognized option '-foobar'
Use `gdb --help' for a complete list of options.

You omitted the bit of text that would show whether it quit or not...

We should make sure if we do exit that we don't output any other text that would obscure the error message. It should be easy to spot the error both so you can easily fix it and to prevent you from typing lldb commands into your shell. If libOption had a "nearest option name to the one you typed" facility that would be useful in this case as well.

That facility exists. Clang uses it for command-line "fix-its". I am not sure what it takes to make use of it.

Jonas said this wasn't part of libOption, but was done by hand in clang.

Oh, that's too bad.

Does anybody remember what gdb does when you mistype a command-line option? We're not at all required to model their behavior, but it would be interesting to consider.

$ gdb -foobar
gdb: unrecognized option '-foobar'
Use `gdb --help' for a complete list of options.

You omitted the bit of text that would show whether it quit or not...

It did quit. With exit code 1. :)

We should make sure if we do exit that we don't output any other text that would obscure the error message. It should be easy to spot the error both so you can easily fix it and to prevent you from typing lldb commands into your shell. If libOption had a "nearest option name to the one you typed" facility that would be useful in this case as well.

That facility exists. Clang uses it for command-line "fix-its". I am not sure what it takes to make use of it.

Jonas said this wasn't part of libOption, but was done by hand in clang.

Oh, that's too bad.

Does anybody remember what gdb does when you mistype a command-line option? We're not at all required to model their behavior, but it would be interesting to consider.

$ gdb -foobar
gdb: unrecognized option '-foobar'
Use `gdb --help' for a complete list of options.

You omitted the bit of text that would show whether it quit or not...

It did quit. With exit code 1. :)

Great, then we all agree that exiting is the right thing to do!