This is an archive of the discontinued LLVM Phabricator instance.

Make OptionDefinition structure store a StringRef
ClosedPublic

Authored by zturner on Dec 14 2016, 3:33 PM.

Details

Summary

The major blocker to this until now has been that we can't have global constructors, no matter how trivial.

Recently LLVM obtained the StringLiteral class which addresses this. This class has a constexpr constructor, so as long as a global StringRef (or class/struct containing a StringRef is declared constexpr, the StringRef can be initialized with a StringLiteral and not invoke a global constructor. This probably sounds confusing, but in practice it's simple. It looks like this:

// ok
constexpr StringRef S = llvm::StringLiteral("foo");

// ok
constexpr StringLiteral T("foo");

// not ok, global constructor invoked.
StringLiteral U("foo");

struct Foo {
  constexpr Foo(StringRef S) : S(S) {}
  StringRef S;
};

// ok
constexpr Foo F(llvm::StringLiteral("foo"));

Admittedly, it's pretty verbose to type out StringLiteral everywhere. To get around this, I alias it to L in the individual TUs where we use it. This way we can write:

using L = llvm::StringLiteral;
static constexpr OptionDefinition[] g_defs = { ..., ..., L("opt"), ..., ..., L("help text") };

Again, note that no global constructor is invoked here, even though this is an array.

The advantage of doing all of this is that we can now have access to all the wonderful member functions of StringRef. Upon fixing up all the compilation failures (there were very few however) triggered by the change, we can already see some code being simplified, and in the future I would like to extend this to other classes like RegisterInfo etc as well.

Note that this patch is incomplete as is. Before submitting, I would need to go change EVERY global definition of OptionDefinition to be marked constexpr, otherwise a global constructor would be invoked. I intentionally did it in only 2 places here, just to illustrate the idea, before changing the code everywhere.

Diff Detail

Event Timeline

zturner updated this revision to Diff 81487.Dec 14 2016, 3:33 PM
zturner retitled this revision from to Make OptionDefinition structure store a StringRef.
zturner updated this object.
zturner added reviewers: clayborg, labath.
zturner added a subscriber: lldb-commits.
clayborg edited edge metadata.Dec 14 2016, 4:25 PM

There seems to be a bunch of places that we are passing a string literal to functions that take a StringRef. Can we pass L("--") instead of "--" to functions that take a string ref and have it be more efficient so a StringRef isn't implicitly constructed for us each time? See inlined comments for places.

lldb/source/Interpreter/Args.cpp
954 ↗(On Diff #81487)

Will this line cause a StringRef constructor to be called on "-{0}"? If so, is there a way to avoid this with some constexpr magic?

956 ↗(On Diff #81487)

Ditto

lldb/source/Interpreter/Options.cpp
337 ↗(On Diff #81487)

Will this line cause a StringRef constructor to be called on "--"?

701 ↗(On Diff #81487)

Will this line cause a StringRef constructor to be called on "--"?

719 ↗(On Diff #81487)

Will this line cause a StringRef constructor to be called on "--"?

728 ↗(On Diff #81487)

Do we still need std::string here for full_name? We might be able to do smarter things with StringRef for all uses of full_name below, including the matches.GetStringAtIndex() by seeing if the string at index starts with "--", and then just comparing the remainder to "def.long_option"?

834 ↗(On Diff #81487)

Will this line cause a StringRef constructor to be called on "shlib"?

In response to all the questions about "Will a StringRef constructor be called on XXX", the answer is usually yes, but the constructor will be inlined and invoke __builtin_strlen (which is constexpr on GCC and Clang) when used on a string literal. In other words, there is zero overhead.

So the code generated for S.startswith("--") will end up just being (2 >= S.Length) && (::memcmp(S.Data, "--", 2) == 0).

The only time we have to worry about constexprness and make sure we use StringLiteral is with static variables (global or local). With static variables, the compiler has to decide between static link-time initialization or runtime initialization, and only if you declare the variable as constexpr will it choose link-time initialization. So the generated code for these two might look something like this:

// No generated code, S is initialized at link-time.
constexpr StringLiteral S("foo");   


// mov <addr-of-T.Data>, <addr-of-global-string>
// mov <addr-of-T.Length>, 3
StringLiteral T("foo");
zturner added inline comments.Dec 14 2016, 4:47 PM
lldb/source/Interpreter/Options.cpp
728 ↗(On Diff #81487)

Yes, I tried that at first, but soon after noticed that matches is an output parameter, so we would have to fix up the caller to stop making assumptions that the -- is tacked onto the beginning. Certainly doable, but it has potential for growing into a large CL depending on how far down the rabbit hole we'd have to go to fix everything.

labath edited edge metadata.Dec 15 2016, 2:22 AM

Looks reasonable. I look forward to using StringRef in more places.

This revision was automatically updated to reflect the committed changes.
zturner reopened this revision.Dec 15 2016, 11:17 AM

My bad, this revision was not actually closed, I attached the wrong diff to an unrelated commit. I will need to re-upload this one.

zturner updated this revision to Diff 81625.Dec 15 2016, 11:18 AM
zturner edited edge metadata.
zturner removed rL LLVM as the repository for this revision.

Re-upload the correct diff.

Greg, did the comments about implicit construction of a StringRef from a char literal being zero overhead make sense? If so, are there any other concerns?

clayborg accepted this revision.Dec 16 2016, 10:43 AM
clayborg edited edge metadata.

Ok, as long as the StringRef constructors are quick and efficient and not running strlen() each time I am good.

This revision is now accepted and ready to land.Dec 16 2016, 10:43 AM

Yea as I mentioned this whole plan might be killed by a blocker I ran into last night. I'm still trying to figure out if there's a workaround.

labath resigned from this revision.May 8 2018, 8:46 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 6:36 AM