This is an archive of the discontinued LLVM Phabricator instance.

Implement the __builtin_call_with_static_chain GNU extension.
ClosedPublic

Authored by pcc on Nov 19 2014, 6:26 PM.

Details

Summary

The extension has the following syntax:

__builtin_call_with_static_chain(Call, Chain)
where Call must be a function call expression and Chain must be of pointer type

This extension performs a function call Call with a static chain pointer
Chain passed to the callee in a designated register. This is useful for
calling foreign language functions whose ABI uses static chain pointers
(e.g. to implement closures).

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 16408.Nov 19 2014, 6:26 PM
pcc retitled this revision from to Implement the __builtin_call_with_static_chain GNU extension..
pcc updated this object.
pcc edited the test plan for this revision. (Show Details)
pcc added a reviewer: rsmith.
pcc added a subscriber: Unknown Object (MLST).
rsmith added inline comments.Dec 8 2014, 3:15 PM
include/clang/AST/Expr.h
4880 ↗(On Diff #16408)

It would seem better to inherit the object kind from Call here, even though in practice a call should always be OK_Ordinary.

include/clang/Basic/TokenKinds.def
366 ↗(On Diff #16408)

Why is this modeled as a keyword rather than as a builtin function?

include/clang/CodeGen/CGFunctionInfo.h
366 ↗(On Diff #16408)

Is this large enough? What happens if we overflow?

lib/CodeGen/CGCall.cpp
86 ↗(On Diff #16408)

Please add an enum for this flag rather than having two mysterious bool flags in a row.

test/CodeGenCXX/cwsc.cpp
1 ↗(On Diff #16408)

Please use a less abbreviated file name for these test files.

pcc updated this revision to Diff 17063.Dec 8 2014, 4:49 PM
  • Use call expr object kind
  • Rename test files
  • Annotate various calls to arrangeLLVMFunctionInfo with argument comments
pcc added inline comments.
include/clang/AST/Expr.h
4880 ↗(On Diff #16408)

Done.

include/clang/Basic/TokenKinds.def
366 ↗(On Diff #16408)

I thought it would be necessary to introduce a new AST node because of the custom type checking (I got this impression from some of the newer additions to Expr.h which do indeed model builtin functions as AST nodes), but I somehow overlooked that builtin functions can have custom type checking. I agree that this should have been modeled as a builtin function. If you like, I'll see if I can revise this patch to do so.

include/clang/CodeGen/CGFunctionInfo.h
366 ↗(On Diff #16408)

I thought it would be large enough because Sema::CheckRegparmAttr limits this value to TargetInfo::RegParmMax, and include/clang/Basic/TargetInfo.h contains this assertion:

assert(RegParmMax < 7 && "RegParmMax value is larger than AST can handle");

presumably because FunctionType::ExtInfo allocates 3 bits for regparm.

But when I looked through lib/Basic/Targets.cpp I found that AArch64 is setting RegParmMax to 8. As a result, the regparm attribute does not work at all in an assertions-enabled build when targeting that architecture, and would presumably break with values greater than 7 in an assertions-disabled build. Tim, you committed this setting to Clang in r205100, would you be able to confirm that it is correct, please?

lib/CodeGen/CGCall.cpp
86 ↗(On Diff #16408)

I've added comments to make it clear what the various flags pertain to. Or I can do the enum thing if you prefer.

test/CodeGenCXX/cwsc.cpp
1 ↗(On Diff #16408)

Done.

rsmith added inline comments.Dec 8 2014, 5:34 PM
include/clang/Basic/TokenKinds.def
366 ↗(On Diff #16408)

Yes, please do make this a normal builtin function; that should remove a lot of the boilerplate (template instantiation and serialization/deserialization in particular).

lib/CodeGen/CGCall.cpp
86 ↗(On Diff #16408)

This function is far from being the worst offender, and a comment is sufficient to make the code readable. If you feel motivated to make another change, I would prefer an enum, but not strongly.

375–378 ↗(On Diff #16408)

Please pick either argTypes and args, or ArgTypes and Args.

pcc updated this revision to Diff 17094.Dec 9 2014, 1:23 PM
  • Model as a normal builtin function
  • Minor cleanup
include/clang/Basic/TokenKinds.def
366 ↗(On Diff #16408)

Done.

lib/CodeGen/CGCall.cpp
375–378 ↗(On Diff #16408)

Done.

rsmith added inline comments.Dec 9 2014, 6:10 PM
include/clang/Basic/Builtins.def
500 ↗(On Diff #17094)

This should presumably also be marked "n"; it itself never throws (its argument might, but that's a separate issue).

lib/Sema/SemaChecking.cpp
177–184 ↗(On Diff #17094)

It might be better to perform conversions as if to a parameter of type void * here (that would be detectable in C++ via conversion operators, and would do the right thing for null pointer constants). What does GCC do in those cases?

187 ↗(On Diff #17094)

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?

pcc updated this revision to Diff 17137.Dec 10 2014, 7:47 PM
  • Mark builtin as nothrow
include/clang/Basic/Builtins.def
500 ↗(On Diff #17094)

Done.

lib/Sema/SemaChecking.cpp
177–184 ↗(On Diff #17094)

GCC appears to only support the extension in C mode, not in C++. It appears to check the second argument's type rather than attempting any implicit conversions, so null pointer constants are rejected. I couldn't identify any cases (in C) where we reject a second argument that GCC accepts or vice versa.

187 ↗(On Diff #17094)

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

rsmith added inline comments.Dec 11 2014, 11:35 AM
lib/Sema/SemaChecking.cpp
187 ↗(On Diff #17094)

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?)

pcc added inline comments.Dec 11 2014, 4:54 PM
lib/Sema/SemaChecking.cpp
187 ↗(On Diff #17094)

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.

pcc updated this revision to Diff 17242.Dec 12 2014, 12:13 PM
  • Assign a correct type to the builtin by creating a new implicit cast
rsmith accepted this revision.Dec 12 2014, 2:42 PM
rsmith edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Dec 12 2014, 2:42 PM
This revision was automatically updated to reflect the committed changes.