patch: new attribute enable_if
Sean Silva
silvas at purdue.edu
Wed Sep 25 08:17:23 PDT 2013
On Mon, Sep 23, 2013 at 10:59 PM, Richard Smith <richard at metafoo.co.uk>wrote:
> Hi Nick,
>
> This looks really promising. Do you yet know whether this is enough to
> implement *all* the functionality of -DFORTIFY_SOURCE?
>
> One big thing missing here is documentation. That should serve to both
> motivate the feature (and explain why it's sufficiently valuable for us to
> have and maintain this extension), and to describe how it's intended to be
> used, plus what we actually guarantee in terms of its semantics.
>
It may also be useful to run this past the GCC folks to get feedback. If
this extension is truly useful, I would expect that we would want GCC to
implement it as well, so it would be good to have them on board with the
idea.
-- Sean Silva
>
> I'm a bit concerned that the semantics of the attribute currently involve
> "try as hard as you can to fold this to a constant, and even ignore
> side-effects, but if constant-folding it fails, don't choose this overload"
> -- if we change how hard we're willing to try when constant-folding in the
> future, it might break code relying on this feature. How essential is it
> that we support conditions that have side-effects?
>
>
> --- include/clang/Sema/TemplateDeduction.h (revision 191171)
> +++ include/clang/Sema/TemplateDeduction.h (working copy)
> @@ -208,6 +209,10 @@
> /// if any.
> Expr *getExpr();
>
> + /// When FailureKind is ovl_fail_enable_if, the attribute which caused
> + /// this function to not be viable.
> + EnableIfAttr *EnableIfAttribute;
>
> Can you reuse the existing Data pointer for this, rather than growing
> DeductionFailureInfo?
>
> --- lib/AST/ExprConstant.cpp (revision 191171)
> +++ lib/AST/ExprConstant.cpp (working copy)
> @@ -5803,26 +5810,35 @@
> //
> // Otherwise, it returns 0.
> //
> + // In the former case, when building with -O, GCC will sometimes return
> 1 if
> + // a constant numeric value is used as an argument to an inline
> function, and
> + // the corresponding parameter is passed to __builtin_constant_p. In the
> + // latter case, it never will. We pretend -O is always specified when
> checking
> + // constexpr function parameters.
> + //
> // FIXME: GCC also intends to return 1 for literals of aggregate types,
> but
> // its support for this does not currently work.
> if (ArgType->isIntegralOrEnumerationType()) {
> - Expr::EvalResult Result;
> - if (!Arg->EvaluateAsRValue(Result, Ctx) || Result.HasSideEffects)
> + llvm::SmallVector<PartialDiagnosticAt, 8> Diag;
> + SpeculativeEvaluationRAII Speculate(Info, &Diag);
> +
> + Info.EvalStatus.HasSideEffects = false;
> + APValue Result;
> + if (!EvaluateAsRValue(Info, Arg, Result) ||
> Info.EvalStatus.HasSideEffects)
> return false;
>
>
> Reusing the current 'Info' here will mean that __builtin_constant_p's
> result will depend on the context as well as its operand. That may be what
> you intended here, but is a big enough change to be worth separately
> calling out (and separately committing). For instance:
>
> constexpr bool f(int n) { return __builtin_constant_p(n); }
> constexpr bool a = f(0);
> bool b = f(0);
> extern int n;
> bool c = f(n);
>
> I think, with your patch, we'll guarantee to initialize 'a' to true and
> 'c' to false, and 'b' may or may not be 'true'. Prior to your patch, all
> three will be false. IIRC, that basically matches g++'s behavior, but it'd
> be good to double-check.
>
> You'll also need to turn off the ability to mutate local state during the
> evaluation of the subexpression in C++1y mode.
>
>
> --- lib/Parse/ParseDecl.cpp (revision 191171)
> +++ lib/Parse/ParseDecl.cpp (working copy)
> @@ -226,6 +228,21 @@
> ParseTypeTagForDatatypeAttribute(*AttrName, AttrNameLoc, Attrs,
> EndLoc);
> return;
> }
> + // These may refer to the function arguments, but need to be parsed
> early to
> + // participate in determining whether it's a redeclaration.
> + llvm::OwningPtr<ParseScope> PrototypeScope;
> + if (AttrName->isStr("enable_if") && D && D->isFunctionDeclarator()) {
> + DeclaratorChunk::FunctionTypeInfo FTI = D->getFunctionTypeInfo();
> + PrototypeScope.reset(new ParseScope(this,
> Scope::FunctionPrototypeScope |
> + Scope::FunctionDeclarationScope |
> + Scope::DeclScope));
> + for (unsigned i = 0; i != FTI.NumArgs; ++i) {
> + ParmVarDecl *Param = cast<ParmVarDecl>(FTI.ArgInfo[i].Param);
> + getCurScope()->AddDecl(Param);
> + if (Param->getDeclName())
> + Actions.IdResolver.AddDecl(Param);
>
> Filling in the scope should be done in Sema/, not in Parse/.
>
> Can we avoid redoing the enable_if check in SemaChecking if we've already
> been through overload resolution (and done the checking there)?
>
> --- lib/Sema/SemaOverload.cpp (revision 191171)
> +++ lib/Sema/SemaOverload.cpp (working copy)
> @@ -1079,6 +1079,21 @@
> return true;
> }
>
> + // enable_if attributes are an order-sensitive part of the signature.
> + for (specific_attr_iterator<EnableIfAttr>
> + NewI = New->specific_attr_begin<EnableIfAttr>(),
> + NewE = New->specific_attr_end<EnableIfAttr>(),
> + OldI = Old->specific_attr_begin<EnableIfAttr>(),
> + OldE = Old->specific_attr_end<EnableIfAttr>();
> + NewI != NewE && OldI != OldE; ++NewI, ++OldI) {
> + if (NewI == NewE || OldI == OldE)
> + return true;
> + llvm::FoldingSetNodeID NewID, OldID;
> + NewI->getCond()->Profile(NewID, Context, true);
> + OldI->getCond()->Profile(OldID, Context, true);
> + return !(NewID == OldID);
>
> I don't think you meant to return here in the case where NewID == OldID.
> (You're only checking the first enable_if attribute on each function.)
>
>
> +EnableIfAttr *Sema::CheckEnableIf(FunctionDecl *Function, ArrayRef<Expr
> *> Args,
> + bool MissingImplicitThis) {
> +
> + for (specific_attr_iterator<EnableIfAttr>
> + I = Function->specific_attr_begin<EnableIfAttr>(),
> + E = Function->specific_attr_end<EnableIfAttr>(); I != E; ++I) {
> + APValue Result;
> +
> + SFINAETrap Trap(*this);
> +
> + // Convert the arguments.
> + SmallVector<Expr *, 16> Params;
>
> ConvertedArgs maybe? These are not parameters.
>
> --- test/Sema/enable_if.c (revision 0)
> +++ test/Sema/enable_if.c (working copy)
> @@ -0,0 +1,93 @@
> +// RUN: %clang_cc1 %s -verify -Wno-gcc-compat
>
>
> Presumably this warning flag is for the 'attributes on function
> definitions' warning? Maybe we should only provide that for GCC-compatible
> attributes. We already suppress it for the thread-safety attributes, IIRC.
>
>
> [...]
> +// This f() is never chosen because the constant evaluator fails on
> non-const global, but there's no error for that.
> +// FIXME: we should issue an error at the point of the enable_if
> declaration
> +int global;
> +void f(int n) __attribute__((enable_if(global == 0, "chosen when 'global'
> is zero"))); // expected-note{{candidate ignored: chosen when 'global' is
> zero}}
>
>
> You can use isPotentialConstantExpr for the interesting half of this
> (where the enable_if expression is never constant). I'm not sure what you
> want to do with the boring half (where the expression evaluates to 'false'
> -- or 'true' -- independently of the function's parameters' values or
> template arguments).
>
>
> On Sun, Sep 22, 2013 at 2:57 PM, Nick Lewycky <nicholas at mxc.ca> wrote:
>
>> Aaron Ballman wrote:
>>
>>> Most of my comments are related to the attribute itself. I've got to
>>> run, but can take another look tomorrow as well.
>>>
>>
>> Thanks! Unless I replied to it here, I've done what you suggested.
>> Updated patch attached!
>>
>>
>> +def EnableIf : InheritableAttr {
>>>> + let Spellings = [GNU<"enable_if">];
>>>>
>>>
>>> Is this really a GNU attribute? If not, perhaps since this is related
>>> to overloading, this could have a C++11 style clang:: attribute.
>>> (Reading further, I see tests for C as well, so perhaps not.)
>>>
>>
>> As much as __attribute__((overloadable)) is. It's intended to be used
>> with the GNU syntax for attributes, but isn't implemented by GCC.
>>
>>
>> + if (FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>**(FDecl)) {
>>>> + if (FD->hasAttr<EnableIfAttr>()) {
>>>> + ArrayRef<Expr *> NonConstArgs =
>>>> + llvm::makeArrayRef<Expr*>(**const_cast<Expr**>(Args.data()**),
>>>> Args.size());
>>>>
>>>
>>> This const_cast makes me a bit nervous; is there a way to avoid it?
>>>
>>
>> Not really, I can move it around a bit or I can remove consts all over
>> the place. I tried sinking it into the callee, but it so happens that
>> adding const requires just as nasty a statement.
>>
>> Index: lib/Sema/SemaDeclAttr.cpp
>>>> ==============================**==============================**=======
>>>> --- lib/Sema/SemaDeclAttr.cpp (revision 191171)
>>>> +++ lib/Sema/SemaDeclAttr.cpp (working copy)
>>>> @@ -982,6 +982,33 @@
>>>> Attr.**getAttributeSpellingListIndex(*
>>>> *)));
>>>> }
>>>>
>>>> +static void handleEnableIfAttr(Sema&S, Decl *D, const
>>>> AttributeList&Attr) {
>>>> + if (!isa<FunctionDecl>(D)&& !isa<FunctionTemplateDecl>(D)) {
>>>>
>>>> + S.Diag(Attr.getLoc(), diag::err_attribute_enable_if_**
>>>> not_function);
>>>> + return;
>>>> + }
>>>>
>>>
>>> Does this apply to function-like things as well (such as function
>>> pointers, etc)?
>>>
>>
>> No.
>>
>>
>> +
>>>> + if (!checkAttributeNumArgs(S, Attr, 2))
>>>> + return;
>>>> +
>>>> + Expr *Cond = Attr.getArgAsExpr(0);
>>>> + Expr *Message = Attr.getArgAsExpr(1);
>>>>
>>>
>>> Attributes can take unresolved identifiers as well as expressions (for
>>> the first argument position), so you should be prepared to handle a
>>> case like that.
>>>
>>
>> Sorry, are you saying that __attribute__((enable_if(id, ...))) could fail
>> to return an Expr? I tried that syntax and it works fine (and added a
>> test), but I'm wondering if there's another syntax you had in mind which I
>> haven't thought of?
>>
>>
>> Missing test cases for everything in SemaDeclAttr.cpp. Be sure to hit
>>> edge cases as well (such as unresolved identifier as the expression).
>>>
>>
>> Thanks! I added a bunch, let me know if there's anything I missed.
>>
>> Nick
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130925/fd85091f/attachment.html>
More information about the cfe-commits
mailing list