[PATCH] D13263: Addition of __attribute__((pass_object_size)) to Clang
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 5 18:31:32 PST 2015
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
More information about the cfe-commits
mailing list