[PATCH] Implement the __builtin_call_with_static_chain GNU extension.

[Richard Smith]

On Thu, Dec 11, 2014 at 4:55 PM, Peter Collingbourne <peter at pcc.me.uk>
wrote:

> ================
>
> Comment at: lib/Sema/SemaChecking.cpp:187
>
> @@ +186,3 @@
>
> +  BuiltinCall->setType(CE->getType());
>
> +  BuiltinCall->getCallee()->setType(CE->getCallee()->getType());
>
> +  BuiltinCall->setValueKind(CE->getValueKind());
>
> ----------------
>
> rsmith wrote:
>
> > pcc wrote:
>
> > > rsmith wrote:
>
> > > > This can't be right: multiple calls to the builtin will share the
> same callee and will want different types here. Just remove this line?
>
> > > I found this to be necessary in cases where the function returns an
> lvalue; without it, I was getting assertion failures.
>
> > >
>
> > > I thought this might be an issue as well, until I looked at the AST
> representation of builtin calls:
>
> > >
>
> > > ```
>
> > > |     |-CallExpr 0x4310ae0 <col:3, col:42> 'int' lvalue
>
> > > |     | |-ImplicitCastExpr 0x4310ac8 <col:3> 'int &(*)(void)'
> <BuiltinFnToFnPtr>
>
> > > |     | | `-DeclRefExpr 0x4310960 <col:3> '<builtin fn type>' Function
> 0x43108c0 '__builtin_call_with_static_chain' 'void ()'
>
> > > |     | |-CallExpr 0x4310a50 <col:36, col:38> 'int' lvalue
>
> > > |     | | `-ImplicitCastExpr 0x4310a38 <col:36> 'int &(*)(void)'
> <FunctionToPointerDecay>
>
> > > |     | |   `-DeclRefExpr 0x43109e0 <col:36> 'int &(void)' lvalue
> Function 0x42ca430 'f' 'int &(void)'
>
> > > |     | `-ImplicitCastExpr 0x4310b18 <col:41> 'int &(*)(void)'
> <FunctionToPointerDecay>
>
> > > |     |   `-DeclRefExpr 0x4310a78 <col:41> 'int &(void)' lvalue
> Function 0x42ca430 'f' 'int &(void)'
>
> > > ```
>
> > >
>
> > > So we are only overwriting the type of the `BuiltinFnToFnPtr` cast.
> This seems to be not too bad (except that the argument types are wrong; I
> should fix that, but it didn't seem to cause any issues).
>
> > That's still not correct. Practically, we could share the same
> `ImplicitCastExpr` between multiple calls to the builtin (through template
> instantiation, for instance), and theoretically, it's wrong to have a
> `BuiltinFnToFnPtr` cast that performs some conversion other than
> function-to-pointer decay. One reasonable thing to do here would be to
> create a new `ImplicitCastExpr` node to represent the conversion between
> function pointer types. (Another would be to teach whatever code was
> asserting that it shouldn't assume the call return type matches that of the
> callee for a call to a builtin -- I guess this is probably the
> ExprClassification.cpp code?)
>
> > Practically, we could share the same ImplicitCastExpr between multiple
> calls to the builtin (through template instantiation, for instance)
>
> Fair point. I couldn't get this to trigger during template instantiation,
> but I suppose it could happen in other cases or if we change how template
> instantiation works.
>
> > theoretically, it's wrong to have a BuiltinFnToFnPtr cast that performs
> some conversion other than function-to-pointer decay.
>
>
>
> I don't understand why. We get to decide what the semantics of the various
> implicit casts are, and it seems reasonable to me for `BuiltinFnToFnPtr` to
> also represent the conversion of a generic builtin to a specific type. I
> also couldn't find any existing code that depends on `BuiltinFnToFnPtr`
> being a strict function-to-pointer decay conversion.


Hmm, you're absolutely right, I was mixing this up with the more usual
CK_FunctionToPointerDecay. I think it'd be fine to create a new
ImplicitCastExpr with the right destination type here.


>
> > One reasonable thing to do here would be to create a new
> ImplicitCastExpr node to represent the conversion between function pointer
> types.
>
>
>
> Agreed that the code should create a new `ImplicitCastExpr` instead of
> trying to change the existing one, but I think you also meant that we
> should introduce a new cast kind for this case, which I don't think is
> necessary.


I meant to use something like CK_BitCast, not to introduce a new cast kind.
But you've convinced me that it's fine to use CK_BuiltinFnToFnPtr. =)


>
> > Another would be to teach whatever code was asserting that it shouldn't
> assume the call return type matches that of the callee for a call to a
> builtin
>
>
>
> I'd feel more comfortable representing this with a cast, as I'd rather not
> carve out an exception to a reasonable AST invariant.
>
>
>
> > I guess this is probably the ExprClassification.cpp code?
>
>
>
> Yes.
>
>
>
> http://reviews.llvm.org/D6332
>
>
>
> EMAIL PREFERENCES
>
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141211/cbcb01e4/attachment.html>