[PATCH] D13263: Addition of __attribute__((pass_object_size)) to Clang

[Richard Smith via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

OK, a couple of trivial changes then I think this is fine to commit.


================
Comment at: include/clang/Basic/AttrDocs.td:351-355
@@ +350,7 @@
+
+* Only one use of ``pass_object_size`` is allowed per parameter.
+
+* It is an error to take the address of a function with ``pass_object_size`` on
+  any of its parameters. If you wish to do this, you can create an overload
+  without pass_object_size on any parameters.
+
----------------
Not done? =)

================
Comment at: lib/AST/ExprConstant.cpp:6507-6509
@@ -6506,5 +6544,1 @@
-    // handle all cases where the expression has side-effects.
-    // Likewise, if Type is 3, we must handle this because CodeGen cannot give a
-    // conservatively correct answer in that case.
-    if (E->getArg(0)->HasSideEffects(Info.Ctx) || Type == 3)
       return Success((Type & 2) ? 0 : -1, E);
----------------
I'd view the GCC documentation as more what you'd call "guidelines" than actual rules. The key part here is that side-effects in the operand of `__builtin_object_size` do not occur; I think the fact that GCC always returns `-1` or `0` if there are side-effects is an implementation detail of GCC's approach rather than a formal part of the design, and we should not be required to treat `__builtin_object_size` as a constant if it's applied to an expression with side-effects. If CodeGen can compute the object size, it should be permitted to do so even if side-effects are present, so long as those side-effects do not actually occur.

However, I think this change is doing enough already, so let's go ahead with things as they are; we can consider changing this at a later date if we are so inclined.

================
Comment at: lib/AST/MicrosoftMangle.cpp:1844-1857
@@ -1843,4 +1843,16 @@
     // Happens for function pointer type arguments for example.
-    for (const QualType &Arg : Proto->param_types())
-      mangleArgumentType(Arg, Range);
+    for (unsigned I = 0, E = Proto->getNumParams(); I != E; ++I) {
+      mangleArgumentType(Proto->getParamType(I), Range);
+      // Mangle each pass_object_size parameter as if it's a paramater of enum
+      // type passed directly after the parameter with the pass_object_size
+      // attribute. The aforementioned enum's name is __pass_object_size, and we
+      // pretend it resides in a top-level namespace called __ext.
+      //
+      // FIXME: Is there a defined extension notation for the MS ABI, or is it
+      // necessary to just cross our fingers and hope this type+namespace
+      // combination doesn't conflict with anything?
+      if (D)
+        if (auto *P = D->getParamDecl(I)->getAttr<PassObjectSizeAttr>())
+          Out << "W4__pass_object_size" << P->getType() << "@__ext@@";
+    }
     // <builtin-type>      ::= Z  # ellipsis
----------------
majnemer wrote:
> This seems fine to me, I'd change the namespace to something like `__clang` though.
Yes, `__clang` would make more sense to me too.


http://reviews.llvm.org/D13263