[llvm] r196158 - Debug Info: drop debug info via upgrading path if version number does not match.
Manman Ren
manman.ren at gmail.com
Thu Dec 5 13:52:23 PST 2013
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.
> >>
> >>
> >>>
> >>> >> 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.
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
> >>> >> >> >
> >>> >> >> >
> >>> >> >
> >>> >> >
> >>> >
> >>> >
> >>
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131205/0f792691/attachment.html>
More information about the llvm-commits
mailing list