[llvm] r200858 - Revert "Fix an invalid check for duplicate option categories."
Jordan Rose
jordan_rose at apple.com
Fri Feb 7 09:29:42 PST 2014
On Feb 6, 2014, at 1:50 , Alexander Kornienko <alexfh at google.com> wrote:
> On Thu, Feb 6, 2014 at 5:03 AM, Jordan Rose <jordan_rose at apple.com> wrote:
> It would actually make sense for the plugin not to link LLVM libraries known to be in Clang, but on the flip side it would also make sense for the general option category to not be a static variable.
>
> <rant>Honestly, my personal opinion is that the whole CommandLine library is really messed up, and is using global variables and static initialization in pretty much exactly the ways a C++ style guide would tell you not to. :-) (You'll notice how I made a stack-based cl::Option variant when adding unit tests for my recent change.)</rant>
>
> Okay, rant aside, I think we should get loadable bundles building without linking in LLVM libraries. That doesn't help if two plugins share a static library that the main executable doesn't include, but Support certainly isn't going to be one of those.
>
> Are you going to take care of the plugin rework? Is it fine to go with the intermediate solution in the meanwhile? It's strictly better, than the status quo, as it resolves crashes when using -help in the assertions-enabled build with a certain STL implementation.
*sigh* Yes, all right. :-)
(The *sigh* is that I haven't touched those examples in over a year.)
Jordan
> 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!");
>
>
> Jordan
>
>
> On Feb 5, 2014, at 14:49 , 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/20140207/0ec2913f/attachment.html>
More information about the llvm-commits
mailing list