[PATCH] Implements DR 712 (odr-use, potential results of an expression)
Faisal Vali
faisalv at gmail.com
Sun Jun 2 12:36:50 PDT 2013
On Wed, May 29, 2013 at 5:01 PM, Richard Smith <richard at metafoo.co.uk>wrote:
> On Wed, May 29, 2013 at 11:19 AM, Faisal Vali <faisalv at gmail.com> wrote:
>
I assume you didn't mean to include the changes to ParseAST.cpp?
>
>
No I did not. Sorry - I'll try and be better about that - but it might
happen again ;)
Index: include/clang/Sema/Sema.h
> ===================================================================
> --- include/clang/Sema/Sema.h (revision 182855)
> +++ include/clang/Sema/Sema.h (working copy)
> @@ -3016,6 +3016,9 @@
> void MarkMemberReferenced(MemberExpr *E);
>
> void UpdateMarkingForLValueToRValue(Expr *E);
> + // Assess the expression for any references to a variable
> + // that do not entail an odr-use of that variable.
> + void UpdateNonODRUsedVariableReferences(Expr *E);
>
> Isn't this function the same thing as UpdateMarkingForLValueToRValue?
> Maybe merge them into a single function? (FWIW, I prefer the old name.)
>
> Done.
>
> @@ -11508,13 +11508,23 @@
> // Note that we use the C++11 definition everywhere because nothing in
> // C++03 depends on whether we get the C++03 version correct. The second
> // part does not apply to references, since they are not objects.
> + // Per DR: 712 constant expressions in discarded value expressions
> + // are not odr-used either.
>
> The mention of discarded value expressions seems out of place here.
>
> + //
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3383.html#712
>
> We usually don't include such links in comments.
>
> + //
> const VarDecl *DefVD;
> - if (E && !isa<ParmVarDecl>(Var) &&
> + if (E && SemaRef.getLangOpts().CPlusPlus11 &&
> + E->isCXX11ConstantExpr(SemaRef.Context)) {
> + if (!Var->getType()->isReferenceType())
> + SemaRef.MaybeODRUseExprs.insert(E);
>
> Hah, this is a literal interpretation of the standard text, but isn't what
> was intended. It allows DeclRefExprs referring to globals, for instance.
> What was *intended* was to look for objects on which the lvalue-to-rvalue
> conversion could be applied in a constant expression, that is, what the
> existing code does:
>
>
+ }
> + else if (E && !isa<ParmVarDecl>(Var) &&
> Var->isUsableInConstantExpressions(SemaRef.Context) &&
> Var->getAnyInitializer(DefVD) && DefVD->checkInitIsICE()) {
>
> Done. The comment might need some additional work though.
>
> Index: lib/Sema/SemaExprCXX.cpp
> ===================================================================
> --- lib/Sema/SemaExprCXX.cpp (revision 182855)
> +++ lib/Sema/SemaExprCXX.cpp (working copy)
> @@ -5518,6 +5518,78 @@
> return false;
> }
>
> +namespace {
> +
> + struct ConstantExpressionODRCleaner : RecursiveASTVisitor<
> + ConstantExpressionODRCleaner> {
> +
>
> We normally don't include blank lines in these cases. Also, maybe split
> this line after the : instead of after the < ?
>
> Done.
> A RecursiveASTVisitor seems overkill here, since you're neither using the
> recursive walk, nor dispatch on anything other than Stmts. Maybe this
> should be a ConstStmtVisitor instead?
>
> Done.
> Maybe name this something like PotentialResultsSetFinder, since that's
> closer to what it does.
>
> Done.
>
> + llvm::SmallPtrSet<Expr*, 2>& MaybeODRUseExprs;
>
> " &", not "& ", please. I have other formatting-related comments on this
> patch, but clang-format can tell you what they are. :-)
>
>
Once I obtain your feedback on this revision - and prior to my next
revision, i'll try and figure out how to use clang-format - thanks!
>
> + typedef RecursiveASTVisitor<ConstantExpressionODRCleaner> inherited;
>
> Should be "Inherited", bu just remove this since you're not actually using
> it.
>
>
> + // C++14 CD, DR 712: 3.2 para 2
>
> Please use "C++1y" here so that we can more easily find it with a grep
> later.
>
> Done.
>
> + // normally, we don't need to do any additional conversions to handle
> it,
> + // but if it is a volatile lvalue with a special form, we perform an
> + // lvalue-to-rvalue conversion. Additionally, if it was a c++11
> constant
> + // expression.
>
> The added comment here doesn't look right; I think you should say
> something like "Even if we do not perform an lvalue-to-rvalue conversion,
> we pretend that one was performed when checking for odr-uses of variables."
>
>
> Done.
> Index: lib/CodeGen/CGExpr.cpp
> ===================================================================
>
> You have CodeGen changes but no tests for them, please add some, covering
> all the ways in which a variable can be referenced without being odr-used.
>
>
Still no codegen tests on this revision - will include them with my next
revision once i get your technical feedback on this one (and figure out the
LLVM IR i need to
ramp up on, so that I can start writing codegen tests for this). Any
preliminary guidance will be appreciated.
>
> --- lib/CodeGen/CGExpr.cpp (revision 182855)
> +++ lib/CodeGen/CGExpr.cpp (working copy)
> @@ -1800,6 +1800,13 @@
> return CGF.EmitLValueForField(LV, FD);
> }
>
> +
> +inline bool isLambdaCallOperator(const Decl *D) {
>
> This seems generally useful; perhaps it should be a member of
> CXXMethodDecl? If not, s/inline/static/, please.
>
> This still needs to be done.
>
> + if (!ND->isUsed(false) || (E->refersToEnclosingLocal() &&
> + isLambdaCallOperator(CurCodeDecl))) {
> + if (const VarDecl *VD = dyn_cast<VarDecl>(ND)) {
>
> Please flip these two 'if's around, and merge this into the preceding code
> (the end result should just be removing the isReferenceType() check from
> the preceding code block and adding the lambda-enclosing-local check).
>
>
> Done.
> @@ -2476,7 +2497,23 @@
>
> LValue CodeGenFunction::EmitMemberExpr(const MemberExpr *E) {
> Expr *BaseExpr = E->getBase();
> + ASTContext &Context = CGM.getContext();
> + APValue ConstExprResult;
>
> + // Emit a field member access that evaluates to a constexpr s.x
> + if (Context.getLangOpts().CPlusPlus11 &&
> E->isCXX11ConstantExpr(Context,
> + &ConstExprResult)) {
> + if (FieldDecl *FD = dyn_cast<FieldDecl>(E->getMemberDecl())) {
> + CharUnits Alignment = Context.getDeclAlign(FD);
> + llvm::Constant *Val =
> + CGM.EmitConstantValue(ConstExprResult, FD->getType(), this);
> + assert(Val && "failed to emit C++11 constant member expression");
> + // FIXME: Eventually we will want to emit vector element references.
> + QualType T = E->getType();
> + return MakeAddrLValue(Val, T, Alignment);
> + }
> + }
>
> I don't think this approach can work. Consider this:
>
> struct S { int x; };
> constexpr S a = { 1 };
> S b = { 2 };
> bool k;
> return (k ? a : b).x;
>
> This does not odr-use 'a', but it does odr-use 'b'. The MemberExpr is not
> a constant expression here. I think instead what you should do is to teach
> AggExprEmitter (and friends) to directly emit a constant value when they
> visit a DeclRefExpr which is not an odr-use
>
>
I think I fixed this - but I must admit I was unable to figure out how to
fix it through AggExprEmitter. My fix uses a visitor in
CodeGenFunction::EmitDeclRefExpr
to try and figure out whether the DeclRefExpr is in a conditional operator
context of an object expression of a member expression (which is pushed
onto a stack), and if
that is the case, it emits a pointer to the struct vs the constant version
of the struct. It seems to pass my tests - but like i said earlier, I have
not had the time yet
to write the appropriate LLVM-IR codegen tests. Please let me know about
the robustness of my fix - it does feel a little brittle to me. If you
stiill feel
AggExprEmitter is the way to go with this, I could certainly use a little
guidance as to how we enter into AggExprEmitter while emitting code for the
conditional expr.
>
> @@ -3239,6 +3276,23 @@
> LValue CodeGenFunction::
> EmitPointerToDataMemberBinaryExpr(const BinaryOperator *E) {
> llvm::Value *BaseV;
> +
> + Expr *ObjExpr = E->getLHS();
> + ASTContext &Context = CGM.getContext();
> + APValue ConstExprResult;
> + // Emit a field member access that evaluates to a constexpr s.*memfn
> + if (Context.getLangOpts().CPlusPlus11 &&
> E->isCXX11ConstantExpr(Context,
> + &ConstExprResult)) {
> + const MemberPointerType *MPT
> + = E->getRHS()->getType()->getAs<MemberPointerType>();
> + llvm::Constant *Val =
> + CGM.EmitConstantValue(ConstExprResult, MPT->getPointeeType(), this);
> + assert(Val && "failed to emit C++11 constant member expression");
> + // FIXME: Eventually we will want to emit vector element references.
> + QualType T = E->getType();
> + return MakeAddrLValue(Val, T);
> + }
>
> In this case, you should constant-evaluate the RHS but not the LHS. Also,
> use EvaluateAsRValue here; it doesn't matter whether we need to perform
> non-standard constant folding. And there's no need to check for C++11 mode.
>
>
> Done.
> Index: test/CXX/basic/basic.def.odr/p2-DR712-C++14-CD.cpp
> ===================================================================
> --- test/CXX/basic/basic.def.odr/p2-DR712-C++14-CD.cpp (revision 0)
> +++ test/CXX/basic/basic.def.odr/p2-DR712-C++14-CD.cpp (working copy)
>
> Maybe just call this .../p2-potential-results.cpp -- this isn't
> C++1y-specific. This file contains both UTF-8 characters in comments and
> \r\n line separators, please fix.
>
Done.
Await your feedback ...
Also *ping* the patch (
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130506/079656.html)
was LGTM'd by Doug, anyway we can get that one committed please - thank you!
Thanks again Richard!
>
>
>>
>>
>>
>>
>> On Sat, May 18, 2013 at 1:42 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>>
>>> On Sat, May 18, 2013 at 9:22 AM, Faisal Vali <faisalv at gmail.com> wrote:
>>>
>>>> Before I return to working on generic lambdas, I thought it might be
>>>> useful to nail down DR 712 which reworded the definition of odr-use in
>>>> terms of the potential results of an expression (in order to avoid
>>>> odr-using a variable that represents a constant expression whenever we can
>>>> get away with it). This is relevant to lambdas, because it affects which
>>>> variables get captured and which don't.
>>>>
>>>>
>>>> Let me review how I think clang currently handles this (pre-712
>>>> resolution - please correct me where I err). While parsing and
>>>> semanalyzing expressions, all variables that are usable in constant
>>>> expressions (and so might not need to be odr-used) are stored in
>>>> MaybeODRUseExprs (a set container) as DeclRefExprs (DRE). Then, if an
>>>> Lvalue-to-Rvalue conversion is performed on the DRE, it is removed from the
>>>> set above (In UpdateMarkingForLValueToRValue). Anything left in that set
>>>> is marked as odr-used by ActOnFinishFullExpr.
>>>>
>>>>
>>>> OK, with the above in mind, these are my initial thoughts on how to
>>>> implement it so that not only do we handle those constant expressions that
>>>> are lvalue-to-rvalue converted, but also those that are discarded, and also
>>>> handle the conditional expressions and member-access expressions.
>>>>
>>>> The relevant hooks/callbacks/functions within clang are:
>>>> - UpdateMarkingForLValueToRValue
>>>> - IgnoredValueConversions
>>>>
>>>> A function i intend to write is GetPotentialResultsOfExpression: when
>>>> passed an Expr* E, returns a vector of its potential results as
>>>> DeclRefExpr's (and MemberExpr's??); this function should follow from 3.2
>>>> para 2.
>>>>
>>>> Now in each hook, GetPotentialResults shall be called, and those
>>>> expressions shall be removed from the MaybeODRUseExpr.
>>>>
>>>
>>> This seems like the right approach, although you presumably don't
>>> actually need to build the vector and could just remove the odr-use marker
>>> directly.
>>>
>>>
>>>> What are your thoughts on the above preliminary strategy? I have this
>>>> sense that Member expressions are going to require some more work, but
>>>> we'll see...
>>>>
>>>> Also before I proceed any further, I was hoping to get some more
>>>> clarity on 3.2 para 2 and para 3 - please see my queries below:
>>>>
>>>>
>>>> Consider the following code:
>>>>
>>>>
>>>> struct S {
>>>> const int mi;
>>>> constexpr S(int i) : mi(i) { }
>>>> };
>>>>
>>>> int f(const int& );
>>>> void g(int);
>>>> int main() {
>>>> constexpr S r{5};
>>>> constexpr S s{r};
>>>> [](char c) {
>>>> c = s.mi; // #1 does this odr-use 's'
>>>>
>>>
>>> No. Here, e = 's.mi', ex = 's' (a member of the set of potential results
>>> of e), and e is subject to an lvalue-to-rvalue conversion.
>>>
>>>
>>>> g(s.mi); // #2 what about this?
>>>>
>>>
>>> Also no, because an lvalue-to-rvalue conversion is applied to s.mi.
>>>
>>>
>>>> s.mi; // #3 and this?
>>>>
>>>
>>> Also no, because this is a discarded-value expression.
>>>
>>>
>>>> c ? s.mi : r.mi; // #4 and this?
>>>>
>>>
>>> Also no.
>>>
>>>
>>>> g( c ? s.mi : r.mi ); // #5
>>>>
>>>
>>> Also no.
>>>
>>>
>>>> };
>>>> }
>>>>
>>>> My intial suspicion was that neither s nor r were odr-used in the
>>>> lambda (because the compiler can figure out the value of 'mi' at its point
>>>> of use, and use it or discard it) - but then based on Richard's analysis of
>>>> the example below where he states that s.operator int() is an odr-use of
>>>> 's', i am now unsure.
>>>>
>>>
>>> Note that member function calls aren't listed in the rules of 3.2/2. 's'
>>> is not in the set of potential results of 's.operator int()'.
>>>
>>>
>>>> Here is my attempt at trying to figure this out (e refers to
>>>> expression, ex as a potentially-evaluated expression, using the language of
>>>> 3.2 para 2 and 3)
>>>>
>>>> In #1,
>>>> the variable 's' appears as a potentially evaluated expression and
>>>> satisfies the requirements for appearing in a constant expression, and it
>>>> is an object, and it is an element of the set of potential results of an
>>>> expression 'e' (s.mi) and the lvalue-to-rvalue conversion is applied to
>>>> that expression so it is not odr-used? But what if 'e' is deemed to be 's'
>>>> in 's.mi' - no lvalue-to-rvalue conversion is applied so it should be
>>>> captured?
>>>>
>>>
>>> The rule says: "x [...] is odr-used unless [...] ex is is an element of
>>> the set of potential results of an expression e, where [...]"
>>>
>>> That is, if there *exists* such an 'e', then 'x' is not odr-used.
>>>
>>> Hope that helps!
>>>
>>>
>>>> OK, I'm going to give up on analyzing the rest, because I feel I'm
>>>> missing something and am unable to think about this clearly.
>>>>
>>>> Any help or guidance will be appreciated!
>>>>
>>>>
>>>>
>>>> On Wed, May 15, 2013 at 3:21 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>>>>
>>>>> On Tue, May 14, 2013 at 9:48 PM, Faisal Vali <faisalv at gmail.com>wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> While we're on the topic, can i ask you to clarify a few capture and
>>>>>> constexpr
>>>>>> questions below:
>>>>>>
>>>>>
>>>>> OK, the relevant context is [basic.def.odr]p2 and p3.
>>>>>
>>>>>
>>>>>> struct S {
>>>>>> constexpr S() {}
>>>>>> constexpr operator int() const { return 0; }
>>>>>> };
>>>>>> void fooS(S s) { }
>>>>>>
>>>>>> void fooInt(int i) { }
>>>>>>
>>>>>> void f() {
>>>>>> constexpr S s {};
>>>>>> auto L = [] (int x) {
>>>>>> (void) s; // 1
>>>>>>
>>>>>
>>>>> Here, the 's' expression is a discarded-value expression, and the 's'
>>>>> variable is in the set of potential results of the 's' expression, and 's'
>>>>> satisfies the constraints for appearing in a constant expression, so 's' is
>>>>> not odr-used, so is not captured. We should not require a capture-default
>>>>> here.
>>>>>
>>>>>
>>>>>> fooS(s); // 2
>>>>>>
>>>>>
>>>>> Here, 's' is implicitly passed to the implicit S::S(const S&)
>>>>> constructor. This is neither an lvalue-to-rvalue conversion nor a
>>>>> discarded-value expression, so 's' is odr-used and we have an error due to
>>>>> the missing capture-default.
>>>>>
>>>>>
>>>>>> fooInt(s); // 3
>>>>>>
>>>>>
>>>>> This is equivalent to 's.operator int()', which again odr-uses 's', so
>>>>> requires a capture-default.
>>>>>
>>>>>
>>>>>> };
>>>>>> }
>>>>>>
>>>>>> Should the above be ok in spite of L not having a default capture?
>>>>>> Currently clang errors on all of them individually - is that the
>>>>>> right behavior?
>>>>>>
>>>>>> Also, how would you want me to start submitting patches for commit -
>>>>>> do you want
>>>>>> me to break up the patch into smaller patches? - and if so, do you
>>>>>> have any thoughts
>>>>>> on how I might want to break up the functionality per patch?
>>>>>>
>>>>>
>>>>> Smaller patches are definitely better, if you can decompose the
>>>>> functionality into coherent smaller chunks. There are some hints on how to
>>>>> decompose the functionality here (but this division may or may not work for
>>>>> you):
>>>>>
>>>>>
>>>>> http://clang.llvm.org/docs/InternalsManual.html#how-to-add-an-expression-or-statement
>>>>>
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130602/b196ebc7/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DR712-post-rs-6-odr-use-works-for-member-expr.patch
Type: application/octet-stream
Size: 21440 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130602/b196ebc7/attachment.obj>
More information about the cfe-commits
mailing list