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