[llvm] r196158 - Debug Info: drop debug info via upgrading path if version number does not match.

[Eric Christopher]

On Tue, Dec 3, 2013 at 3:12 PM, Manman Ren <manman.ren at gmail.com> wrote:
>
>
>
> 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.
>

Ignoring or throwing away. I.e. what if another pass used a module
flag to do the same sort of thing and it was then seen as invalid?
Would they, then, have to work around it the same way you have? I
think so and so it's not a good long term solution.

>>
>>
>>>
>>> >> 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.
>

Upgrade the IR, verify the IR, strip+verify the debug info is what I
was thinking. Move debug info verify out of the IR verify.

-eric


> 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
>>> >> >> >
>>> >> >> >
>>> >> >
>>> >> >
>>> >
>>> >
>>
>>
>