rsmith added a comment. As discussed off-line:
1. Can we avoid tracking the `pass_object_size` attributes in the canonical function type? As the only permitted use of a function with this attribute is as a direct callee in a function call, it seems like we'll always have the parameters to hand when we need to know how they affect the calling convention. 2. In order to pass non-frontend-constant object sizes (those computed in the middle-end by @llvm.objectsize) into `pass_object_size` parameters, we need to keep functions viable even if the object size is not known to be a constant. ================ 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; ---------------- ... 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. ================ 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 ---------------- Seems nicer to put this right after `HasAnyConsumedParams`. (I'm also surprised to find that we have a spare bit here...) ================ 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. ---------------- 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? ================ Comment at: include/clang/Basic/AttrDocs.td:269-270 @@ +268,4 @@ + let Content = [{ +.. Note:: The mangling of functions with parameters that are annotated with + ``pass_object_size`` is not yet standardized. + ---------------- Rather than "not yet standardized", perhaps "subject to change" is more useful for user-facing documentation. Might be worth noting that `asm("blah")` can be used to avoid ABI changes if we do change the mangling. ================ Comment at: include/clang/Basic/AttrDocs.td:351 @@ +350,3 @@ + +* It is an error to apply the ``pass_object_size`` attribute on parameters that + are not const pointers ---------------- This should only apply to the function definition; the top-level `const` has no effect on a non-defining declaration. ================ Comment at: include/clang/Basic/AttrDocs.td:352 @@ +351,3 @@ +* It is an error to apply the ``pass_object_size`` attribute on parameters that + are not const pointers + ---------------- Missing period. ================ 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)); + } + }]; +} ---------------- I would expect this to result in an error: neither A nor B is viable because their conditions are non-constant. ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5101 @@ -5089,1 +5100,3 @@ +def err_address_of_function_with_pass_object_size_params: Error< + "functions with pass_object_size params cannot have their address taken">; def err_parens_pointer_member_function : Error< ---------------- params -> parameters or better, something like: cannot take address of function %0 because parameter %1 uses pass_object_size attribute ================ Comment at: include/clang/Sema/TemplateDeduction.h:261 @@ -256,2 +260,3 @@ public: - TemplateSpecCandidateSet(SourceLocation Loc) : Loc(Loc) {} + TemplateSpecCandidateSet(SourceLocation Loc, bool ForTakingAddress=false) + : Loc(Loc), ForTakingAddress(ForTakingAddress) {} ---------------- Spaces around `=`. ================ Comment at: lib/AST/ExprConstant.cpp:6390-6391 @@ +6389,4 @@ + + // These are here more for readability than out of laziness -- yes, this was + // ripped from IntExprEvaluator :) + auto Error = [&WasError](const Expr *E) { ---------------- Comment doesn't really add anything. ================ Comment at: lib/AST/ExprConstant.cpp:6398 @@ +6397,3 @@ + + auto Success = [&WasError, &Size](uint64_t S, const Expr *E) { + Size = S; ---------------- You explicitly capture `WasError` but don't use it. Just use `[&]`? ================ 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); ---------------- Where is the handling for `Type == 3` now? ================ Comment at: lib/AST/ExprConstant.cpp:9523 @@ +9522,3 @@ + if (!getType()->isPointerType() && + !getType()->canDecayToPointerType()) + return false; ---------------- I don't see where you handle types that decay to pointers (in particular, for the case where the expression has array type). ================ 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> ---------------- 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. ================ Comment at: lib/AST/MicrosoftMangle.cpp:1731-1760 @@ -1729,1 +1730,32 @@ +void MicrosoftCXXNameMangler::manglePassObjectSizeParams( + const FunctionDecl *D) { + // Current mangling scheme: + // <pass-object-size-spec-list> ::= _PS <pass-object-size-specs> _PE + // <pass-object-size-specs> ::= <pass-object-size-spec> + // | <pass-object-size-spec> & + // <pass-object-size-specs> + // <pass-object-size-spec> ::= <arg-no> T <type> + // + // <arg-no> is the 0-indexed argument number, <type> is the integer passed + // for type. If no pass_object_size attributes are present, this won't add + // anything. + + unsigned ArgNo = 0; + bool EmittedPS = false; + for (auto *Param : D->params()) { + if (auto *PS = Param->getAttr<PassObjectSizeAttr>()) { + if (!EmittedPS) { + Out << "_PS"; + EmittedPS = true; + } else + Out << '&'; + Out << ArgNo << 'T' << PS->getType(); + } + ++ArgNo; + } + + if (EmittedPS) + Out << "_PE"; +} + ---------------- @majnemer, what would be a good mangling scheme for this extension in the MS ABI? ================ 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 // ---------------- Update this comment. ================ 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]); + } } ---------------- This doesn't quite work: a function type with consumed parameters and a function type with pass_object_size parameters could profile the same. ================ Comment at: lib/CodeGen/CGCall.cpp:97-99 @@ +96,5 @@ +/// FPT has pass_object_size attrs, then we'll add parameters for those, too. +static void appendTypesToPrefix(const CodeGenTypes &CGT, + SmallVectorImpl<CanQualType> &prefix, + const CanQual<FunctionProtoType> &FPT) { + auto *PT = FPT.getTypePtr(); ---------------- I don't think the "prefix" nature is relevant to this function (that's an implementation detail of the callers). "appendParameterTypes" might be a better name. ================ 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) ---------------- The second check here is redundant if `APType` is more than 2 bits wide (and looks wrong when `Int` is of type `bool`). http://reviews.llvm.org/D13263 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits