patch: new attribute enable_if

[Nick Lewycky]

Updated patch attached. Almost all review comments implemented!

Richard Smith wrote:
> Hi Nick,
>
> This looks really promising. Do you yet know whether this is enough to
> implement *all* the functionality of -DFORTIFY_SOURCE?

I asked Geremy to try implementing it with the patch I posted, and he 
says he has -DFORTIFY_SOURCE=1 done, and forsees no problems with 
-DFORTIFY_SOURCE=2.

> 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.

Great point. I've started on this, but I'm not really sure yet what the 
semantics ought to be ...

> 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?

On the one hand, I will not support anything that actually changes state 
inside the expression. On the other hand, imagine trying to express 
strlen(foo) inside an enable_if expression in C mode where you can't 
write constexpr functions, loops, etc. (No, statement expressions are 
not allowed inside the enable_if attribute, I tried that.)

I've asked Geremy to let me know what he's actually using in his 
expressions. If he isn't using anything that isn't a language-defined 
constant expression then that's the obvious way to go.

> --- 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?

Done.

> --- 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.

All true. And it does match g++.

I am extremely nervous about changing __builtin_constant_p's behaviour, 
even if the result does more closely match gcc in this particular instance.

My ideal would be to have a way to get builtin_constant_p continue to 
have access to the function parameters as I need for enable_if 
evaluation with no other visible changes. It looks like doing that would 
require making a copy of the EvalInfo and that would break all the 
CallStackFrames which hold it by reference, so I'd need to copy them 
too, yuck.

What do you think I should do here? Try to land this one bit on its own? 
Try to make a deep copy of the EvalInfo state? Something more clever?

> You'll also need to turn off the ability to mutate local state during
> the evaluation of the subexpression in C++1y mode.

Can I punt this to a future patch? It might turn out to be a small 
number of lines of code, but I'm worried about the cognitive load of 
adding yet another "thread foo through ExprConstant" getting into this 
patch.

> --- 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/.

Done.

> Can we avoid redoing the enable_if check in SemaChecking if we've
> already been through overload resolution (and done the checking there)?

Done!

> --- 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.)

Whoops! I had that right previously, then broke it during self-review.

> +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.

Done.

> --- 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.

Done. We now have Parser::IsAttributeSupportedByGCC.

> [...]
> +// 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).

Thanks for the pointer, I've implemented this.

I'm okay with expressions that evaluate to true or to false. It could be 
reasonable if it's expanded from a preprocessor macro or somesuch.

It'd be nice to have warnings for "{none|two or more} of the overloads 
are enabled" but I think because of the way we parse we'd get the errors 
at the call-sites before we could detect this.

Nick
-------------- next part --------------
A non-text attachment was scrubbed...
Name: enable-if-3.patch
Type: text/x-diff
Size: 41802 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131002/b1f13eb5/attachment.patch>