[llvm] r211260 - CommandLine: bail out when options get multiply registered

[Alp Toker]

On 19/06/2014 17:23, David Blaikie wrote:
> On Thu, Jun 19, 2014 at 12:25 AM, Alp Toker <alp at nuanti.com> wrote:
>> Author: alp
>> Date: Thu Jun 19 02:25:25 2014
>> New Revision: 211260
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=211260&view=rev
>> Log:
>> CommandLine: bail out when options get multiply registered
>>
>> These errors are strictly unrecoverable and indicate serious issues such as
>> conflicting option names or an incorrectly linked LLVM distribution.
>>
>> With this change, the errors actually get detected so tests don't pass
>> silently.
>>
>> Modified:
>>      llvm/trunk/lib/Support/CommandLine.cpp
>>
>> Modified: llvm/trunk/lib/Support/CommandLine.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/CommandLine.cpp?rev=211260&r1=211259&r2=211260&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Support/CommandLine.cpp (original)
>> +++ llvm/trunk/lib/Support/CommandLine.cpp Thu Jun 19 02:25:25 2014
>> @@ -145,6 +145,7 @@ void OptionCategory::registerCategory()
>>   static void GetOptionInfo(SmallVectorImpl<Option*> &PositionalOpts,
>>                             SmallVectorImpl<Option*> &SinkOpts,
>>                             StringMap<Option*> &OptionsMap) {
>> +  bool HadErrors = false;
>>     SmallVector<const char*, 16> OptionNames;
>>     Option *CAOpt = nullptr;  // The ConsumeAfter option if it exists.
>>     for (Option *O = RegisteredOptionList; O; O = O->getNextRegisteredOption()) {
>> @@ -158,8 +159,9 @@ static void GetOptionInfo(SmallVectorImp
>>       for (size_t i = 0, e = OptionNames.size(); i != e; ++i) {
>>         // Add argument to the argument map!
>>         if (OptionsMap.GetOrCreateValue(OptionNames[i], O).second != O) {
>> -        errs() << ProgramName << ": CommandLine Error: Argument '"
>> -             << OptionNames[i] << "' defined more than once!\n";
>> +        errs() << ProgramName << ": CommandLine Error: Option '"
>> +               << OptionNames[i] << "' registered more than once!\n";
>> +        HadErrors = true;
> Is there any reason to use the flag and defer the fatal error until
> later rather than just calling it directly here

Yes. It's this way because a single notice isn't helpful in diagnosing 
issues.

You need to see the complete set to gain sufficient context to diagnose 
link issues.

Take for example a plugin that mistakenly statically links in parts of 
LLVM, causing each of the static initializations in certain modules to 
get run twice. It's only when you see them all that you can figure out 
what's going on.


> (and in the other case
> below)?

That one also provides an important diagnostic. If there's more than one 
cl::ConsumeAfter option that's a strong indicator that you linked in 
modules containing sets of options that weren't meant to be initialized 
in the same application.

>
>>         }
>>       }
>>
>> @@ -171,8 +173,10 @@ static void GetOptionInfo(SmallVectorImp
>>       else if (O->getMiscFlags() & cl::Sink) // Remember sink options
>>         SinkOpts.push_back(O);
>>       else if (O->getNumOccurrencesFlag() == cl::ConsumeAfter) {
>> -      if (CAOpt)
>> +      if (CAOpt) {
>>           O->error("Cannot specify more than one option with cl::ConsumeAfter!");
>> +        HadErrors = true;
>> +      }
>>         CAOpt = O;
>>       }
>>     }
>> @@ -182,6 +186,12 @@ static void GetOptionInfo(SmallVectorImp
>>
>>     // Make sure that they are in order of registration not backwards.
>>     std::reverse(PositionalOpts.begin(), PositionalOpts.end());
>> +
>> +  // Fail hard if there were errors. These are strictly unrecoverable and
>> +  // indicate serious issues such as conflicting option names or an incorrectly
>> +  // linked LLVM distribution.
>> +  if (HadErrors)
>> +    report_fatal_error("inconsistency in registered CommandLine options");
>>   }

The report_fatal_error() exit is placed intentionally at the end of the 
function. We emit all useful diagnostics to help fix the issue before 
preventing progression to the next phase if fatal errors were 
encountered -- if someone wants to do so in future they can propagate 
this as a bool return, but there isn't a need to do so at present 
because the state is always fatal for current callers.

Alp.


>>
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-- 
http://www.nuanti.com
the browser experts