[llvm] r200858 - Revert "Fix an invalid check for duplicate option categories."

[Alexander Kornienko]

Here's the alternative patch:


Index: lib/Support/CommandLine.cpp
===================================================================
--- lib/Support/CommandLine.cpp (revision 200882)
+++ lib/Support/CommandLine.cpp (working copy)
@@ -36,6 +36,7 @@
 #include <cerrno>
 #include <cstdlib>
 #include <map>
+#include <set>
 using namespace llvm;
 using namespace cl;

@@ -1495,9 +1496,7 @@
   // It shall return true if A's name should be lexographically
   // ordered before B's name. It returns false otherwise.
   static bool OptionCategoryCompare(OptionCategory *A, OptionCategory *B) {
-    int Length = strcmp(A->getName(), B->getName());
-    assert(Length != 0 && "Duplicate option categories");
-    return Length < 0;
+    return strcmp(A->getName(), B->getName()) < 0;
   }

   // Make sure we inherit our base class's operator=()
@@ -1507,13 +1506,17 @@
   virtual void printOptions(StrOptionPairVector &Opts, size_t MaxArgLen) {
     std::vector<OptionCategory *> SortedCategories;
     std::map<OptionCategory *, std::vector<Option *> > CategorizedOptions;
+    std::set<std::string> CategoryNames;

     // Collect registered option categories into vector in preparation for
     // sorting.
     for (OptionCatSet::const_iterator I = RegisteredOptionCategories->begin(),
                                       E = RegisteredOptionCategories->end();
-         I != E; ++I)
+         I != E; ++I) {
       SortedCategories.push_back(*I);
+      assert(CategoryNames.insert((*I)->getName()).second &&
+             "Duplicate option categories");
+    }

     // Sort the different option categories alphabetically.
     assert(SortedCategories.size() > 0 && "No option categories registered!");



On Wed, Feb 5, 2014 at 11:49 PM, Alexander Kornienko <alexfh at google.com>wrote:

> Jordan,
>
> I guess, you know something about analyzer plugins ;)
>
> We have a problem here, which this patch just reveals. The analyzer plugin
> links with the llvm/Support library, and it has the second copy of all the
> global variables defined in llvm/Support- the first one is in the clang
> binary - in particular llvm::cl::GeneralCategory. As the patch in r200853
> performs the check for duplicate option categories during the registration,
> this leads to a crash.
>
> I'm not sure what a proper solution would look like. Avoiding global
> variables or avoiding linking plugins with llvm/clang libraries would
> certainly solve the problem ;), but would require non-trivial effort. I can
> implement a correct check, and leave it only when printing the help
> message. It doesn't seem completely right, but solves both crashes.
>
>
> On Wed, Feb 5, 2014 at 6:49 PM, Rafael Espindola <
> rafael.espindola at gmail.com> wrote:
>
>> Author: rafael
>> Date: Wed Feb  5 11:49:31 2014
>> New Revision: 200858
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=200858&view=rev
>> Log:
>> Revert "Fix an invalid check for duplicate option categories."
>>
>> This reverts commit r200853.
>>
>> It was causing clang/Analysis/checker-plugins.c to crash.
>>
>> Modified:
>>     llvm/trunk/include/llvm/Support/CommandLine.h
>>     llvm/trunk/lib/Support/CommandLine.cpp
>>
>> Modified: llvm/trunk/include/llvm/Support/CommandLine.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/CommandLine.h?rev=200858&r1=200857&r2=200858&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Support/CommandLine.h (original)
>> +++ llvm/trunk/include/llvm/Support/CommandLine.h Wed Feb  5 11:49:31 2014
>> @@ -149,8 +149,8 @@ private:
>>  public:
>>    OptionCategory(const char *const Name, const char *const Description =
>> 0)
>>        : Name(Name), Description(Description) { registerCategory(); }
>> -  const char *getName() const { return Name; }
>> -  const char *getDescription() const { return Description; }
>> +  const char *getName() { return Name; }
>> +  const char *getDescription() { return Description; }
>>  };
>>
>>  // The general Option Category (used as default category).
>>
>> Modified: llvm/trunk/lib/Support/CommandLine.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/CommandLine.cpp?rev=200858&r1=200857&r2=200858&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Support/CommandLine.cpp (original)
>> +++ llvm/trunk/lib/Support/CommandLine.cpp Wed Feb  5 11:49:31 2014
>> @@ -125,21 +125,8 @@ static ManagedStatic<OptionCatSet> Regis
>>  // Initialise the general option category.
>>  OptionCategory llvm::cl::GeneralCategory("General options");
>>
>> -struct HasName {
>> -  HasName(StringRef Name) : Name(Name) {}
>> -  bool operator()(const OptionCategory *Category) const {
>> -    return Name == Category->getName();
>> -  }
>> -  StringRef Name;
>> -};
>> -
>>  void OptionCategory::registerCategory()
>>  {
>> -  assert(std::count_if(RegisteredOptionCategories->begin(),
>> -                       RegisteredOptionCategories->end(),
>> -                       HasName(getName())) == 0 &&
>> -         "Duplicate option categories");
>> -
>>    RegisteredOptionCategories->insert(this);
>>  }
>>
>> @@ -1508,7 +1495,9 @@ public:
>>    // It shall return true if A's name should be lexographically
>>    // ordered before B's name. It returns false otherwise.
>>    static bool OptionCategoryCompare(OptionCategory *A, OptionCategory
>> *B) {
>> -    return strcmp(A->getName(), B->getName()) < 0;
>> +    int Length = strcmp(A->getName(), B->getName());
>> +    assert(Length != 0 && "Duplicate option categories");
>> +    return Length < 0;
>>    }
>>
>>    // Make sure we inherit our base class's operator=()
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140206/d57c9011/attachment.html>