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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
