Page MenuHomePhabricator

[Driver] Use libOption with tablegen.
ClosedPublic

Authored by JDevlieghere on Nov 19 2018, 1:43 AM.

Details

Summary

This changes the driver to use libOption for option parsing. This means that option are now generated from a tablegen file.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Nov 19 2018, 1:43 AM

Can you post a short snippet of old help and new help output?

Would be great to see old and new output like Zach suggested. Is there a reason we need to use TableGen? Other command line tools just use llvm:🆑:opt stuff. Seems a bit obtuse to use TableGen?

I actually don't find this too bad, and like Pavel mentioned earlier, it's something that we can fix in LLVM and improve everyone's output. Which is one of the nice things about sharing code like this.

davide added a subscriber: davide.Nov 26 2018, 8:57 AM

Would be great to see old and new output like Zach suggested. Is there a reason we need to use TableGen? Other command line tools just use llvm:🆑:opt stuff. Seems a bit obtuse to use TableGen?

I'm afraid this is not true. lld uses TableGen and that was a big success. See https://github.com/llvm-mirror/lld/blob/master/ELF/Options.td (considering that linkers have to handle all the annoying cmdline compatibility with gcc, e.g. options which take single or double underscores).

FWIW, I'm in favour of this change.

labath added a subscriber: labath.Nov 26 2018, 8:59 AM

There’s actually been a slow push away from cl::opt. It’s less flexible
and doesn’t support some things that the TableGen approach does.
Recently there’s been a few efforts to port existing tools onto TableGen
options from cl::opt.

I don’t think cl::opt is going away anytime soon so if it works I don’t
have a strong opinion, but it’s kinda nice to standardize on “the one
true method” if that’s the direction things are heading anyway

Another reason for using libOption is that it is also usable as a parser for the lldb command line, whereas cl::opt is definitely not (it uses global variables). And there's value in consistency between the lldb driver and the built-in command line.

There’s actually been a slow push away from cl::opt. It’s less flexible
and doesn’t support some things that the TableGen approach does.
Recently there’s been a few efforts to port existing tools onto TableGen
options from cl::opt.

I don’t think cl::opt is going away anytime soon so if it works I don’t
have a strong opinion, but it’s kinda nice to standardize on “the one
true method” if that’s the direction things are heading anyway

Another reason for using libOption is that it is also usable as a parser for the lldb command line, whereas cl::opt is definitely not (it uses global variables). And there's value in consistency between the lldb driver and the built-in command line.

This is true too. Although I believe libOption doesn't support subcommands, which would be required in order to use it for the interactive lldb command line, but again, there would be value in adding that to libOption outside of llvm (cl::opt supports it, so it's required in order to port some remaining llvm tools to libOption)

Another reason for using libOption is that it is also usable as a parser for the lldb command line, whereas cl::opt is definitely not (it uses global variables). And there's value in consistency between the lldb driver and the built-in command line.

This is true too. Although I believe libOption doesn't support subcommands, which would be required in order to use it for the interactive lldb command line, but again, there would be value in adding that to libOption outside of llvm (cl::opt supports it, so it's required in order to port some remaining llvm tools to libOption)

Adding subcommands is one way. Another would be to simply keep the existing subcommand-parsing code (which we already have, as getopt doesn't support that either), and just replace the getopt part with libOption.

clayborg added a comment.EditedNov 26 2018, 10:10 AM

the old usage:

Usage:

  lldb -h
  lldb -v [[--] <PROGRAM-ARG-1> [<PROGRAM_ARG-2> ...]]
  lldb -a <arch> -f <filename> [-c <filename>] [-s <filename>] [-o <none>] [-S <filename>] [-O <none>] [-k <none>] [-K <filename>] [-Q] [-b] [-e] [-x] [-X] [-l <script-language>] [-d] [-z <filename>] [[--] <PROGRAM-ARG-1> [<PROGRAM_ARG-2> ...]]
  lldb -n <process-name> -w [-s <filename>] [-o <none>] [-S <filename>] [-O <none>] [-k <none>] [-K <filename>] [-Q] [-b] [-e] [-x] [-X] [-l <script-language>] [-d] [-z <filename>]
  lldb -p <pid> [-s <filename>] [-o <none>] [-S <filename>] [-O <none>] [-k <none>] [-K <filename>] [-Q] [-b] [-e] [-x] [-X] [-l <script-language>] [-d] [-z <filename>]
  lldb -P
  lldb -r [<none>] -R <none>

Showed how all the options can be used together. It would be nice to group the options into the following groups:

  • general options (--arch, --version, --help, --file, --editor, --no-use-colors, -debug, --reproducer, --repl*)
  • scripting options (--python-path, --script-language)
  • command options (--no-lldbinit, --batch, --one-line*, --source*)
  • attach options (--attach-name, --attach-pid, --wait-for)

Add old usage.

Can we still group the options as mentioned in my previous comment?

tools/driver/Driver.cpp
943 ↗(On Diff #175300)

Get file base name of this so we don't show a full path to the tool if the user used the full path?

I know cl::opt had ways to group options and the table gen is more powerful so it must have this feature?

I didn't read the patch in detail yet, these are just meta-comments:

It looks like the libOption approach (or this implementation of it) is missing the notion of option groups. That's what you see in the first section of the lldb --help printout in the current version. That's not that big a deal for the lldb driver command, as it doesn't have all that many disparate groups of options. But Davide mentioned using this for the lldb command line as well, where we do use this to help auto-validate complex commands in a fair number of commands.

One thing that is missing from the SB API's was some way of defining the arguments and options for a command. Because of this, Python based commands can't take part in the completions features and auto-help generation that "real" commands had. If we had such a thing, then the driver could use the same mechanism to define itself as a command, and then just run itself at startup. That would centralize all the argument parsing that we do regardless of how it is backed.

  • Add grouping
  • Use basename for help
JDevlieghere marked an inline comment as done.Nov 26 2018, 1:03 PM

Can you attach new output with the grouping and extra usage?

Can you attach new output with the grouping and extra usage?

I updated the original snippet, see https://reviews.llvm.org/P8118

clayborg accepted this revision.Nov 26 2018, 1:18 PM
This revision is now accepted and ready to land.Nov 26 2018, 1:18 PM

I would wait to get another OK from anyone else just to be sure.

I would wait to get another OK from anyone else just to be sure.

Sounds good. Thanks Greg!

zturner accepted this revision.Nov 26 2018, 1:42 PM

Will land this tomorrow once we've figured out how to integrate tablegen with the Xcode project.

In the meantime I found some issues due to ordering of command options. Their relative order matters and we had tests relying on this. For example consider what happens when you say

lldb -o 'do one thing' -s 'source/some/file'

You'd expect that the one-line is executed before sourcing the file. This wasn't the case with the previous version of this patch and is now fixed.

You have to gather all the -O's and -S's and -Q's and add them to the list of code that gets sourced before the file is loaded in the order in which you find them. There can be more than one of each of these and they can be interspersed anywhere among the other command options. Ditto for the -o's, -s's and -q's (except they get run after the file). I don't know how complicated a set of tests I wrote when I originally added these, so you might want to check that all these cases are covered.

labath added inline comments.Nov 27 2018, 6:54 AM
tools/driver/Driver.cpp
876–888 ↗(On Diff #175377)

I am not entirely thrilled by the hard-coding of the option combinations here. It sounds like the kind of thing that will invariably get out of sync (I think it would be better to just not have it). Instead of trying to exhaustively list all possible option combinations (which I generally find too long to make sense of), and still getting it wrong (Why is there a [[--] <PROGRAM-ARG-1> [<PROGRAM_ARG-2> ...]] after -v in the second option form?), maybe it would be better to just output a short prose here explaining the general principle. Maybe something like "LLDB can be started in several modes. Passing an executable as a positional arguments prepares lldb to debug the given executable. Using one of the attach options causes lldb to immediately attach to the given process. Command options can be combined with either mode and cause lldb to run the specified commands before starting the interactive shell. Using --repl starts lldb in REPL mode. etc."

However, if everyone is happy with the current approach then I won't stand in the way..

JDevlieghere edited the summary of this revision. (Show Details)
  • Add lit test for command options.
  • Change usage with prose.

I would be happy if we just have an EXAMPLES section much like many of the man pages for built in shell commands where we show a few examples.

I would like to see setting up a target a program with arguments that might conflict with the argument parser like:

% lldb --arch x86_64 /path/to/program/a.out -- --arch arvm7

The first arch is for lldb, and any options past the -- are for the inferior program

Attaching examples, loading a core file, printing python path, maybe specifying some lldb commands with -o and -O to show how those work.

JDevlieghere updated this revision to Diff 175526.EditedNov 27 2018, 10:25 AM
  • Add EXAMPLES section
EXAMPLES:
  The debugger can be started in several modes.

  Passing an executable as a positional argument prepares lldb to debug the
  given executable. Arguments passed after -- are considered arguments to the
  debugged executable.

    lldb --arch x86_64 /path/to/program -- --arch arvm7

  Passing one of the attach options causes lldb to immediately attach to the
  given process.

    lldb -p <pid>
    lldb -n <process-name>

  Passing --repl starts lldb in REPL mode.

    lldb -r

  Passing --core causes lldb to debug the core file.

    lldb -c /path/to/core

  Command options can be combined with either mode and cause lldb to run the
  specified commands before or after events, like loading the file or crashing,
  in the order provided on the command line.

    lldb -O 'settings set stop-disassembly-count 20' -o 'run' -o 'bt'
    lldb -S /source/before/file -s /source/after/file
    lldb -K /source/before/crash -k /source/after/crash
This revision was automatically updated to reflect the committed changes.
lemo added a subscriber: lemo.Nov 28 2018, 4:09 PM

I noticed a small problem, this change breaks "lldb -c <corefile>". The inline comment explains the root cause.

Thanks

lldb/trunk/tools/driver/Driver.cpp
310

there's a small bug in here: optarg is the global definition, I assume the intention was to use the getValue() instead. as is, it breaks "lldb -c <corefile>"

it would be nice to get rid the global optarg as well since it masks these kind of mistakes.