[llvm] r196158 - Debug Info: drop debug info via upgrading path if version number does not match.
Manman Ren
manman.ren at gmail.com
Tue Dec 3 12:06:45 PST 2013
On Mon, Dec 2, 2013 at 4:48 PM, Eric Christopher <echristo at gmail.com> wrote:
> On Mon, Dec 2, 2013 at 4:34 PM, Manman Ren <manman.ren at gmail.com> wrote:
> >
> >
> >
> > On Mon, Dec 2, 2013 at 4:26 PM, Eric Christopher <echristo at gmail.com>
> wrote:
> >>
> >> On Mon, Dec 2, 2013 at 4:21 PM, Manman Ren <manman.ren at gmail.com>
> wrote:
> >> >
> >> >
> >> >
> >> > On Mon, Dec 2, 2013 at 2:07 PM, Eric Christopher <echristo at gmail.com>
> >> > wrote:
> >> >>
> >> >> > Add a helper function getDebugInfoVersionFromModule to return the
> >> >> > debug
> >> >> > info
> >> >> > version number for a module.
> >> >> >
> >> >>
> >> >> I think this should be getDebugMetadataVersionFromModule since this
> >> >> could be easily confused with the dwarf version that someone has
> asked
> >> >> for.
> >> >
> >> >
> >> > Done in r196172.
> >> >>
> >> >>
> >> >> > "Verifier/module-flags-1.ll" checks for verification errors.
> >> >> > It will seg fault when calling getDebugInfoVersionFromModule
> because
> >> >> > of
> >> >> > the
> >> >> > incorrect format for module flags in the testing case. We make
> >> >> > getModuleFlagsMetadata more robust by checking for error
> conditions.
> >> >> >
> >> >>
> >> >> I'm not sure what's going on here, can you elaborate?
> >> >
> >> >
> >> > We have the code path and the verification path. Code path should not
> >> > seg
> >> > fault when the format of a module flag is incorrect (as in the testing
> >> > case
> >> > Verifier/module-flags-1.ll).
> >> > module-flags-1.ll tries to make sure that the verifier dumps the right
> >> > error
> >> > message when the format of a module flag is incorrect.
> >> >
> >> > Before this patch, the code path will seg fault because we don't
> perform
> >> > error checking in the code path as shown below.
> >> >
> >>
> >> You seem to be hard coding certain operations leading me to believe
> >> there's some specific format here that you're looking for, but it's
> >> not obvious what it is
> >
> >
> > 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.
>
> >
> >>
> >> - are you saying that if you turn off the
> >> verifier we'll crash on certain types of bitcode files that normally
> >> we wouldn't be able to read anyhow since we'd fail verification?
> >
> >
> > No, it does not matter whether the verifier is on or off. The code path
> > should not seg fault when the input bit code has incorrect module flag.
> >
> > The reason my patch exposes the seg fault is that we parse the module
> flag
> > before
> > the verifier and the testing case will seg fault before getting the
> > verification error.
> >
>
> 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 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 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.
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/20131203/ba0c1bc7/attachment.html>
More information about the llvm-commits
mailing list