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