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