[llvm-commits] Newest LandingPad Patch

[Bill Wendling]

Hi Duncan,

Thanks for the review. The possibility of 'bitcasts' makes this annoying. If I cannot guarantee that they will always be Constant*s or Function*s, I'll just change this to use Value*s instead.

-bw

On Aug 8, 2011, at 2:46 AM, Duncan Sands wrote:

> Hi Bill,
> 
>> +  /// getPersonalityFn - Get the personality function associated with this
>> +  /// landing pad.
>> +  const Function *getPersonalityFn() const {
>> +    return cast<Function>(getOperand(0));
>> +  }
> 
> I think you should change this to
>   return cast<Function>(getOperand(0)->stripPointerCasts());
> 
> That's because you can't avoid sometimes getting a bitcast here.  For example,
> suppose you have two modules that both declare __gxx_personality_v0 and use it
> in a landingpad instruction, but they use a slightly different prototype for
> __gxx_personality_v0.  When you link the bitcode from the two modules together,
> the prototype difference will be resolved by replacing one __gxx_personality_v0
> with a bitcast of the other.  This results in landingpad instructions with a
> bitcast of a function for the personality function operand, and then... boom!
> 
> Likewise, you can't assume that typeinfos are global variables, you have to
> allow for the possibility that they are bitcasts of global variables.  I didn't
> notice any place that assumes they are global variables, so hopefully everything
> is OK for them.
> 
>> --- lib/VMCore/AsmWriter.cpp	(revision 136744)
>> +++ lib/VMCore/AsmWriter.cpp	(working copy)
>> @@ -1735,6 +1735,31 @@
>>     writeOperand(I.getOperand(1), true);
>>     for (const unsigned *i = IVI->idx_begin(), *e = IVI->idx_end(); i != e; ++i)
>>       Out << ", " << *i;
>> +  } else if (const LandingPadInst *LPI = dyn_cast<LandingPadInst>(&I)) {
>> +    Out << ' ';
>> +    TypePrinter.print(I.getType(), Out);
>> +    Out << " personality ";
>> +    writeOperand(LPI->getPersonalityFn(), true); Out << '\n';
> 
> If the personality operand was a bitcast then here you would fail to output the
> bitcast.  You should probably just write out operand 0.
> 
>> @@ -3513,6 +3514,57 @@
>>   return AteExtraComma ? InstExtraComma : InstNormal;
>> }
>> 
>> +/// ParseLandingPad
>> +///   ::= 'landingpad' Type 'personality' TypeAndValue 'cleanup'? Clause+
>> +/// Clause
>> +///   ::= 'catch' TypeAndValue
>> +///   ::= 'filter' TypeAndValue*
>> +bool LLParser::ParseLandingPad(Instruction *&Inst, PerFunctionState &PFS) {
>> +  Type *Ty = 0; LocTy TyLoc;
>> +  Value *PersFn; LocTy PersFnLoc;
>> +  LocTy LPLoc = Lex.getLoc();
>> +
>> +  if (ParseType(Ty, TyLoc) ||
>> +      ParseToken(lltok::kw_personality, "expected 'personality'") ||
>> +      ParseTypeAndValue(PersFn, PersFnLoc, PFS))
>> +    return true;
>> +
>> +  LandingPadInst *LP = LandingPadInst::Create(Ty, cast<Function>(PersFn), 0);
> 
> Here you will blow up if the personality function is a bitcast.  Also, you
> should probably write the parsing in such a way as to output a helpful message
> rather than crashing (as cast<Function> would) if someone provides invalid
> input.
> 
>> +  LP->setCleanup(EatIfPresent(lltok::kw_cleanup));
>> +
>> +  while (Lex.getKind() == lltok::kw_catch || Lex.getKind() == lltok::kw_filter){
>> +    if (Lex.getKind() == lltok::kw_catch) {
>> +      ParseToken(lltok::kw_catch, "expected 'catch'");
>> +
>> +      Value *V; LocTy VLoc;
>> +      if (ParseTypeAndValue(V, VLoc, PFS)) {
>> +        delete LP;
>> +        return true;
>> +      }
>> +      LP->addClause(LandingPadInst::Catch, cast<Constant>(V));
> 
> This should probably output an error rather than crashing if V is not a
> constant.
> 
>> +    } else {
>> +      ParseToken(lltok::kw_filter, "expected 'filter'");
>> +      SmallVector<Constant*, 2> Filters;
>> +
>> +      if (Lex.getKind() == lltok::Type) {
>> +        do {
>> +          Value *V; LocTy VLoc;
>> +          if (ParseTypeAndValue(V, VLoc, PFS)) {
>> +            delete LP;
>> +            return true;
>> +          }
>> +          Filters.push_back(cast<Constant>(V));
> 
> This should probably output an error rather than crashing if V is not a
> constant.
> 
>> --- lib/Bitcode/Reader/BitcodeReader.cpp	(revision 136744)
>> +++ lib/Bitcode/Reader/BitcodeReader.cpp	(working copy)
>> @@ -2543,6 +2543,45 @@
>>       break;
>>     }
>> 
>> +    case bitc::FUNC_CODE_INST_LANDINGPAD: {
>> +      // LANDINGPAD: [ty, val, val, num, (id0,val0 ...)?]
>> +      unsigned Idx = 0;
>> +      if (Record.size() < 4)
>> +        return Error("Invalid LANDINGPAD record");
>> +      Type *Ty = getTypeByID(Record[Idx++]);
>> +      if (!Ty) return Error("Invalid LANDINGPAD record");
>> +      Value *PersFn = 0;
>> +      if (getValueTypePair(Record, Idx, NextValueNo, PersFn))
>> +        return Error("Invalid LANDINGPAD record");
>> +
>> +      bool IsCleanup = !!Record[Idx++];
>> +      unsigned NumClauses = Record[Idx++];
>> +      LandingPadInst *LP = LandingPadInst::Create(Ty, cast<Function>(PersFn),
> 
> Kaboom if a bitcast of a function rather than a function.  Here too you should
> probably try to output a message rather than crashing if you get some strange
> input.
> 
>> Index: lib/Bitcode/Writer/BitcodeWriter.cpp
>> ===================================================================
>> --- lib/Bitcode/Writer/BitcodeWriter.cpp	(revision 136744)
>> +++ lib/Bitcode/Writer/BitcodeWriter.cpp	(working copy)
>> @@ -1166,6 +1166,26 @@
>>     break;
>>   }
>> 
>> +  case Instruction::LandingPad: {
>> +    const LandingPadInst &LP = cast<LandingPadInst>(I);
>> +    Code = bitc::FUNC_CODE_INST_LANDINGPAD;
>> +    Vals.push_back(VE.getTypeID(LP.getType()));
>> +    PushValueAndType(LP.getPersonalityFn(), InstID, Vals, VE);
> 
> This would drop any bitcast on the personality function.
> 
> Ciao, Duncan.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits