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

[Eric Christopher]

On Thu, Dec 5, 2013 at 1:52 PM, Manman Ren <manman.ren at gmail.com> wrote:
>
>
>
> On Thu, Dec 5, 2013 at 1:11 PM, Eric Christopher <echristo at gmail.com> wrote:
>>
>> 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.
>
>
> The change is on getModuleFlagsMetadata to make sure it does not seg fault.
> There is no change required on passes that call getModuleFlagsMetadata.
>
> I could have separated the change from dropping debug info patches. It is
> required to
> make sure "llc module-flags-1.ll" does not seg fault.
>

Hrm. OK, I thought originally that you were working around a
particular flag in the file and not the general case.

It definitely needs a better comment on the structure of the flag that
we're actually expecting.

>>
>> >>
>> >>
>> >>>
>> >>> >> 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.
>
>
>
> 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.
>
> 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 ...
>
> What we gain is that with bad IR, stripping after verifying will exit
> earlier at the verifying phase.
>

Fair enough. My actual concern was the first part and that it
highlighted a change that had to be made otherwise.

-eric

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