On Thu, Dec 11, 2014 at 4:55 PM, Peter Collingbourne <[email protected]> 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/ > > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
