Page MenuHomePhabricator

[Driver] Extract option parsing and option processing.
AbandonedPublic

Authored by JDevlieghere on Nov 18 2018, 7:17 PM.

Details

Summary

In order to deal consistently with global state in LLDB, the reproducer feature affects LLDB's initialization. For example, when replaying, the FileSystem singleton is initialized with a virtual file system.

This is a problem for the driver because it initialized the debugger before parsing command line options. The reason is that the driver, among other things, checks whether files exists (e.g. core files, target, files to be sourced). It also relies on the debugger to parse things like the (scripting) language, the architecture, etc.

In an initial attempt I tried to populate the OptionData before the debugger is initialized. This proved to be complicated, because of the sanity checks that are performed by calling into the debugger of the filesystem. Although it would be possible to perform these checks after parsing, it would cause errors to no longer appear in the same order as specified by the user, but in an arbitrary order specified by the driver implementation. Although I like the idea conceptually I don't believe this is an acceptable regression.

Implemented in this patch is a new ArgParser class that extracts the existing argument parsing logic. Basically it calls getopt_long_only repeatedly and populates a list with the short option and its value. Because the ArgParser is dumb it can do all its work before the debugger is initialized. Afterwards the driver iterates over the options from the argparser (instead of calling getopt_long_only every time) and do whatever is needed.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

JDevlieghere created this revision.Nov 18 2018, 7:17 PM

I’ve often thought we should convert LLDB’s command line parsing code over
to use either cl::opt or lib/Option. This would also solve the problem you
describe here at the same time.

Do you think it’s worth trying to do this?

I’ve often thought we should convert LLDB’s command line parsing code over
to use either cl::opt or lib/Option. This would also solve the problem you
describe here at the same time.

Do you think it’s worth trying to do this?

I believe both would have the issue that options are not processed in the order specified. I guess it depends on whether people care about this? Happy to give that a shot if the consensus is "nobody cares" :-)

Lib option definitely does, as its entire purpose is to be powerful and
flexible enough to mimic arbitrary command line tools.

cl::opt is a little less powerful but it still preserves order among
multiple options with the same flag, just not multiple options with
different flags.

I don’t have a strong opinion that this particular change should be gated
on that, it’s just nice when we can reuse llvm logic. Sometimes it fixes
latent bugs, sometimes it enables new functionality that wasn’t possible
before, etc. up to you if you wanna give it a shot

Lib option definitely does, as its entire purpose is to be powerful and
flexible enough to mimic arbitrary command line tools.

I created D54692 to show what the driver would look like using libOption. Personally I think the code looks better but the help is more messy, especially with the aliases. It's more consistent with LLVM though, which has to count for something.

The only thing that I found missing was support for the "end of options" marker (--). I ended up implementing a simple work-around, but it seems like the lib could benefit from supporting that. I'll have a look when I have some spare time.

cl::opt is a little less powerful but it still preserves order among
multiple options with the same flag, just not multiple options with
different flags.

I don’t have a strong opinion that this particular change should be gated
on that, it’s just nice when we can reuse llvm logic. Sometimes it fixes
latent bugs, sometimes it enables new functionality that wasn’t possible
before, etc. up to you if you wanna give it a shot

I'm always a strong proponent of reusing llvm logic.

If by "it would cause errors to no longer appear in the same order as specified by the user, but in an arbitrary order specified by the driver implementation", you mean that now for a command line like:

$ new/lldb -S /tmp/sadg --file /tmp/qwr

I get

error: file specified in --file (-f) option doesn't exist: '/tmp/qwr'

instead of

error: file specified in --source (-s) option doesn't exist: '/tmp/sadg'

then that's something I really don't care about.

I have to agree that the new --help output is less readable than the old one, but that's a price I would be willing to pay for getting rid of getopt. And this can be easily improved in the future, either by rolling our own --help output (as we do now anyway), or by improving the central libOption help printer (thereby improving clang --help output as well, which I also find lacking in readability).

So yes, I'm in favour of the libOption approach, or at least cleaning this up by "sacrificing" the error message order.

(I also have to point out that for libOption to work, someone will have to dive into the xcode project to get the tablegen rules in there.)

JDevlieghere abandoned this revision.Nov 26 2018, 9:02 AM

Thanks everyone! Looks like the general consensus is in favor of using libOption, so I'm abandoning this in favor of D54692. Please continue the discussion there.