<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Dec 5, 2013 at 1:11 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">On Tue, Dec 3, 2013 at 3:12 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Tue, Dec 3, 2013 at 1:47 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
>><br>
>><br>
>><br>
>><br>
>> On Tue, Dec 3, 2013 at 1:26 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>><br>
>> wrote:<br>
>>><br>
>>> >> > 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<br>
>>> >> 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>
>>> >><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<br>
>>> > accessing<br>
>>> > the operands".<br>
>>> > We are checking that the MDNode has at least 3 operands and the first<br>
>>> > 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>
>>> Right, but this presupposes that you're doing it at the right place in<br>
>>> the workflow.<br>
>><br>
>><br>
>> Do you have any specific suggestion on what we should do when running llc<br>
>> on bad IR?<br>
>> Right now, it crashes without this patch. With this patch, it will ignore<br>
>> bad-formed module flags.<br>
><br>
><br>
> Expand a little bit :)<br>
> With my patch, llc will throw verification error. The code path does safe<br>
> check, and ignores ill-formed module flags. The verifier<br>
> will throw verification error. And I believe that is the right way to do it.<br>
><br>
<br>
</div></div>Ignoring or throwing away. I.e. what if another pass used a module<br>
flag to do the same sort of thing and it was then seen as invalid?<br>
Would they, then, have to work around it the same way you have? I<br>
think so and so it's not a good long term solution.<br></blockquote><div><br></div><div>The change is on <span style="font-family:arial,sans-serif;font-size:13px">getModuleFlagsMetadata to make sure it does</span><span style="font-family:arial,sans-serif;font-size:13px"> not seg fault.</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">There is no change required on passes that call </span><span style="font-family:arial,sans-serif;font-size:13px">getModuleFlagsMetadata.</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br>
</span></div><div><span style="font-family:arial,sans-serif;font-size:13px">I could have separated the change from dropping debug info patches. It is required to</span></div><div><span style="font-family:arial,sans-serif;font-size:13px">make sure "llc </span><span style="font-family:arial,sans-serif;font-size:13px">module-flags-1.ll" does not seg fault.</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div class="h5"><br>
>><br>
>><br>
>>><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<br>
>>> > you<br>
>>> > agree on that, then we should guard the above accesses.<br>
>>><br>
>>> I agree that we should not crash, however, the guard is interesting.<br>
>>><br>
>>> > I don't think we should run the IR verifier before we upgrade (drop)<br>
>>> > the<br>
>>> > debug info. Similar to other IR upgrading, we upgrade first, then<br>
>>> > verify the<br>
>>><br>
>>> The part that you're dealing with is not part of the debug info<br>
>>> though. It's the normal IR sequence.<br>
>>><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<br>
>>> > always.<br>
>>> ><br>
>>><br>
>>> 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>
>><br>
>><br>
>> This is a general problem with bad IR (it is not specific to dropping the<br>
>> debug info), what should we do if we have bad IR and the IR is also in an<br>
>> old format?<br>
>> Should we auto-upgrade first? What we do currently is to auto-upgrading<br>
>> during loading the files.<br>
>> Do you have any specific suggestion?<br>
><br>
><br>
> Currently, we auto-upgrade first, then run verifier to catch errors after<br>
> upgrading. I think that is the right way to do it.<br>
> We ignore bad IR during upgrading, but the verifier will catch errors later<br>
> on.<br>
><br>
> I can't think of other orders that work better.<br>
><br>
<br>
</div></div>Upgrade the IR, verify the IR, strip+verify the debug info is what I<br>
was thinking. Move debug info verify out of the IR verify.<br></blockquote><div><br></div><div><br></div><div>To me stripping debug info is one kind of upgrading the IR, I don't quite get why stripping debug info needs special treatment from other upgrading.</div>
<div><br></div><div>If we want to add a separate pass for stripping, we have to modify sources for executables that can read in a .bc|.ll file: llc, opt, llvm_as, lto, llvm_dis ...</div><div><br></div><div>What we gain is that with bad IR, stripping after verifying will exit earlier at the verifying phase.</div>
<div><br></div><div>Manman</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><font color="#888888"><br>
-eric<br>
</font></span><div class=""><div class="h5"><br>
<br>
> Manman<br>
><br>
>><br>
>> Manman<br>
>><br>
>>><br>
>>> -eric<br>
>>><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>
>>> >> >> >> ><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>
>>> >> >> >> > ==============================================================================<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 !=<br>
>>> >> >> >> > 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>
>>> >> >> >> ><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<br>
>>> >> >> >> > 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>
>>> >> >> >> ><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">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>
>><br>
>><br>
><br>
</div></div></blockquote></div><br></div></div>