Page MenuHomePhabricator

[lldb] Let table gen create command option initializers.
ClosedPublic

Authored by teemperor on Jul 8 2019, 1:24 PM.

Details

Summary

We currently have man large arrays containing initializers for our command options.
These tables are tricky maintain as we don't have any good place to check them for consistency and
it's also hard to read (nullptr, {}, 0 is not very descriptive).

This patch fixes this by letting table gen generate those tables. This way we can have a more readable
syntax for this (especially for all the default arguments) and we can let TableCheck check them
for consistency (e.g. an option with an optional argument can't have eArgTypeNone, naming of flags', etc.).

Also refactoring the related data structures can now be done without changing the hundred of option initializers.

For example, this line:

{LLDB_OPT_SET_ALL, false, "hide-aliases",         'a', OptionParser::eNoArgument, nullptr, {}, 0, eArgTypeNone, "Hide aliases in the command list."},

becomes this:

def hide_aliases : Option<"hide-aliases", "a">, Desc<"Hide aliases in the command list.">;

For now I just moved a few initializers to the new format to demonstrate the change. I'll slowly migrate the other
option initializers tables in separate patches.

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor created this revision.Jul 8 2019, 1:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2019, 1:24 PM
teemperor marked an inline comment as done.Jul 8 2019, 1:25 PM
teemperor added inline comments.
lldb/source/Commands/OptionsBase.td
11 ↗(On Diff #208487)

I'm open to renaming this if anyone has a better name for this.

labath added a subscriber: labath.Jul 8 2019, 1:55 PM

I like this, though I am not really familiar with details of implementing a tablegen backend. This should make it easier to migrate to the llvmOption library, once we arrive to that point.

lldb/utils/TableGen/LLDBOptionDefEmitter.cpp
26 ↗(On Diff #208487)

llvm::StringMap

lldb/utils/TableGen/LLDBTableGen.cpp
31 ↗(On Diff #208487)

use static instead of anonymous namespace.

teemperor updated this revision to Diff 208506.Jul 8 2019, 2:21 PM
  • Added comment that std::map is also used to make sure our commands are sorted.
  • anonymous namespace -> static
teemperor marked 3 inline comments as done.Jul 8 2019, 2:23 PM
teemperor added inline comments.
lldb/utils/TableGen/LLDBOptionDefEmitter.cpp
26 ↗(On Diff #208487)

So the map is also supposed to make sure that our commands in the options file are (deterministically) sorted, so that's why it's just a map. I documented that now.

xiaobai added a subscriber: xiaobai.Jul 8 2019, 2:59 PM

This is awesome, thanks for doing this! I was also thinking about doing something like this at some point as well. :)

lldb/cmake/modules/AddLLDB.cmake
17 ↗(On Diff #208506)

clang_tablegen -> lldb_tablegen

lldb/source/Commands/BreakpointList.td
1 ↗(On Diff #208506)

Is BreakpointList.td used? I noticed hide_aliases is also implemented in Options.td too.

JDevlieghere added inline comments.Jul 8 2019, 3:01 PM
lldb/cmake/modules/AddLLDB.cmake
16 ↗(On Diff #208506)

nit: remove the spaces to be consistent.

lldb/utils/TableGen/LLDBOptionDefEmitter.cpp
57 ↗(On Diff #208506)

s/grous/groups/

68 ↗(On Diff #208506)

ternary operator?

lldb/utils/TableGen/LLDBTableGenBackends.h
1 ↗(On Diff #208506)

Use the new license header.

teemperor updated this revision to Diff 208539.Jul 8 2019, 4:43 PM
teemperor marked 3 inline comments as done.
  • Addressed feedback
teemperor marked 5 inline comments as done.Jul 8 2019, 4:45 PM

Thanks for the feedback!

lldb/source/Commands/BreakpointList.td
1 ↗(On Diff #208506)

Yeah, that was just me accidentally adding some unneeded file I used for prototyping. Thanks!

jingham added a subscriber: jingham.Jul 8 2019, 4:50 PM

Do you know how you are going to do enum option values?

teemperor marked an inline comment as done.Jul 8 2019, 5:03 PM

Do you know how you are going to do enum option values?

We could implement the enums in our table gen file, but then we would have the enums duplicated in code and table gen (unless we also table gen the enum definition). At the moment I'm just using plain strings in table gen which then get type checked by Clang during compilation. The *.inc file should have enough comments in it that it should be trivial to find out in what option definition you made a typo.

But I'm open to suggestions how to make this more user-friendly if people encounter problems with this.

JDevlieghere accepted this revision.Jul 9 2019, 4:40 PM

Thanks Raphael, great job! LGTM

This revision is now accepted and ready to land.Jul 9 2019, 4:40 PM
teemperor updated this revision to Diff 209119.Jul 10 2019, 9:00 PM
  • Better formatting and comments for generated Options.inc
  • Added support and example for arguments that have enum values.
  • Fixed some typos in the Options.td definitions that caused some tests to fail.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2019, 8:32 AM