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

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/AST/Expr.h:631-634
@@ -630,1 +630,6 @@
 
+  /// \brief If the current Expr is either a pointer, this will try to
+  /// statically determine the number of bytes available where the pointer is
+  /// pointing.  Returns true if all of the above holds and we were able to
+  /// figure out the size, false otherwise.
+  ///
----------------
Looks OK, but you have an undesirable "either" in the comment now.

================
Comment at: include/clang/Basic/AttrDocs.td:272
@@ +271,3 @@
+  using ``__asm__("foo")`` to explicitly name your functions, thus preserving
+  your ABI; also, non-overloadable C functions with pass_object_size are
+  not mangled.
----------------
Missing `` around pass_object_size here.

================
Comment at: include/clang/Basic/AttrDocs.td:317
@@ +316,3 @@
+candidates are otherwise equally good, then the overload with one or more
+parameters with pass_object_size is preferred. This implies that the choice
+between two identical overloads both with ``pass_object_size`` on one or more
----------------
And here.

================
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.
+
----------------
And here.

================
Comment at: include/clang/Basic/AttrDocs.td:359
@@ +358,3 @@
+  are not pointers. Additionally, any parameter that ``pass_object_size`` is
+  applied to must be marked const at its function's definition.
+  }];
----------------
And around "const" here, maybe.

================
Comment at: include/clang/Sema/Initialization.h:832
@@ +831,3 @@
+    /// \brief Trying to take the address of a function that doesn't support
+    /// having its address taken
+    FK_AddressOfUnaddressableFunction,
----------------
Add a period at the end of this comment.

================
Comment at: lib/AST/ExprConstant.cpp:6507-6509
@@ -6506,5 +6545,3 @@
     // 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 don't disagree, but it seems logically similar to the `HasSideEffects` case, which is still here. Maybe that should be moved to `CodeGen` too.

================
Comment at: lib/AST/ExprConstant.cpp:9534
@@ +9533,3 @@
+
+  Result = llvm::APInt(/*NumBits=*/64, Size, /*IsSigned=*/true);
+  return true;
----------------
I would expect this to produce a `size_t`, rather than an arbitrary-seeming signed 64-bit integer. It also seems suspicious to be converting a uint64_t to a signed `APInt` here.

================
Comment at: lib/AST/ItaniumMangle.cpp:2205
@@ +2204,3 @@
+
+    if (FD != nullptr) {
+      if (auto *Attr = FD->getParamDecl(I)->getAttr<PassObjectSizeAttr>()) {
----------------
`if (FD)` is more common in these parts.

================
Comment at: lib/CodeGen/CGCall.cpp:110
@@ +109,3 @@
+  // pass_object_size. So, we preallocate for the common case.
+  prefix.reserve(FPT->getNumParams());
+
----------------
Given that this appends, `reserve(prefix.size() + FPT->getNumParams())` seems better.

================
Comment at: lib/CodeGen/CGCall.cpp:2847-2849
@@ -2803,4 +2846,5 @@
       EmitCallArg(Args, *Arg, ArgTypes[I]);
+      MaybeEmitImplicitObjectSize(I, *Arg);
       EmitNonNullArgCheck(Args.back().RV, ArgTypes[I], (*Arg)->getExprLoc(),
                           CalleeDecl, ParamsToSkip + I);
     }
----------------
These seem to be in the wrong order, and the call to `Args.back()` will misbehave if a parameter is marked with `__attribute__((nonnull, pass_object_size(N)))`. Swap these calls over.

================
Comment at: lib/CodeGen/CGCall.cpp:2862-2864
@@ -2817,4 +2861,5 @@
     EmitCallArg(Args, *Arg, ArgTypes[I]);
+    MaybeEmitImplicitObjectSize(I, *Arg);
     EmitNonNullArgCheck(Args.back().RV, ArgTypes[I], (*Arg)->getExprLoc(),
                         CalleeDecl, ParamsToSkip + I);
   }
----------------
Likewise.

================
Comment at: lib/Sema/SemaOverload.cpp:8832-8834
@@ +8831,5 @@
+
+  auto HasPassObjSize = std::mem_fn(&ParmVarDecl::hasAttr<PassObjectSizeAttr>);
+
+  auto I = std::find_if(FD->param_begin(), FD->param_end(), HasPassObjSize);
+  if (I == FD->param_end()) {
----------------
Any reason why you factor out `HasPassObjSize` here and not in the previous (essentially identical) function? And can you factor this out into a `getPassObjectSizeParamIndex` function or something, to avoid some of the duplication between this and the previous function?


http://reviews.llvm.org/D13263