[llvm-commits] Newest LandingPad Patch

[Duncan Sands]

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.