[llvm] r211260 - CommandLine: bail out when options get multiply registered
Alp Toker
alp at nuanti.com
Thu Jun 19 09:05:59 PDT 2014
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