[llvm] r211260 - CommandLine: bail out when options get multiply registered
David Blaikie
dblaikie at gmail.com
Thu Jun 19 09:08:57 PDT 2014
Can't say I'd recalled seeing this ever come up, actually - I stand
corrected. (all I had to go on was the recent issue with debug-version
I assumed you were motivated by/fixing)
On Thu, Jun 19, 2014 at 9:05 AM, Alp Toker <alp at nuanti.com> wrote:
>
> On 19/06/2014 18:39, David Blaikie wrote:
>>
>> 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.
>
>
> Infrequent? By my reckoning it's one of the most-reported issues from new
> LLVM developers and we're not doing enough to help diagnose the problem.
> Just the first page results from a quick web search:
>
> [LLVMdev] Question about compiling plugins for LLVM tools
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2011-February/037825.html
>
> LLVM: Optimization loading failing on OSX
> http://stackoverflow.com/questions/15475889/llvm-optimization-loading-failing-on-osx
>
> Loading Static Analyzer plugin fails with CommandLine Errors
> http://comments.gmane.org/gmane.comp.compilers.clang.devel/31462
>
> CLANG getting started
> http://comments.gmane.org/gmane.comp.compilers.clang.devel/6870
>
> [cfe-users] clang interpreter example - The Mail Archive
> https://www.mail-archive.com/cfe-users@cs.uiuc.edu/msg00224.html
>
> Strange error on make in OS X lion - LassoSoft
> http://newlists.lassosoft.com/pipermail/lasso9beta/2012-April/000681.html
>
> Re: [llvm] r191343
> http://marc.info/?l=llvm-commits&m=138031725214525&w=2
>
> Alp.
>
>
>
>>
>> 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
>>>
>
> --
> http://www.nuanti.com
> the browser experts
>
More information about the llvm-commits
mailing list