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

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

Ditto

lldb/source/Interpreter/Options.cpp

337

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

701

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

719

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

728

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

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

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.

lldb/include/lldb/Interpreter/Options.h

public:
 
   void OutputFormattedUsageText(Stream &strm,
                                 const OptionDefinition &option_def,
                                 uint32_t output_max_columns);
 
   void GenerateOptionUsage(Stream &strm, CommandObject *cmd,
                            uint32_t screen_width);
 
-  bool SupportsLongOption(const char *long_option);
+  bool SupportsLongOption(llvm::StringRef long_option);
 
   // The following two pure virtual functions must be defined by every
   // class that inherits from this class.
 
   virtual llvm::ArrayRef<OptionDefinition> GetDefinitions() {
     return llvm::ArrayRef<OptionDefinition>();
   }
 

lldb/include/lldb/lldb-private-types.h

struct OptionValidator {
   virtual ~OptionValidator() {}
   virtual bool IsValid(Platform &platform,
                        const ExecutionContext &target) const = 0;
   virtual const char *ShortConditionString() const = 0;
   virtual const char *LongConditionString() const = 0;
 };
 
 struct OptionDefinition {
-  uint32_t usage_mask; // Used to mark options that can be used together.  If (1
-                       // << n & usage_mask) != 0
-                       // then this option belongs to option set n.
-  bool required;       // This option is required (in the current usage level)
-  const char *long_option; // Full name for this option.
-  int short_option;        // Single character for this option.
-  int option_has_arg; // no_argument, required_argument or optional_argument
-  OptionValidator *validator; // If non-NULL, option is valid iff
-                              // |validator->IsValid()|, otherwise always valid.
-  OptionEnumValueElement *enum_values; // If non-NULL an array of enum values.
-  uint32_t completion_type; // Cookie the option class can use to do define the
-                            // argument completion.
-  lldb::CommandArgumentType argument_type; // Type of argument this option takes
-  const char *usage_text; // Full text explaining what this options does and
-                          // what (if any) argument to
-                          // pass it.
+  OptionDefinition() {}
+
+  constexpr OptionDefinition(uint32_t mask, bool required,
+                             llvm::StringRef long_opt, int short_opt,
+                             int has_arg, OptionValidator *validator,
+                             OptionEnumValueElement *enum_values,
+                             uint32_t completion_type,
+                             lldb::CommandArgumentType arg_type,
+                             llvm::StringRef usage)
+      : usage_mask(mask), required(required), long_option(long_opt),
+        short_option(short_opt), option_has_arg(has_arg), validator(validator),
+        enum_values(enum_values), completion_type(completion_type),
+        argument_type(arg_type), usage_text(usage_text) {}
+
+  // Used to mark options that can be used together.  If
+  // (1 << n & usage_mask) != 0 then this option belongs to option set n.
+  uint32_t usage_mask = 0;
+
+  // This option is required (in the current usage level)
+  bool required = false;
+
+  // Full name for this option.
+  llvm::StringRef long_option;
+
+  // Single character for this option.
+  int short_option = 0;
+
+  // no_argument, required_argument or optional_argument
+  int option_has_arg = 0;
+
+  // If non-NULL, option is valid iff |validator->IsValid()|, otherwise always
+  // valid.
+  OptionValidator *validator = nullptr;
+
+  // If non-NULL an array of enum values.
+  OptionEnumValueElement *enum_values = nullptr;
+
+  // Cookie the option class can use to do define the argument completion.
+  uint32_t completion_type = 0;
+
+  // Type of argument this option takes
+  lldb::CommandArgumentType argument_type = lldb::eArgTypeNone;
+
+  // Full text explaining what this options does and what argument(s) it takes.
+  llvm::StringRef usage_text;
 };
 
 typedef struct type128 { uint64_t x[2]; } type128;
 typedef struct type256 { uint64_t x[4]; } type256;
 
 } // namespace lldb_private
 
 #endif // #if defined(__cplusplus)
 
 #endif // liblldb_lldb_private_types_h_

lldb/source/Commands/CommandObjectSettings.cpp

 
 using namespace lldb;
 using namespace lldb_private;
 
 //-------------------------------------------------------------------------
 // CommandObjectSettingsSet
 //-------------------------------------------------------------------------
 
-static OptionDefinition g_settings_set_options[] = {
+using L = llvm::StringLiteral;
+
+static constexpr OptionDefinition g_settings_set_options[] = {
     // clang-format off
-  { LLDB_OPT_SET_2, false, "global", 'g', OptionParser::eNoArgument, nullptr, nullptr, 0, eArgTypeNone, "Apply the new value to the global default value." }
+  { LLDB_OPT_SET_2, false, L("global"), 'g', OptionParser::eNoArgument, nullptr, nullptr, 0, eArgTypeNone, L("Apply the new value to the global default value.") }
     // clang-format on
 };
 
 class CommandObjectSettingsSet : public CommandObjectRaw {
 public:
   CommandObjectSettingsSet(CommandInterpreter &interpreter)
       : CommandObjectRaw(interpreter, "settings set",
                          "Set the value of the specified debugger setting."),

lldb/source/Host/common/OptionParser.cpp

 //===-- source/Host/common/OptionParser.cpp ---------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Host/HostGetOpt.h"
 #include "lldb/lldb-private-types.h"
 
+#include "llvm/Support/Allocator.h"
+#include "llvm/Support/StringSaver.h"
+
 #include <vector>
 
 using namespace lldb_private;
 
 void OptionParser::Prepare(std::unique_lock<std::mutex> &lock) {
   static std::mutex g_mutex;
   lock = std::unique_lock<std::mutex>(g_mutex);
 #ifdef __GLIBC__
   optind = 0;
 #else
   optreset = 1;
   optind = 1;
 #endif
 }
 
 void OptionParser::EnableError(bool error) { opterr = error ? 1 : 0; }
 
 int OptionParser::Parse(int argc, char *const argv[], llvm::StringRef optstring,
                         const Option *longopts, int *longindex) {
   std::vector<option> opts;
+  llvm::BumpPtrAllocator allocator;
+  llvm::StringSaver saver(allocator);
+
   while (longopts->definition != nullptr) {
     option opt;
     opt.flag = longopts->flag;
     opt.val = longopts->val;
-    opt.name = longopts->definition->long_option;
+    opt.name = saver.save(longopts->definition->long_option).data();
     opt.has_arg = longopts->definition->option_has_arg;
     opts.push_back(opt);
     ++longopts;
   }
   opts.push_back(option());
   std::string opt_cstr = optstring;
   return getopt_long_only(argc, argv, opt_cstr.c_str(), &opts[0], longindex);
 }

lldb/source/Interpreter/Args.cpp

 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Interpreter/Options.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Target.h"
 
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 
+#include "llvm/Support/FormatVariadic.h"
+
 using namespace lldb;
 using namespace lldb_private;
 
 
 // A helper function for argument parsing.
 // Parses the initial part of the first argument using normal double quote
 // rules:
 // backslash escapes the double quote and itself. The parsed string is appended
bool Args::ContainsEnvironmentVariable(llvm::StringRef env_var_name,
   }
 
   // We didn't find a match.
   return false;
 }
 
 size_t Args::FindArgumentIndexForOption(Option *long_options,
                                         int long_options_index) const {
-  char short_buffer[3];
-  char long_buffer[255];
-  ::snprintf(short_buffer, sizeof(short_buffer), "-%c",
-             long_options[long_options_index].val);
-  ::snprintf(long_buffer, sizeof(long_buffer), "--%s",
-             long_options[long_options_index].definition->long_option);
+  llvm::SmallString<3> short_buffer =
+      llvm::formatv("-{0}", (char)long_options[long_options_index].val);
+  llvm::SmallString<255> long_buffer = llvm::formatv(
+      "--{0}", long_options[long_options_index].definition->long_option);
 
   for (auto entry : llvm::enumerate(m_entries)) {
     if (entry.Value.ref.startswith(short_buffer) ||
         entry.Value.ref.startswith(long_buffer))
       return entry.Index;
   }
 
   return size_t(-1);

lldb/source/Interpreter/OptionGroupOutputFile.cpp

 using namespace lldb;
 using namespace lldb_private;
 
 OptionGroupOutputFile::OptionGroupOutputFile()
     : m_file(), m_append(false, false) {}
 
 OptionGroupOutputFile::~OptionGroupOutputFile() {}
 
-static const uint32_t SHORT_OPTION_APND = 0x61706e64; // 'apnd'
+static const int SHORT_OPTION_APND = 0x61706e64; // 'apnd'
 
-static OptionDefinition g_option_table[] = {
-    {LLDB_OPT_SET_1, false, "outfile", 'o', OptionParser::eRequiredArgument,
+using L = llvm::StringLiteral;
+
+static constexpr OptionDefinition g_option_table[] = {
+    {LLDB_OPT_SET_1, false, L("outfile"), 'o', OptionParser::eRequiredArgument,
      nullptr, nullptr, 0, eArgTypeFilename,
-     "Specify a path for capturing command output."},
-    {LLDB_OPT_SET_1, false, "append-outfile", SHORT_OPTION_APND,
+     L("Specify a path for capturing command output.")},
+    {LLDB_OPT_SET_1, false, L("append-outfile"), SHORT_OPTION_APND,
      OptionParser::eNoArgument, nullptr, nullptr, 0, eArgTypeNone,
-     "Append to the file specified with '--outfile <path>'."},
+     L("Append to the file specified with '--outfile <path>'.")},
 };
 
 llvm::ArrayRef<OptionDefinition> OptionGroupOutputFile::GetDefinitions() {
   return llvm::makeArrayRef(g_option_table);
 }
 
 Error OptionGroupOutputFile::SetOptionValue(
     uint32_t option_idx, llvm::StringRef option_arg,

lldb/source/Interpreter/Options.cpp

while (end < final_end) {
       assert(start + sub_len <= final_end);
       strm.Write(actual_text.c_str() + start, sub_len);
       start = end + 1;
     }
     strm.EOL();
   }
 }
 
-bool Options::SupportsLongOption(const char *long_option) {
-  if (!long_option || !long_option[0])
+bool Options::SupportsLongOption(llvm::StringRef long_option) {
+  long_option.consume_front("--");
+  if (long_option.empty())
     return false;
 
-  auto opt_defs = GetDefinitions();
-  if (opt_defs.empty())
-    return false;
-
-  const char *long_option_name = long_option;
-  if (long_option[0] == '-' && long_option[1] == '-')
-    long_option_name += 2;
-
-  for (auto &def : opt_defs) {
-    if (!def.long_option)
-      continue;
-
-    if (strcmp(def.long_option, long_option_name) == 0)
-      return true;
-  }
-
-  return false;
+  return llvm::any_of(GetDefinitions(), [=](const OptionDefinition &D) {
+    return D.long_option == long_option;
+  });
 }
 
 enum OptionDisplayType {
   eDisplayBestOption,
   eDisplayShortOption,
   eDisplayLongOption
 };
 
for (auto pos : options_seen) {
         // Short option is not printable, just print long option
         PrintOption(opt_defs[i], eDisplayLongOption, nullptr, nullptr, false,
                     strm);
       }
       strm.EOL();
 
       strm.IndentMore(5);
 
-      if (opt_defs[i].usage_text)
+      if (!opt_defs[i].usage_text.empty())
         OutputFormattedUsageText(strm, opt_defs[i], screen_width);
-      if (opt_defs[i].enum_values != nullptr) {
+
+      if (opt_defs[i].enum_values == nullptr) {
         strm.Indent();
         strm.Printf("Values: ");
         for (int k = 0; opt_defs[i].enum_values[k].string_value != nullptr;
              k++) {
           if (k == 0)
             strm.Printf("%s", opt_defs[i].enum_values[k].string_value);
           else
             strm.Printf(" | %s", opt_defs[i].enum_values[k].string_value);
bool Options::HandleOptionCompletion(
   word_complete = true;
 
   // For now we just scan the completions to see if the cursor position is in
   // an option or its argument.  Otherwise we'll call HandleArgumentCompletion.
   // In the future we can use completion to validate options as well if we want.
 
   auto opt_defs = GetDefinitions();
 
-  std::string cur_opt_std_str(input.GetArgumentAtIndex(cursor_index));
-  cur_opt_std_str.erase(char_pos);
-  const char *cur_opt_str = cur_opt_std_str.c_str();
-
   for (size_t i = 0; i < opt_element_vector.size(); i++) {
     int opt_pos = opt_element_vector[i].opt_pos;
     int opt_arg_pos = opt_element_vector[i].opt_arg_pos;
     int opt_defs_index = opt_element_vector[i].opt_defs_index;
     if (opt_pos == cursor_index) {
       // We're completing the option itself.
 
       if (opt_defs_index == OptionArgElement::eBareDash) {
if (opt_pos == cursor_index) {
             continue;
 
           full_name.erase(full_name.begin() + 2, full_name.end());
           full_name.append(def.long_option);
           matches.AppendString(full_name.c_str());
         }
         return true;
       } else if (opt_defs_index != OptionArgElement::eUnrecognizedArg) {
+        auto cur_opt_str = input[cursor_index].ref.take_front(char_pos);
+
         // We recognized it, if it an incomplete long option, complete it anyway
-        // (getopt_long_only is
-        // happy with shortest unique string, but it's still a nice thing to
-        // do.)  Otherwise return
-        // The string so the upper level code will know this is a full match and
-        // add the " ".
-        if (cur_opt_str && strlen(cur_opt_str) > 2 && cur_opt_str[0] == '-' &&
-            cur_opt_str[1] == '-' &&
-            strcmp(opt_defs[opt_defs_index].long_option, cur_opt_str) != 0) {
+        // (getopt_long_only is happy with shortest unique string, but it's
+        // still a nice thing to do.)  Otherwise return the string so the upper
+        // level code will know this is a full match and add the " ".
+        if (cur_opt_str.startswith("--") &&
+            cur_opt_str.drop_front(2) != opt_defs[opt_defs_index].long_option) {
           std::string full_name("--");
           full_name.append(opt_defs[opt_defs_index].long_option);
-          matches.AppendString(full_name.c_str());
+          matches.AppendString(full_name);
           return true;
         } else {
           matches.AppendString(input.GetArgumentAtIndex(cursor_index));
           return true;
         }
       } else {
         // FIXME - not handling wrong options yet:
         // Check to see if they are writing a long option & complete it.
         // I think we will only get in here if the long option table has two
-        // elements
-        // that are not unique up to this point.  getopt_long_only does shortest
-        // unique match
-        // for long options already.
+        // elements that are not unique up to this point.  getopt_long_only
+        // does shortest unique match for long options already.
+        auto cur_opt_str = input[cursor_index].ref.take_front(char_pos);
 
-        if (cur_opt_str && strlen(cur_opt_str) > 2 && cur_opt_str[0] == '-' &&
-            cur_opt_str[1] == '-') {
-          for (auto &def : opt_defs) {
-            if (!def.long_option)
-              continue;
+        if (!cur_opt_str.consume_front("--"))
+          return true;
+
+        auto range = llvm::make_filter_range(
+            opt_defs, [cur_opt_str](const OptionDefinition &D) {
+              return D.long_option == cur_opt_str;
+            });
 
-            if (strstr(def.long_option, cur_opt_str + 2) == def.long_option) {
-              std::string full_name("--");
-              full_name.append(def.long_option);
-              // The options definitions table has duplicates because of the
-              // way the grouping information is stored, so only add once.
-              bool duplicate = false;
-              for (size_t k = 0; k < matches.GetSize(); k++) {
-                if (matches.GetStringAtIndex(k) == full_name) {
-                  duplicate = true;
-                  break;
-                }
-              }
-              if (!duplicate)
-                matches.AppendString(full_name.c_str());
-            }
-          }
+        for (auto &def : range) {
+          // The options definitions table has duplicates because of the way the
+          // grouping information is stored, so only add once.
+            matches.AppendString(full_name);
         }
         return true;
       }
 
     } else if (opt_arg_pos == cursor_index) {
       // Okay the cursor is on the completion of an argument.
       // See if it has a completion, otherwise return no matches.
 
for (size_t i = 0; i < opt_element_vector.size(); i++) {
 
       // trying to use <0 indices will definitely cause problems
       if (cur_defs_index == OptionArgElement::eUnrecognizedArg ||
           cur_defs_index == OptionArgElement::eBareDash ||
           cur_defs_index == OptionArgElement::eBareDoubleDash)
         continue;
 
       int cur_arg_pos = opt_element_vector[i].opt_arg_pos;
-      const char *cur_opt_name = opt_defs[cur_defs_index].long_option;
+      auto cur_opt_name = opt_defs[cur_defs_index].long_option;
 
       // If this is the "shlib" option and there was an argument provided,
       // restrict it to that shared library.
-      if (cur_opt_name && strcmp(cur_opt_name, "shlib") == 0 &&
-          cur_arg_pos != -1) {
-        const char *module_name = input.GetArgumentAtIndex(cur_arg_pos);
-        if (module_name) {
+      if (cur_opt_name == "shlib" && cur_arg_pos != -1) {
+        auto module_name = input[cur_arg_pos].ref;
+        if (!module_name.empty()) {
           FileSpec module_spec(module_name, false);
           lldb::TargetSP target_sp =
               interpreter.GetDebugger().GetSelectedTarget();
           // Search filters require a target...
           if (target_sp)
             filter_ap.reset(new SearchFilterByModule(target_sp, module_spec));
         }
         break;