<div dir="ltr"><div>Hi Nick,</div><div><br></div><div>This looks really promising. Do you yet know whether this is enough to implement *all* the functionality of -DFORTIFY_SOURCE?</div><div><br></div><div>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.</div>
<div><br></div><div>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?</div>
<div><br></div><div><br></div><div>--- include/clang/Sema/TemplateDeduction.h<span class="" style="white-space:pre"> </span>(revision 191171)</div><div>+++ include/clang/Sema/TemplateDeduction.h<span class="" style="white-space:pre"> </span>(working copy)</div>
<div>@@ -208,6 +209,10 @@<br></div><div> /// if any.</div><div> Expr *getExpr();</div><div> </div><div>+ /// When FailureKind is ovl_fail_enable_if, the attribute which caused</div><div>+ /// this function to not be viable.</div>
<div>+ EnableIfAttr *EnableIfAttribute;</div><div><br></div><div>Can you reuse the existing Data pointer for this, rather than growing DeductionFailureInfo?</div><div><br></div><div><div>--- lib/AST/ExprConstant.cpp<span class="" style="white-space:pre"> </span>(revision 191171)</div>
<div>+++ lib/AST/ExprConstant.cpp<span class="" style="white-space:pre"> </span>(working copy)</div></div><div><div>@@ -5803,26 +5810,35 @@</div><div> //</div><div> // Otherwise, it returns 0.</div><div> //</div><div>
+ // In the former case, when building with -O, GCC will sometimes return 1 if</div><div>+ // a constant numeric value is used as an argument to an inline function, and</div><div>+ // the corresponding parameter is passed to __builtin_constant_p. In the</div>
<div>+ // latter case, it never will. We pretend -O is always specified when checking</div><div>+ // constexpr function parameters.</div><div>+ //</div><div> // FIXME: GCC also intends to return 1 for literals of aggregate types, but</div>
<div> // its support for this does not currently work.</div><div> if (ArgType->isIntegralOrEnumerationType()) {</div><div>- Expr::EvalResult Result;</div><div>- if (!Arg->EvaluateAsRValue(Result, Ctx) || Result.HasSideEffects)</div>
<div>+ llvm::SmallVector<PartialDiagnosticAt, 8> Diag;</div><div>+ SpeculativeEvaluationRAII Speculate(Info, &Diag);</div><div>+</div><div>+ Info.EvalStatus.HasSideEffects = false;</div><div>+ APValue Result;</div>
<div>+ if (!EvaluateAsRValue(Info, Arg, Result) || Info.EvalStatus.HasSideEffects)</div><div> return false;</div><div><br></div><div><br></div><div>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:</div>
<div><br></div><div> constexpr bool f(int n) { return __builtin_constant_p(n); }</div><div> constexpr bool a = f(0);</div><div> bool b = f(0);</div><div> extern int n;</div><div> bool c = f(n);</div><div><br></div><div>
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.</div>
<div><br></div><div>You'll also need to turn off the ability to mutate local state during the evaluation of the subexpression in C++1y mode.</div><div><br></div><div><br></div><div><div>--- lib/Parse/ParseDecl.cpp<span class="" style="white-space:pre"> </span>(revision 191171)</div>
<div>+++ lib/Parse/ParseDecl.cpp<span class="" style="white-space:pre"> </span>(working copy)</div></div><div><div>@@ -226,6 +228,21 @@</div><div> ParseTypeTagForDatatypeAttribute(*AttrName, AttrNameLoc, Attrs, EndLoc);</div>
<div> return;</div><div> }</div><div>+ // These may refer to the function arguments, but need to be parsed early to</div><div>+ // participate in determining whether it's a redeclaration.</div><div>+ llvm::OwningPtr<ParseScope> PrototypeScope;</div>
<div>+ if (AttrName->isStr("enable_if") && D && D->isFunctionDeclarator()) {</div><div>+ DeclaratorChunk::FunctionTypeInfo FTI = D->getFunctionTypeInfo();</div><div>+ PrototypeScope.reset(new ParseScope(this, Scope::FunctionPrototypeScope |</div>
<div>+ Scope::FunctionDeclarationScope |</div><div>+ Scope::DeclScope));</div><div>+ for (unsigned i = 0; i != FTI.NumArgs; ++i) {</div><div>
+ ParmVarDecl *Param = cast<ParmVarDecl>(FTI.ArgInfo[i].Param);</div><div>+ getCurScope()->AddDecl(Param);</div><div>+ if (Param->getDeclName())</div><div>+ Actions.IdResolver.AddDecl(Param);</div>
<div><br></div></div></div><div>Filling in the scope should be done in Sema/, not in Parse/.</div><div><br></div><div>Can we avoid redoing the enable_if check in SemaChecking if we've already been through overload resolution (and done the checking there)?</div>
<div><br></div><div><div>--- lib/Sema/SemaOverload.cpp<span class="" style="white-space:pre"> </span>(revision 191171)</div><div>+++ lib/Sema/SemaOverload.cpp<span class="" style="white-space:pre"> </span>(working copy)</div>
</div><div><div>@@ -1079,6 +1079,21 @@</div><div> return true;</div><div> }</div><div> </div><div>+ // enable_if attributes are an order-sensitive part of the signature.</div><div>+ for (specific_attr_iterator<EnableIfAttr></div>
<div>+ NewI = New->specific_attr_begin<EnableIfAttr>(),</div><div>+ NewE = New->specific_attr_end<EnableIfAttr>(),</div><div>+ OldI = Old->specific_attr_begin<EnableIfAttr>(),</div>
<div>+ OldE = Old->specific_attr_end<EnableIfAttr>();</div><div>+ NewI != NewE && OldI != OldE; ++NewI, ++OldI) {</div><div>+ if (NewI == NewE || OldI == OldE)</div><div>+ return true;</div>
<div>+ llvm::FoldingSetNodeID NewID, OldID;</div><div>+ NewI->getCond()->Profile(NewID, Context, true);</div><div>+ OldI->getCond()->Profile(OldID, Context, true);</div><div>+ return !(NewID == OldID);</div>
</div><div><br></div><div>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.)<br></div><div><br></div><div><br></div><div>
<div>+EnableIfAttr *Sema::CheckEnableIf(FunctionDecl *Function, ArrayRef<Expr *> Args,</div><div>+ bool MissingImplicitThis) {</div><div>+ </div><div>
+ for (specific_attr_iterator<EnableIfAttr></div><div>+ I = Function->specific_attr_begin<EnableIfAttr>(),</div><div>+ E = Function->specific_attr_end<EnableIfAttr>(); I != E; ++I) {</div>
<div>+ APValue Result;</div><div>+</div><div>+ SFINAETrap Trap(*this);</div><div>+</div><div>+ // Convert the arguments.</div><div>+ SmallVector<Expr *, 16> Params;</div></div><div><br></div><div>ConvertedArgs maybe? These are not parameters.</div>
<div><br></div><div><div>--- test/Sema/enable_if.c<span class="" style="white-space:pre"> </span>(revision 0)</div><div>+++ test/Sema/enable_if.c<span class="" style="white-space:pre"> </span>(working copy)</div></div><div>
<div>@@ -0,0 +1,93 @@</div><div>+// RUN: %clang_cc1 %s -verify -Wno-gcc-compat</div></div><div><br></div><div><br></div><div>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.</div>
<div><br></div><div><br></div><div>[...]</div><div><div>+// This f() is never chosen because the constant evaluator fails on non-const global, but there's no error for that.</div><div>+// FIXME: we should issue an error at the point of the enable_if declaration</div>
<div>+int global;</div><div>+void f(int n) __attribute__((enable_if(global == 0, "chosen when 'global' is zero"))); // expected-note{{candidate ignored: chosen when 'global' is zero}}</div></div>
<div><br></div><div><br></div><div>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).</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, Sep 22, 2013 at 2:57 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nicholas@mxc.ca" target="_blank">nicholas@mxc.ca</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">Aaron Ballman wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Most of my comments are related to the attribute itself. I've got to<br>
run, but can take another look tomorrow as well.<br>
</blockquote>
<br></div>
Thanks! Unless I replied to it here, I've done what you suggested. Updated patch attached!<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+def EnableIf : InheritableAttr {<br>
+ let Spellings = [GNU<"enable_if">];<br>
</blockquote>
<br>
Is this really a GNU attribute? If not, perhaps since this is related<br>
to overloading, this could have a C++11 style clang:: attribute.<br>
(Reading further, I see tests for C as well, so perhaps not.)<br>
</blockquote>
<br></div>
As much as __attribute__((overloadable)) is. It's intended to be used with the GNU syntax for attributes, but isn't implemented by GCC.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ if (FunctionDecl *FD = dyn_cast_or_null<FunctionDecl><u></u>(FDecl)) {<br>
+ if (FD->hasAttr<EnableIfAttr>()) {<br>
+ ArrayRef<Expr *> NonConstArgs =<br>
+ llvm::makeArrayRef<Expr*>(<u></u>const_cast<Expr**>(Args.data()<u></u>), Args.size());<br>
</blockquote>
<br>
This const_cast makes me a bit nervous; is there a way to avoid it?<br>
</blockquote>
<br></div>
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.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
Index: lib/Sema/SemaDeclAttr.cpp<br>
==============================<u></u>==============================<u></u>=======<br>
--- lib/Sema/SemaDeclAttr.cpp (revision 191171)<br>
+++ lib/Sema/SemaDeclAttr.cpp (working copy)<br>
@@ -982,6 +982,33 @@<br>
Attr.<u></u>getAttributeSpellingListIndex(<u></u>)));<br>
}<br>
<br>
+static void handleEnableIfAttr(Sema&S, Decl *D, const AttributeList&Attr) {<br></div>
+ if (!isa<FunctionDecl>(D)&& !isa<FunctionTemplateDecl>(D)) {<div class="im"><br>
+ S.Diag(Attr.getLoc(), diag::err_attribute_enable_if_<u></u>not_function);<br>
+ return;<br>
+ }<br>
</div></blockquote><div class="im">
<br>
Does this apply to function-like things as well (such as function<br>
pointers, etc)?<br>
</div></blockquote>
<br>
No.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ if (!checkAttributeNumArgs(S, Attr, 2))<br>
+ return;<br>
+<br>
+ Expr *Cond = Attr.getArgAsExpr(0);<br>
+ Expr *Message = Attr.getArgAsExpr(1);<br>
</blockquote>
<br>
Attributes can take unresolved identifiers as well as expressions (for<br>
the first argument position), so you should be prepared to handle a<br>
case like that.<br>
</blockquote>
<br></div>
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?<div class="im">
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Missing test cases for everything in SemaDeclAttr.cpp. Be sure to hit<br>
edge cases as well (such as unresolved identifier as the expression).<br>
</blockquote>
<br></div>
Thanks! I added a bunch, let me know if there's anything I missed.<span class="HOEnZb"><font color="#888888"><br>
<br>
Nick<br>
</font></span><br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div>