[PATCH] Implement the __builtin_call_with_static_chain GNU extension.

Peter Collingbourne peter at pcc.me.uk
Mon Dec 8 16:49:54 PST 2014


================
Comment at: include/clang/AST/Expr.h:4880
@@ +4879,3 @@
+      : Expr(CallWithStaticChainExprClass, Call->getType(),
+             Call->getValueKind(), OK_Ordinary, Call->isTypeDependent(),
+             Call->isValueDependent(), Call->isInstantiationDependent() ||
----------------
rsmith wrote:
> It would seem better to inherit the object kind from `Call` here, even though in practice a call should always be `OK_Ordinary`.
Done.

================
Comment at: include/clang/Basic/TokenKinds.def:366
@@ -365,2 +365,3 @@
 KEYWORD(__PRETTY_FUNCTION__         , KEYALL)
+KEYWORD(__builtin_call_with_static_chain, KEYALL)
 
----------------
rsmith wrote:
> Why is this modeled as a keyword rather than as a builtin function?
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.

================
Comment at: include/clang/CodeGen/CGFunctionInfo.h:366
@@ -362,3 +365,3 @@
   unsigned HasRegParm : 1;
-  unsigned RegParm : 4;
+  unsigned RegParm : 3;
 
----------------
rsmith wrote:
> Is this large enough? What happens if we overflow?
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?

================
Comment at: lib/CodeGen/CGCall.cpp:86
@@ -85,3 +85,3 @@
   return arrangeLLVMFunctionInfo(FTNP->getReturnType().getUnqualifiedType(),
-                                 false, None, FTNP->getExtInfo(),
+                                 false, false, None, FTNP->getExtInfo(),
                                  RequiredArgs(0));
----------------
rsmith wrote:
> Please add an enum for this flag rather than having two mysterious bool flags in a row.
I've added comments to make it clear what the various flags pertain to. Or I can do the enum thing if you prefer.

================
Comment at: test/CodeGenCXX/cwsc.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm -o - %s | FileCheck -check-prefix=CHECK32 %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck -check-prefix=CHECK64 %s
----------------
rsmith wrote:
> Please use a less abbreviated file name for these test files.
Done.

http://reviews.llvm.org/D6332






More information about the cfe-commits mailing list