This is an archive of the discontinued LLVM Phabricator instance.

PR45879: Keep evaluated expression in LValue object
AbandonedPublic

Authored by sepavloff on May 31 2021, 4:09 AM.

Details

Summary

Class LValue keeps result of lvalue evaluation. In some analyses it is
also necessary to have access to the original expression. This change
adds new member to LValue to keep the expression and initializes this
member when an lvalue is evaluated using LValueExprEvaluator.

With this change LHS expression becomes available in
HandleUnionActiveMemberChange and it becomes possible to fix PR45879.

Diff Detail

Event Timeline

sepavloff requested review of this revision.May 31 2021, 4:09 AM
sepavloff created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2021, 4:09 AM
tambre added a subscriber: tambre.Jul 5 2021, 11:54 PM

Rebased patch

Gentle ping. I can't review this, but if someone can, it would be awesome. https://bugs.llvm.org/show_bug.cgi?id=45879 is breaking some libc++ tests.

aaron.ballman added a subscriber: aaron.ballman.

Trying to help get this review going again as it impacts libc++.

clang/lib/AST/ExprConstant.cpp
1596–1597

Should we be asserting that LExpr is null?

8079

It's not super clear to me when consumers should call Evaluate() instead of calling Visit(). Some comments on the function may help make it more clear.

8080

Should we be asserting that Result.LExpr is not already set?

8617

Is there a reason we're not setting Result.LExpr here?

This breaks the following code:

struct sub
{
	char data;
};

struct main
{
	constexpr main()
	{
		member = {};
	}

	sub member;
};

constexpr main a{};

With:

fmt.cpp:16:16: error: constexpr variable 'a' must be initialized by a constant expression
constexpr main a{};
               ^~~
1 error generated.

Clang trunk and GCC (Debian 11.2.0-10) handle it fine.
Noticed in libfmt.

sepavloff updated this revision to Diff 384386.Nov 3 2021, 3:37 AM

Updated patch

Thank you for feedback!

This breaks the following code:

struct sub
{
	char data;
};

struct main
{
	constexpr main()
	{
		member = {};
	}

	sub member;
};

constexpr main a{};

With:

fmt.cpp:16:16: error: constexpr variable 'a' must be initialized by a constant expression
constexpr main a{};
               ^~~
1 error generated.

Clang trunk and GCC (Debian 11.2.0-10) handle it fine.
Noticed in libfmt.

Strange, I see that it cannot be compiled neither by gcc nor by clang: https://godbolt.org/z/1dY9Gs6zM. Do I miss something?

clang/lib/AST/ExprConstant.cpp
1596–1597

LExpr is initialized only once, when LValue starts evaluation. Later on the evaluation can proceed to subexpressions but the upper expression is sufficient to detect active union member change.

8079

Visit methods are now made non-public, hopefully it can make usage more clear.

8080

Result.LExpr is updated while evaluator descend the evaluated tree, so it is normal to see it set.

8617

Actually This method was used in the initial implementation, now it can be removed.

tambre added a comment.Nov 3 2021, 4:21 AM

Strange, I see that it cannot be compiled neither by gcc nor by clang: https://godbolt.org/z/1dY9Gs6zM. Do I miss something?

Sorry, should've been more specific. Try in C++20 mode: https://godbolt.org/z/4v8b3nsET
I think the difference might be due to P1331R2, but I'm not sure.

sepavloff updated this revision to Diff 386823.Nov 12 2021, 5:33 AM

Update the patch

  • Use more careful check for LHS expression,
  • Make a bit more precise LHS expression tracking,
  • Add comments,
  • Add the test from discussion.

Strange, I see that it cannot be compiled neither by gcc nor by clang: https://godbolt.org/z/1dY9Gs6zM. Do I miss something?

Sorry, should've been more specific. Try in C++20 mode: https://godbolt.org/z/4v8b3nsET
I think the difference might be due to P1331R2, but I'm not sure.

Thank you for this helpful test. I used a bit more accurate use of LHS expression and the issue seems resolved.

The new variant uses slightly more careful LHS expression tracking. Previously the expression was assigned only once. Now if the expression for LValue can be determined, it is updated. Only if the LValue starts to track non-expression entities, LHS expression is frozen to the last seen value. It does not affect the evaluation of active union member though.

P1330R0 (https://wg21.link/P1330R0) allowed changing the active member of a union in constexpr expressions. To implement it the Constant Evaluator starting from
https://github.com/llvm/llvm-project/commit/31c69a3d6363463c08b86914c0c8cfc5c929c37e checks if an assignment changes active member of a union. This is why a change proposed for union only affects compilation of code that does not contain unions at all. The issue is in the assignment treatment.

The check is made by the function HandleUnionActiveMemberChange, which is called from VisitBinAssign and HandleFunctionCall, they handle builtin and trivial assignment operators respectively. The check is made according to the rule specified in the standard (http://eel.is/c++draft/class.union#general-6), which is based on syntactic form. Constant evaluator on the other hand uses objects of class APValue and related LValue to make the evaluations. In some cases it is not possible to get Expr used for the evaluation.

Consider slightly modified test case provided in the bug report:

struct Base { int m; };
struct Derived : Base {};
 
constexpr int func_01(int x) {
  Base v1{1};
  Derived v2{x};
  v1 = v2;
  return v1.m;
}
 
constexpr int k = func_01(0);

When Constant Evaluator evaluates the expression v1 = v2, it eventually calls LValueExprEvaluator::handleCallExpr as this expression is a call of assignment operator. This is a method call, so the function evaluates the object reference, an LValue, which represents v1 as ValueDecl. Then HandleFunctionCall is called, provided with the LValue for v1 and an expression for v2. This function calls HandleUnionActiveMemberChange, which checks if the assignment changes an union active member.

HandleUnionActiveMemberChange makes the check according to the standard, so it needs v1 in the form of expression. However, v1 is represented by LValue in HandleFunctionCall, which contains ValueDecl and the original expression is unavailable. It makes the check impossible.

To make the check HandleUnionActiveMemberChange must be provided by the expression representing v1. A simple solution proposed here is to keep the original expression in LValue. It is set when LValue is evaluated when by LValueExprEvaluator in its method Evaluate. If the evaluation produces LValue based on something other than an expression, the reference to the original method remains in that LValue and may be used when the original expression is required.

The obtained solution is low-invasive. Actually only the changes in LValueExprEvaluator::Evaluate and Lvalue::set are essential for maintaining expression reference in LValue, and the change in HandleUnionActiveMemberChange is the only usage of it.

Hello, I know I'm not helping move this patch along because I'm not the right person to review this. However, I'd like to kindly reiterate that fixing this would be helpful for libc++. I just had to re-XFAIL one of our tuple tests because of this :-)

rsmith added inline comments.Feb 2 2022, 4:23 PM
clang/lib/AST/ExprConstant.cpp
1549

I don't think this is an appropriate approach. A source expression is fundamentally not part of an lvalue, and so we shouldn't be tracking one. I think we can fix this bug by handling the active union member change for a class assignment at a higher level, working on a patch...

sepavloff abandoned this revision.Feb 3 2022, 8:50 AM

@rsmith, thank you!

@rsmith Thanks a bunch!

And thanks @sepavloff for trying out this approach too.