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

[David Blaikie]

On Thu, Jun 19, 2014 at 8:33 AM, Alp Toker <alp at nuanti.com> wrote:
>
> 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.

In the example this came up in initially there was just one error and
it was at least /fairly/ easily diagnosed, so I'm not sure it's
vitally important that we print all the errors - doubly so given the
infrequency with which this seems to come up.

It's just a bit easier to understand the code when there's less
state/non-locality and I'm not sure it provides that much more benefit
in diagnosing these issues, but I could be wrong.

>> (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.

Oh, I wasn't questioning the need for the diagnostic - just whether or
not, again, it could just be reported/failed immediately, rather than
having to use a flag and a distant erroring out. It's just easier to
understand the code when you see the report_fatal_error immediately,
you don't have to go chasing the flag to see what other behavior it
causes.

Anyway - up to you, that's just my 2c.

- David

>
>
>>
>>>         }
>>>       }
>>>
>>> @@ -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
>