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

[George Burgess IV via cfe-commits]

george.burgess.iv 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.
+  ///
----------------
rsmith wrote:
> Looks OK, but you have an undesirable "either" in the comment now.
Danke

================
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);
----------------
rsmith wrote:
> 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.
They seem sufficiently different to me. GCC's docs for `__builtin_object_size` say "__builtin_object_size never evaluates its arguments for side-effects. If there are any side-effects in them, it returns (size_t) -1 for type 0 or 1 and (size_t) 0 for type 2 or 3," so this is more a feature of the builtin itself than an artifact of how we generate code. That said, I can't tell if our partial relaxation of this restriction is intentional or not. :)

FWIW, the static analyzer also apparently depends on us returning `Success` here if there are side-effects; tests fail if I move this check to CodeGen.

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

================
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()) {
----------------
rsmith wrote:
> 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?
> Any reason why you factor out HasPassObjSize here and not in the previous (essentially identical) function? 

Artifact of not cleaning up completely after playing with different ways to do this -- sorry.

> And can you factor this out into a getPassObjectSizeParamIndex function or something, to avoid some of the duplication between this and the previous function?

I tried merging the functions entirely; if we don't like the result, I'm happy to go back to the way they were and just factor out the `find_if` bit. :)


http://reviews.llvm.org/D13263