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

George Burgess IV via cfe-commits cfe-commits at lists.llvm.org
Fri Nov 13 16:34:59 PST 2015


george.burgess.iv added inline comments.

================
Comment at: include/clang/AST/Expr.h:631-634
@@ -630,1 +630,6 @@
 
+  /// tryEvaluateObjectSize - If the current Expr is a pointer, this will try to
+  /// statically determine how many bytes remain in the object this pointer is
+  /// pointing to.
+  bool tryEvaluateObjectSize(llvm::APSInt &Result, ASTContext &Ctx,
+                             unsigned Type) const;
----------------
rsmith wrote:
> ... and what if the current expression isn't a pointer? Is that a precondition (function might assert / crash) or does it result in 'return false'?
> 
> Also, don't use `"/// FunctionName - [...]"` in new code, use `"/// \brief [...]"` instead.
> ... and what if the current expression isn't a pointer? Is that a precondition (function might assert / crash) or does it result in 'return false'?

Fixed-ish. Do you think the new comment is sufficient, or would you like to see each parameter documented for consistency?

> Also, don't use "/// FunctionName - [...]" in new code, use "/// \brief [...]" instead

Thanks!

================
Comment at: include/clang/AST/Type.h:3110-3112
@@ -3106,2 +3109,5 @@
 
+  /// Whether this function has pass_object_size attribute(s) on its parameters
+  unsigned HasPassObjectSizeParams : 1;
+
   // ParamInfo - There is an variable size array after the class in memory that
----------------
rsmith wrote:
> Seems nicer to put this right after `HasAnyConsumedParams`. (I'm also surprised to find that we have a spare bit here...)
No longer an issue. (I was too.)

================
Comment at: include/clang/AST/Type.h:3131-3134
@@ -3124,3 +3130,6 @@
 
-  friend class ASTContext;  // ASTContext creates these.
+  // PassObjectSizeParams - A variable size array, following ConsumedParameters
+  // and of length NumParams, holding flags indicating which parameters have the
+  // pass_object_size attribute. This only appears if HasPassObjectSizeParams is
+  // true.
 
----------------
rsmith wrote:
> Do we really want just flags here rather than the `type` value? Should `int (void * __attribute__((pass_object_size(1))))` and `int (void *__attribute((pass_object_size(2))))` be the same type?
No longer an issue

================
Comment at: include/clang/Basic/AttrDocs.td:354-365
@@ +353,14 @@
+
+* Because the size is passed at runtime, ``CallFoo`` below will always call
+  A:
+
+  .. code-block:: c++
+
+    int Foo(int n) __attribute__((enable_if(n > 0, ""))); // function A
+    int Foo(int n) __attribute__((enable_if(n <= 0, ""))); // function B
+    int CallFoo(const void *p __attribute__((pass_object_size(0))))
+        __attribute__((noinline)) {
+      return Foo(__builtin_object_size(p, 0));
+    }
+  }];
+}
----------------
rsmith wrote:
> I would expect this to result in an error: neither A nor B is viable because their conditions are non-constant.
I agree; I have no clue why I thought this was sane. ¯\_(ツ)_/¯

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2068
@@ -2064,1 +2067,3 @@
 def err_attribute_pointers_only : Error<warn_attribute_pointers_only.Text>;
+def err_attribute_constant_pointers_only : Error<
+  "%0 attribute only applies to constant pointer arguments">;
----------------
aaron.ballman wrote:
> Instead of making a new diagnostic, I think it better to modify warn_attribute_pointers_only to use a %select statement (then err_attribute_pointers_only also gets updated). I would separate that into a prequel patch since it's likely to introduce a fair amount of unrelated changes to this patch.
There were only four small distinct changes, so I just rolled it all into this patch; I'm happy to pull it out into its own if we feel like that's a bit too much clutter.

================
Comment at: lib/AST/ExprConstant.cpp:6505-6507
@@ -6504,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:
> Where is the handling for `Type == 3` now?
It was moved to CodeGenFunction::emitBuiltinSizeOf. I feel it fits better there because it's more of an implementation detail of @llvm.objectsize; I'm happy to move the check back here if you disagree.

================
Comment at: lib/AST/ExprConstant.cpp:9523
@@ +9522,3 @@
+  if (!getType()->isPointerType() &&
+      !getType()->canDecayToPointerType())
+    return false;
----------------
rsmith wrote:
> I don't see where you handle types that decay to pointers (in particular, for the case where the expression has array type).
Do you have recommendations for how to test that? Code like


```
int fn(void *__attribute__((pass_object_size(0))));
int a[5];
int r = fn(a);
```

Generates an implicit Array->Pointer decay on `a` in `fn(a)`, which is handled fine; so, AFAICT, the support for being handed Exprs decayable types is unnecessary. I'll just remove it.

================
Comment at: lib/AST/ItaniumMangle.cpp:501
@@ +500,3 @@
+  // Current mangling scheme:
+  // <pass-object-size-spec-list> ::= _PS <pass-object-size-specs> _PE
+  // <pass-object-size-specs>     ::= <pass-object-size-spec>
----------------
rsmith wrote:
> Use `U16pass_object_size` instead, per http://mentorembedded.github.io/cxx-abi/abi.html#mangling-type
> 
> I would suggest mangling this as if it were a qualifier on the type of the parameter rather than a modifier on the function type itself.
WFM; changed to `U17pass_object_size${Type}`.

================
Comment at: lib/AST/Type.cpp:2813-2817
@@ -2805,7 +2812,7 @@
   //      bool type*
   // This is followed by an optional "consumed argument" section of the
   // same length as the first type sequence:
   //      bool*
   // Finally, we have the ext info and trailing return type flag:
   //      int bool
   // 
----------------
rsmith wrote:
> Update this comment.
No longer necessary now that we're leaving FPTs alone.

================
Comment at: lib/AST/Type.cpp:2848-2858
@@ -2840,8 +2847,13 @@
   }
   if (epi.ConsumedParameters) {
     for (unsigned i = 0; i != NumParams; ++i)
       ID.AddBoolean(epi.ConsumedParameters[i]);
   }
   epi.ExtInfo.Profile(ID);
   ID.AddBoolean(epi.HasTrailingReturn);
+
+  if (epi.PassObjectSizeParams) {
+    for (unsigned I = 0; I != NumParams; ++I)
+      ID.AddBoolean(epi.PassObjectSizeParams[I]);
+  }
 }
----------------
rsmith wrote:
> This doesn't quite work: a function type with consumed parameters and a function type with pass_object_size parameters could profile the same.
Oooh, nice catch. Thankfully, this change isn't necessary now :)

================
Comment at: lib/Sema/SemaDeclAttr.cpp:820
@@ +819,3 @@
+  Expr *E = Attr.getArgAsExpr(0);
+  IntegerLiteral *Int = dyn_cast<IntegerLiteral>(E->IgnoreParenCasts());
+  if (Int == nullptr) {
----------------
aaron.ballman wrote:
> We don't usually ignore parens and casts for attributes like this for other attributes. Is that important for you uses for some reason?
Not important -- it's gone now. :)

================
Comment at: lib/Sema/SemaDeclAttr.cpp:828
@@ +827,3 @@
+  auto APType = Int->getValue();
+  if (APType.ugt(3) || APType.slt(0)) {
+    S.Diag(E->getLocStart(), diag::err_attribute_argument_out_of_bounds)
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > The second check here is redundant if `APType` is more than 2 bits wide (and looks wrong when `Int` is of type `bool`).
> > I'm wondering why the magic value 3 at all? It seems like this (and the above code) should be replaced by a call to checkFunctionOrMethodParameterIndex().
> There should be comments explaining the magic number 3.
> I'm wondering why the magic value 3 at all? It seems like this (and the above code) should be replaced by a call to checkFunctionOrMethodParameterIndex().

Marking this as done because checkFunctionOrMethodParameterIndex doesn't seem to accomplish our goal.

> There should be comments explaining the magic number 3.

The docs for `pass_object_size(N)` state that `N` is passed as the second parameter to __builtin_object_size, which means that `N` must be in the range [0, 3]. I'll add a comment, but given your other comment, it looks like there may have been a miscommunication about the purpose of pass_object_size's argument. :)

================
Comment at: lib/Sema/SemaDeclAttr.cpp:829
@@ +828,3 @@
+  if (APType.ugt(3) || APType.slt(0)) {
+    S.Diag(E->getLocStart(), diag::err_attribute_argument_out_of_bounds)
+      << Attr.getName() << 1 << E->getSourceRange();
----------------
aaron.ballman wrote:
> That isn't the correct diagnostic to emit. That one is used when a function attribute that refers to a parameter (like format_arg) uses an index that does not refer to a parameter. For this one, I would probably modify err_attribute_argument_outof_range to not be specific to 'init_priority'.
Ahh, that makes sense. Thanks :)


http://reviews.llvm.org/D13263





More information about the cfe-commits mailing list