fix lldb crash by changing const char * with StringRef

[kevgs]

Summary

Description: http://lists.llvm.org/pipermail/llvm-dev/2015-November/092429.html

[dblaikie]

What's the actual bug in LLDB's usage & how does this address it? Is it
testable in LLVM? (unit tests?)

[kevgs]

Hello.

Here is the description of a crash http://lists.llvm.org/pipermail/llvm-dev/2015-November/092429.html

I thinks its testable. Option::ArgStr, Option::HelpStr, Option::ValueStr must not be nullptr. We could make them private and put assertions in setter function. And test it. StringRef can not be created from nullptr string.

16.11.2015, 20:38, "David Blaikie" <[e-mail]>:

What's the actual bug in LLDB's usage & how does this address it? Is it testable in LLVM? (unit tests?)

llvm/trunk/include/llvm/Support/CommandLine.h

class Option {
   unsigned Value : 2;
   unsigned HiddenFlag : 2; // enum OptionHidden
   unsigned Formatting : 2; // enum FormattingFlags
   unsigned Misc : 3;
   unsigned Position;       // Position of last occurrence of the option
   unsigned AdditionalVals; // Greater than 0 for multi-valued option.
 
 public:
-  const char *ArgStr;   // The argument string itself (ex: "help", "o")
-  const char *HelpStr;  // The descriptive text message for -help
-  const char *ValueStr; // String describing what the value of this option is
+  StringRef ArgStr;   // The argument string itself (ex: "help", "o")
+  StringRef HelpStr;  // The descriptive text message for -help
+  StringRef ValueStr; // String describing what the value of this option is
   OptionCategory *Category; // The Category this option belongs to
   bool FullyInitialized;    // Has addArguemnt been called?
 
   inline enum NumOccurrencesFlag getNumOccurrencesFlag() const {
     return (enum NumOccurrencesFlag)Occurrences;
   }
   inline enum ValueExpected getValueExpectedFlag() const {
     return Value ? ((enum ValueExpected)Value) : getValueExpectedFlagDefault();
   }
   inline enum OptionHidden getOptionHiddenFlag() const {
     return (enum OptionHidden)HiddenFlag;
   }
   inline enum FormattingFlags getFormattingFlag() const {
     return (enum FormattingFlags)Formatting;
   }
   inline unsigned getMiscFlags() const { return Misc; }
   inline unsigned getPosition() const { return Position; }
   inline unsigned getNumAdditionalVals() const { return AdditionalVals; }
 
   // hasArgStr - Return true if the argstr != ""
-  bool hasArgStr() const { return ArgStr[0] != 0; }
+  bool hasArgStr() const { return !ArgStr.empty(); }
 
   //-------------------------------------------------------------------------===
   // Accessor functions set by OptionModifiers
   //
-  void setArgStr(const char *S);
-  void setDescription(const char *S) { HelpStr = S; }
-  void setValueStr(const char *S) { ValueStr = S; }
+  void setArgStr(StringRef S);
+  void setDescription(StringRef S) { HelpStr = S; }
+  void setValueStr(StringRef S) { ValueStr = S; }
   void setNumOccurrencesFlag(enum NumOccurrencesFlag Val) { Occurrences = Val; }
   void setValueExpectedFlag(enum ValueExpected Val) { Value = Val; }
   void setHiddenFlag(enum OptionHidden Val) { HiddenFlag = Val; }
   void setFormattingFlag(enum FormattingFlags V) { Formatting = V; }
   void setMiscFlag(enum MiscFlags M) { Misc |= M; }
   void setPosition(unsigned pos) { Position = pos; }
   void setCategory(OptionCategory &C) { Category = &C; }
 
public:
 
   // printOptionInfo - Print out information about this option.  The
   // to-be-maintained width is specified.
   //
   virtual void printOptionInfo(size_t GlobalWidth) const = 0;
 
   virtual void printOptionValue(size_t GlobalWidth, bool Force) const = 0;
 
-  virtual void getExtraOptionNames(SmallVectorImpl<const char *> &) {}
+  virtual void getExtraOptionNames(SmallVectorImpl<StringRef> &) {}
 
   // addOccurrence - Wrapper around handleOccurrence that enforces Flags.
   //
   virtual bool addOccurrence(unsigned pos, StringRef ArgName, StringRef Value,
                              bool MultiArg = false);
 
   // Prints option name followed by message.  Always returns true.
   bool error(const Twine &Message, StringRef ArgName = StringRef());
public:
   void printOptionDiff(const Option &O, const AnyOptionValue &V,
                        const AnyOptionValue &Default,
                        size_t GlobalWidth) const {
     printGenericOptionDiff(O, V, Default, GlobalWidth);
   }
 
   void initialize() {}
 
-  void getExtraOptionNames(SmallVectorImpl<const char *> &OptionNames) {
+  void getExtraOptionNames(SmallVectorImpl<StringRef> &OptionNames) {
     // If there has been no argstr specified, that means that we need to add an
     // argument for every possible option.  This ensures that our options are
     // vectored to us.
     if (!Owner.hasArgStr())
       for (unsigned i = 0, e = getNumOptions(); i != e; ++i)
         OptionNames.push_back(getOption(i));
   }
 

 public:
   basic_parser_impl(Option &) {}
 
 
   enum ValueExpected getValueExpectedFlagDefault() const {
     return ValueRequired;
   }
 
-  void getExtraOptionNames(SmallVectorImpl<const char *> &) {}
+  void getExtraOptionNames(SmallVectorImpl<StringRef> &) {}
 
   void initialize() {}
 
   // Return the width of the option tag for printing...
   size_t getOptionWidth(const Option &O) const;
 
   // printOptionInfo - Print out information about this option.  The
   // to-be-maintained width is specified.
bool handleOccurrence(unsigned pos, StringRef ArgName,
     this->setValue(Val);
     this->setPosition(pos);
     return false;
   }
 
   enum ValueExpected getValueExpectedFlagDefault() const override {
     return Parser.getValueExpectedFlagDefault();
   }
-  void
-  getExtraOptionNames(SmallVectorImpl<const char *> &OptionNames) override {
+  void getExtraOptionNames(SmallVectorImpl<StringRef> &OptionNames) override {
     return Parser.getExtraOptionNames(OptionNames);
   }
 
   // Forward printing stuff to the parser...
   size_t getOptionWidth() const override {
     return Parser.getOptionWidth(*this);
   }
   void printOptionInfo(size_t GlobalWidth) const override {
template <class DataType, class StorageClass = bool,
           class ParserClass = parser<DataType>>
 class list : public Option, public list_storage<DataType, StorageClass> {
   std::vector<unsigned> Positions;
   ParserClass Parser;
 
   enum ValueExpected getValueExpectedFlagDefault() const override {
     return Parser.getValueExpectedFlagDefault();
   }
-  void
-  getExtraOptionNames(SmallVectorImpl<const char *> &OptionNames) override {
+  void getExtraOptionNames(SmallVectorImpl<StringRef> &OptionNames) override {
     return Parser.getExtraOptionNames(OptionNames);
   }
 
   bool handleOccurrence(unsigned pos, StringRef ArgName,
                         StringRef Arg) override {
     typename ParserClass::parser_data_type Val =
         typename ParserClass::parser_data_type();
     if (Parser.parse(*this, ArgName, Arg, Val))
template <class DataType, class Storage = bool,
           class ParserClass = parser<DataType>>
 class bits : public Option, public bits_storage<DataType, Storage> {
   std::vector<unsigned> Positions;
   ParserClass Parser;
 
   enum ValueExpected getValueExpectedFlagDefault() const override {
     return Parser.getValueExpectedFlagDefault();
   }
-  void
-  getExtraOptionNames(SmallVectorImpl<const char *> &OptionNames) override {
+  void getExtraOptionNames(SmallVectorImpl<StringRef> &OptionNames) override {
     return Parser.getExtraOptionNames(OptionNames);
   }
 
   bool handleOccurrence(unsigned pos, StringRef ArgName,
                         StringRef Arg) override {
     typename ParserClass::parser_data_type Val =
         typename ParserClass::parser_data_type();
     if (Parser.parse(*this, ArgName, Arg, Val))

llvm/trunk/lib/Support/CommandLine.cpp

if (!Opt.hasArgStr()) {
                << "' registered more than once!\n";
         report_fatal_error("inconsistency in registered CommandLine options");
       }
     }
   }
 
   void addOption(Option *O) {
     bool HadErrors = false;
-    if (O->ArgStr[0]) {
+    if (O->hasArgStr()) {
       // Add argument to the argument map!
       if (!OptionsMap.insert(std::make_pair(O->ArgStr, O)).second) {
         errs() << ProgramName << ": CommandLine Error: Option '" << O->ArgStr
                << "' registered more than once!\n";
         HadErrors = true;
       }
     }
 
void addOption(Option *O) {
     // indicate serious issues such as conflicting option names or an
     // incorrectly
     // linked LLVM distribution.
     if (HadErrors)
       report_fatal_error("inconsistency in registered CommandLine options");
   }
 
   void removeOption(Option *O) {
-    SmallVector<const char *, 16> OptionNames;
+    SmallVector<StringRef, 16> OptionNames;
     O->getExtraOptionNames(OptionNames);
-    if (O->ArgStr[0])
+    if (O->hasArgStr())
       OptionNames.push_back(O->ArgStr);
     for (auto Name : OptionNames)
-      OptionsMap.erase(StringRef(Name));
+      OptionsMap.erase(Name);
 
     if (O->getFormattingFlag() == cl::Positional)
       for (auto Opt = PositionalOpts.begin(); Opt != PositionalOpts.end();
            ++Opt) {
         if (*Opt == O) {
           PositionalOpts.erase(Opt);
           break;
         }
else if (O == ConsumeAfterOpt)
       ConsumeAfterOpt = nullptr;
   }
 
   bool hasOptions() {
     return (!OptionsMap.empty() || !PositionalOpts.empty() ||
             nullptr != ConsumeAfterOpt);
   }
 
-  void updateArgStr(Option *O, const char *NewName) {
+  void updateArgStr(Option *O, StringRef NewName) {
     if (!OptionsMap.insert(std::make_pair(NewName, O)).second) {
       errs() << ProgramName << ": CommandLine Error: Option '" << O->ArgStr
              << "' registered more than once!\n";
       report_fatal_error("inconsistency in registered CommandLine options");
     }
-    OptionsMap.erase(StringRef(O->ArgStr));
+    OptionsMap.erase(O->ArgStr);
   }
 
   void printOptionValues();
 
   void registerCategory(OptionCategory *cat) {
     assert(std::count_if(RegisteredOptionCategories.begin(),
                          RegisteredOptionCategories.end(),
                          [cat](const OptionCategory *Category) {

 
 void Option::addArgument() {
   GlobalParser->addOption(this);
   FullyInitialized = true;
 }
 
 void Option::removeArgument() { GlobalParser->removeOption(this); }
 
-void Option::setArgStr(const char *S) {
+void Option::setArgStr(StringRef S) {
   if (FullyInitialized)
     GlobalParser->updateArgStr(this, S);
   ArgStr = S;
 }
 
 // Initialise the general option category.
 OptionCategory llvm::cl::GeneralCategory("General options");
 
static Option *LookupNearestOption(StringRef Arg,
 
   // Find the closest match.
   Option *Best = nullptr;
   unsigned BestDistance = 0;
   for (StringMap<Option *>::const_iterator it = OptionsMap.begin(),
                                            ie = OptionsMap.end();
        it != ie; ++it) {
     Option *O = it->second;
-    SmallVector<const char *, 16> OptionNames;
+    SmallVector<StringRef, 16> OptionNames;
     O->getExtraOptionNames(OptionNames);
-    if (O->ArgStr[0])
+    if (O->hasArgStr())
       OptionNames.push_back(O->ArgStr);
 
     bool PermitValue = O->getValueExpectedFlag() != cl::ValueDisallowed;
     StringRef Flag = PermitValue ? LHS : Arg;
-    for (size_t i = 0, e = OptionNames.size(); i != e; ++i) {
-      StringRef Name = OptionNames[i];
+    for (auto Name : OptionNames) {
       unsigned Distance = StringRef(Name).edit_distance(
           Flag, /*AllowReplacements=*/true, /*MaxEditDistance=*/BestDistance);
       if (!Best || Distance < BestDistance) {
         Best = O;
         BestDistance = Distance;
         if (RHS.empty() || !PermitValue)
-          NearestString = OptionNames[i];
+          NearestString = Name;
         else
-          NearestString = (Twine(OptionNames[i]) + "=" + RHS).str();
+          NearestString = (Twine(Name) + "=" + RHS).str();
       }
     }
   }
 
   return Best;
 }
 
 /// CommaSeparateAndAddOccurrence - A wrapper around Handler->addOccurrence()
for (size_t i = 0, e = PositionalOpts.size(); i != e; ++i) {
       else if (ConsumeAfterOpt) {
         // ConsumeAfter cannot be combined with "optional" positional options
         // unless there is only one positional argument...
         if (PositionalOpts.size() > 1)
           ErrorParsing |= Opt->error(
               "error - this positional option will never be matched, "
               "because it does not Require a value, and a "
               "cl::ConsumeAfter option is active!");
-      } else if (UnboundedFound && !Opt->ArgStr[0]) {
+      } else if (UnboundedFound && !Opt->hasArgStr()) {
         // This option does not "require" a value...  Make sure this option is
         // not specified after an option that eats all extra arguments, or this
         // one will never get any!
         //
         ErrorParsing |= Opt->error("error - option can never match, because "
                                    "another positional argument will match an "
                                    "unbounded number of values, and this option"
                                    " does not require a value!");
bool Option::addOccurrence(unsigned pos, StringRef ArgName, StringRef Value,
   }
 
   return handleOccurrence(pos, ArgName, Value);
 }
 
 // getValueStr - Get the value description string, using "DefaultMsg" if nothing
 // has been specified yet.
 //
-static const char *getValueStr(const Option &O, const char *DefaultMsg) {
-  if (O.ValueStr[0] == 0)
+static StringRef getValueStr(const Option &O, StringRef DefaultMsg) {
+  if (O.ValueStr.empty())
     return DefaultMsg;
   return O.ValueStr;
 }
 
 //===----------------------------------------------------------------------===//
 // cl::alias class implementation
 //
 
 // Return the width of the option tag for printing...
-size_t alias::getOptionWidth() const { return std::strlen(ArgStr) + 6; }
+size_t alias::getOptionWidth() const { return ArgStr.size() + 6; }
 
 static void printHelpStr(StringRef HelpStr, size_t Indent,
                          size_t FirstLineIndentedBy) {
   std::pair<StringRef, StringRef> Split = HelpStr.split('\n');
   outs().indent(Indent - FirstLineIndentedBy) << " - " << Split.first << "\n";
   while (!Split.second.empty()) {
     Split = Split.second.split('\n');
     outs().indent(Indent) << Split.first << "\n";
   }
 }
 
 // Print out the option for the alias.
 void alias::printOptionInfo(size_t GlobalWidth) const {
   outs() << "  -" << ArgStr;
-  printHelpStr(HelpStr, GlobalWidth, std::strlen(ArgStr) + 6);
+  printHelpStr(HelpStr, GlobalWidth, ArgStr.size() + 6);
 }
 
 //===----------------------------------------------------------------------===//
 // Parser Implementation code...
 //
 
 // basic_parser implementation
 //
 
 // Return the width of the option tag for printing...
 size_t basic_parser_impl::getOptionWidth(const Option &O) const {
-  size_t Len = std::strlen(O.ArgStr);
+  size_t Len = O.ArgStr.size();
   if (const char *ValName = getValueName())
-    Len += std::strlen(getValueStr(O, ValName)) + 3;
+    Len += getValueStr(O, ValName).size() + 3;
 
   return Len + 6;
 }
 
 // printOptionInfo - Print out information about this option.  The
 // to-be-maintained width is specified.
 //
 void basic_parser_impl::printOptionInfo(const Option &O,
                                         size_t GlobalWidth) const {
   outs() << "  -" << O.ArgStr;
 
   if (const char *ValName = getValueName())
     outs() << "=<" << getValueStr(O, ValName) << '>';
 
   printHelpStr(O.HelpStr, GlobalWidth, getOptionWidth(O));
 }
 
 void basic_parser_impl::printOptionName(const Option &O,
                                         size_t GlobalWidth) const {
   outs() << "  -" << O.ArgStr;
-  outs().indent(GlobalWidth - std::strlen(O.ArgStr));
+  outs().indent(GlobalWidth - O.ArgStr.size());
 }
 
 // parser<bool> implementation
 //
 bool parser<bool>::parse(Option &O, StringRef ArgName, StringRef Arg,
                          bool &Value) {
   if (Arg == "" || Arg == "true" || Arg == "TRUE" || Arg == "True" ||
       Arg == "1") {
if (strcmp(getOption(i), Name) == 0)
       return i;
   }
   return e;
 }
 
 // Return the width of the option tag for printing...
 size_t generic_parser_base::getOptionWidth(const Option &O) const {
   if (O.hasArgStr()) {
-    size_t Size = std::strlen(O.ArgStr) + 6;
+    size_t Size = O.ArgStr.size() + 6;
     for (unsigned i = 0, e = getNumOptions(); i != e; ++i)
       Size = std::max(Size, std::strlen(getOption(i)) + 8);
     return Size;
   } else {
     size_t BaseSize = 0;
     for (unsigned i = 0, e = getNumOptions(); i != e; ++i)
       BaseSize = std::max(BaseSize, std::strlen(getOption(i)) + 8);
     return BaseSize;
   }
 }
 
 // printOptionInfo - Print out information about this option.  The
 // to-be-maintained width is specified.
 //
 void generic_parser_base::printOptionInfo(const Option &O,
                                           size_t GlobalWidth) const {
   if (O.hasArgStr()) {
     outs() << "  -" << O.ArgStr;
-    printHelpStr(O.HelpStr, GlobalWidth, std::strlen(O.ArgStr) + 6);
+    printHelpStr(O.HelpStr, GlobalWidth, O.ArgStr.size() + 6);
 
     for (unsigned i = 0, e = getNumOptions(); i != e; ++i) {
       size_t NumSpaces = GlobalWidth - strlen(getOption(i)) - 8;
       outs() << "    =" << getOption(i);
       outs().indent(NumSpaces) << " -   " << getDescription(i) << '\n';
     }
   } else {
-    if (O.HelpStr[0])
+    if (!O.HelpStr.empty())
       outs() << "  " << O.HelpStr << '\n';
     for (unsigned i = 0, e = getNumOptions(); i != e; ++i) {
       const char *Option = getOption(i);
       outs() << "    -" << Option;
       printHelpStr(getDescription(i), GlobalWidth, std::strlen(Option) + 8);
     }
   }
 }
 
 static const size_t MaxOptWidth = 8; // arbitrary spacing for printOptionDiff
 
 // printGenericOptionDiff - Print the value of this option and it's default.
 //
 // "Generic" options have each value mapped to a name.
 void generic_parser_base::printGenericOptionDiff(
     const Option &O, const GenericOptionValue &Value,
     const GenericOptionValue &Default, size_t GlobalWidth) const {
   outs() << "  -" << O.ArgStr;
-  outs().indent(GlobalWidth - std::strlen(O.ArgStr));
+  outs().indent(GlobalWidth - O.ArgStr.size());
 
   unsigned NumOpts = getNumOptions();
   for (unsigned i = 0; i != NumOpts; ++i) {
     if (Value.compare(getOptionValue(i)))
       continue;
 
     outs() << "= " << getOption(i);
     size_t L = std::strlen(getOption(i));
void operator=(bool Value) {
     sortOpts(GlobalParser->OptionsMap, Opts, ShowHidden);
 
     if (GlobalParser->ProgramOverview)
       outs() << "OVERVIEW: " << GlobalParser->ProgramOverview << "\n";
 
     outs() << "USAGE: " << GlobalParser->ProgramName << " [options]";
 
     for (auto Opt : GlobalParser->PositionalOpts) {
-      if (Opt->ArgStr[0])
+      if (Opt->hasArgStr())
         outs() << " --" << Opt->ArgStr;
       outs() << " " << Opt->HelpStr;
     }
 
     // Print the consume after option info if it exists...
     if (GlobalParser->ConsumeAfterOpt)
       outs() << " " << GlobalParser->ConsumeAfterOpt->HelpStr;
 

llvm/trunk/unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, ModifyExisitingOption) {
   ASSERT_EQ(&cl::GeneralCategory,Retrieved->Category) <<
     "Incorrect default option category.";
 
   Retrieved->setCategory(TestCategory);
   ASSERT_EQ(&TestCategory,Retrieved->Category) <<
     "Failed to modify option's option category.";
 
   Retrieved->setDescription(Description);
-  ASSERT_STREQ(Retrieved->HelpStr, Description) <<
-    "Changing option description failed.";
+  ASSERT_STREQ(Retrieved->HelpStr.data(), Description)
+      << "Changing option description failed.";
 
   Retrieved->setArgStr(ArgString);
-  ASSERT_STREQ(ArgString, Retrieved->ArgStr) <<
-    "Failed to modify option's Argument string.";
+  ASSERT_STREQ(ArgString, Retrieved->ArgStr.data())
+      << "Failed to modify option's Argument string.";
 
   Retrieved->setValueStr(ValueString);
-  ASSERT_STREQ(Retrieved->ValueStr, ValueString) <<
-    "Failed to modify option's Value string.";
+  ASSERT_STREQ(Retrieved->ValueStr.data(), ValueString)
+      << "Failed to modify option's Value string.";
 
   Retrieved->setHiddenFlag(cl::Hidden);
   ASSERT_EQ(cl::Hidden, TestOption.getOptionHiddenFlag()) <<
     "Failed to modify option's hidden flag.";
 }
 #ifndef SKIP_ENVIRONMENT_TESTS
 
 const char test_env_var[] = "LLVM_TEST_COMMAND_LINE_FLAGS";

llvm/trunk/utils/FileCheck/FileCheck.cpp

static bool ReadCheckFile(SourceMgr &SM,
   StringRef Buffer = F->getBuffer();
 
   SM.AddNewSourceBuffer(std::move(F), SMLoc());
 
   std::vector<Pattern> ImplicitNegativeChecks;
   for (const auto &PatternString : ImplicitCheckNot) {
     // Create a buffer with fake command line content in order to display the
     // command line option responsible for the specific implicit CHECK-NOT.
-    std::string Prefix = std::string("-") + ImplicitCheckNot.ArgStr + "='";
+    std::string Prefix = (Twine("-") + ImplicitCheckNot.ArgStr + "='").str();
     std::string Suffix = "'";
     std::unique_ptr<MemoryBuffer> CmdLine = MemoryBuffer::getMemBufferCopy(
         Prefix + PatternString + Suffix, "command line");
 
     StringRef PatternInBuffer =
         CmdLine->getBuffer().substr(Prefix.size(), PatternString.size());
     SM.AddNewSourceBuffer(std::move(CmdLine), SMLoc());