Make OptionDefinition structure store a StringRef

[zturner]

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.

[clayborg]

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"?

[zturner]

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]

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]

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

[zturner]

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]

Re-upload the correct diff.

[zturner]

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]

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

[zturner]

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.

llvm/include/llvm/ADT/StringRef.h

StringRef trim(StringRef Chars = " \t\n\v\f\r") const {
       return ltrim(Chars).rtrim(Chars);
     }
 
     /// @}
   };
 
   /// A wrapper around a string literal that serves as a proxy for constructing
   /// global tables of StringRefs with the length computed at compile time.
-  /// Using this class with a non-literal char array is considered undefined
-  /// behavior.  To prevent this, it is recommended that StringLiteral *only*
-  /// be used in a constexpr context, as such:
+  /// In order to avoid the invocation of a global constructor, StringLiteral
+  /// should *only* be used in a constexpr context, as such:
   ///
   /// constexpr StringLiteral S("test");
   ///
-  /// Note: There is a subtle behavioral difference in the constructor of
-  /// StringRef and StringLiteral, as illustrated below:
-  ///
-  /// constexpr StringLiteral S("a\0b");  // S.size() == 3
-  /// StringRef S("a\0b");  // S.size() == 1
-  ///
   class StringLiteral : public StringRef {
   public:
     template <size_t N>
-    constexpr StringLiteral(const char (&Str)[N]) : StringRef(Str, N - 1) {}
+    constexpr StringLiteral(const char (&Str)[N])
+#if __has_attribute(enable_if)
+        __attribute((enable_if(__builtin_strlen(Str) == N - 1,
+                               "invalid string literal")))
+#endif
+        : StringRef(Str, N - 1) {
+    }
   };
 
   /// @name StringRef Comparison Operators
   /// @{
 
   LLVM_ATTRIBUTE_ALWAYS_INLINE
   inline bool operator==(StringRef LHS, StringRef RHS) {
     return LHS.equals(RHS);
   }
 
   LLVM_ATTRIBUTE_ALWAYS_INLINE
-  inline bool operator!=(StringRef LHS, StringRef RHS) {
-    return !(LHS == RHS);
-  }
+  inline bool operator!=(StringRef LHS, StringRef RHS) { return !(LHS == RHS); }
 
   inline bool operator<(StringRef LHS, StringRef RHS) {
     return LHS.compare(RHS) == -1;
   }
 
   inline bool operator<=(StringRef LHS, StringRef RHS) {
     return LHS.compare(RHS) != 1;
   }