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