On Tue, Dec 3, 2013 at 1:47 PM, Manman Ren <manman.ren at gmail.com> wrote:
>
>
>
> On Tue, Dec 3, 2013 at 1:26 PM, Eric Christopher <echristo at gmail.com>wrote:
>
>> >> > Can you be specific on where I am hard coding operations?
>> >>
>> >> When you check for which operand is where. In other words:
>> >>
>> >> + if (Flag->getNumOperands() >= 3 &&
>> >> isa<ConstantInt>(Flag->getOperand(0)) &&
>> >> + isa<MDString>(Flag->getOperand(1))) {
>> >> + // Check the operands of the MDNode before accessing the
>> operands.
>> >> + // The verifier will actually catch these failures.
>> >> + ConstantInt *Behavior = cast<ConstantInt>(Flag->getOperand(0));
>> >> + MDString *Key = cast<MDString>(Flag->getOperand(1));
>> >> + Value *Val = Flag->getOperand(2);
>> >> +
>> >>
>> Flags.push_back(ModuleFlagEntry(ModFlagBehavior(Behavior->getZExtValue()),
>> >> + Key, Val));
>> >>
>> >> here.
>> >
>> >
>> > As the comments stated, "Check the operands of the MDNode before
>> accessing
>> > the operands".
>> > We are checking that the MDNode has at least 3 operands and the first
>> and
>> > second operands have the correct format before
>> > accessing the 3rd operand in Flag->getOperand(2),
>> > cast<ConstantInt>(Flag->getOperands(0)) and
>> > cast<MDString>(Flag->getOperand(1)).
>> >
>> > My understanding is that we should not seg fault when module flag has
>> > incorrect format, that is why I added the if condition to guard the
>> > accesses.
>>
>> Right, but this presupposes that you're doing it at the right place in
>> the workflow.
>>
>
> Do you have any specific suggestion on what we should do when running llc
> on bad IR?
> Right now, it crashes without this patch. With this patch, it will ignore
> bad-formed module flags.
>
Expand a little bit :)
With my patch, llc will throw verification error. The code path does safe
check, and ignores ill-formed module flags. The verifier
will throw verification error. And I believe that is the right way to do it.
>
>
>> >> This seems to me to be an indication that we're doing this in the
>> >> wrong location. I think it might be more clean if we were to run the
>> >> IR verifier and then as a separate pass to verify debug info metadata
>> >> that will also strip if the version number isn't what's expected.
>> >> Seems to have the right mental sequencing at least.
>> >
>> >
>> > No matter whether the verifier is on or not, we should not crash. If you
>> > agree on that, then we should guard the above accesses.
>>
>> I agree that we should not crash, however, the guard is interesting.
>>
>> > I don't think we should run the IR verifier before we upgrade (drop) the
>> > debug info. Similar to other IR upgrading, we upgrade first, then
>> verify the
>>
>> The part that you're dealing with is not part of the debug info
>> though. It's the normal IR sequence.
>>
>> > upgraded IR passes the verifier.
>> > The old format before upgrading should not pass the verifier.
>> >
>> > I thought we agreed that stripping should not be part of debug info
>> > verifier. User can turn on/off verifier, but stripping should happen
>> always.
>> >
>>
>> I agree with this part, however, you're working around bad IR in the
>> pass rather than having it fail to load/warn/upgrade/etc and I
>> disagree with this.
>>
>
> This is a general problem with bad IR (it is not specific to dropping the
> debug info), what should we do if we have bad IR and the IR is also in an
> old format?
> Should we auto-upgrade first? What we do currently is to auto-upgrading
> during loading the files.
> Do you have any specific suggestion?
>
Currently, we auto-upgrade first, then run verifier to catch errors after
upgrading. I think that is the right way to do it.
We ignore bad IR during upgrading, but the verifier will catch errors later
on.
I can't think of other orders that work better.
Manman
> Manman
>
>
>> -eric
>>
>>
>>
>> > Thanks,
>> > Manman
>> >
>> >>
>> >>
>> >> -eric
>> >>
>> >>
>> >>
>> >> > Manman
>> >> >
>> >> >>
>> >> >>
>> >> >> -eric
>> >> >>
>> >> >> > Cheers,
>> >> >> > Manman
>> >> >> >
>> >> >> >>
>> >> >> >>
>> >> >> >> > Modified: llvm/trunk/lib/IR/Module.cpp
>> >> >> >> > URL:
>> >> >> >> >
>> >> >> >> >
>> >> >> >> >
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Module.cpp?rev=196158&r1=196157&r2=196158&view=diff
>> >> >> >> >
>> >> >> >> >
>> >> >> >> >
>> >> >> >> >
>> ==============================================================================
>> >> >> >> > --- llvm/trunk/lib/IR/Module.cpp (original)
>> >> >> >> > +++ llvm/trunk/lib/IR/Module.cpp Mon Dec 2 15:29:56 2013
>> >> >> >> > @@ -318,11 +318,16 @@ getModuleFlagsMetadata(SmallVectorImpl<M
>> >> >> >> >
>> >> >> >> > for (unsigned i = 0, e = ModFlags->getNumOperands(); i != e;
>> >> >> >> > ++i)
>> >> >> >> > {
>> >> >> >> > MDNode *Flag = ModFlags->getOperand(i);
>> >> >> >> > - ConstantInt *Behavior =
>> >> >> >> > cast<ConstantInt>(Flag->getOperand(0));
>> >> >> >> > - MDString *Key = cast<MDString>(Flag->getOperand(1));
>> >> >> >> > - Value *Val = Flag->getOperand(2);
>> >> >> >> > -
>> >> >> >> >
>> >> >> >> >
>> >> >> >> >
>> Flags.push_back(ModuleFlagEntry(ModFlagBehavior(Behavior->getZExtValue()),
>> >> >> >> > - Key, Val));
>> >> >> >> > + if (Flag->getNumOperands() >= 3 &&
>> >> >> >> > isa<ConstantInt>(Flag->getOperand(0)) &&
>> >> >> >> > + isa<MDString>(Flag->getOperand(1))) {
>> >> >> >> > + // Check the operands of the MDNode before accessing the
>> >> >> >> > operands.
>> >> >> >> > + // The verifier will actually catch these failures.
>> >> >> >> > + ConstantInt *Behavior =
>> >> >> >> > cast<ConstantInt>(Flag->getOperand(0));
>> >> >> >> > + MDString *Key = cast<MDString>(Flag->getOperand(1));
>> >> >> >> > + Value *Val = Flag->getOperand(2);
>> >> >> >> > +
>> >> >> >> >
>> >> >> >> >
>> >> >> >> >
>> Flags.push_back(ModuleFlagEntry(ModFlagBehavior(Behavior->getZExtValue()),
>> >> >> >> > + Key, Val));
>> >> >> >> > + }
>> >> >> >> > }
>> >> >> >> > }
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > _______________________________________________
>> >> >> >> > 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/20131203/853a10f1/attachment.html>