Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang][ExprConst] Don't create useless temporary variable #67716

Closed
wants to merge 1 commit into from

Conversation

tbaederr
Copy link
Contributor

We never use it anyway.

This code has been introduced in
410306b, but it didn't really do anything back then either, as far as I can tell.

Fixes #57135

We never use it anyway.

This code has been introduced in
410306b, but it didn't really do
anything back then either, as far as I can tell.

Fixes llvm#57135
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 28, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 28, 2023

@llvm/pr-subscribers-clang

Changes

We never use it anyway.

This code has been introduced in
410306b, but it didn't really do anything back then either, as far as I can tell.

Fixes #57135


Full diff: https://github.com/llvm/llvm-project/pull/67716.diff

1 Files Affected:

  • (modified) clang/lib/AST/ExprConstant.cpp (+2-10)
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index fea06b97259fe31..04a751f1b4d09fb 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -10950,16 +10950,8 @@ bool ArrayExprEvaluator::VisitCXXParenListOrInitListExpr(
 }
 
 bool ArrayExprEvaluator::VisitArrayInitLoopExpr(const ArrayInitLoopExpr *E) {
-  LValue CommonLV;
-  if (E->getCommonExpr() &&
-      !Evaluate(Info.CurrentCall->createTemporary(
-                    E->getCommonExpr(),
-                    getStorageType(Info.Ctx, E->getCommonExpr()),
-                    ScopeKind::FullExpression, CommonLV),
-                Info, E->getCommonExpr()->getSourceExpr()))
-    return false;
-
-  auto *CAT = cast<ConstantArrayType>(E->getType()->castAsArrayTypeUnsafe());
+  const auto *CAT =
+      cast<ConstantArrayType>(E->getType()->castAsArrayTypeUnsafe());
 
   uint64_t Elements = CAT->getSize().getZExtValue();
   Result = APValue(APValue::UninitArray(), Elements, Elements);

@zygoloid
Copy link
Collaborator

zygoloid commented Sep 28, 2023

The semantics of ArrayInitLoopExpr are to first evaluate (once, up-front) the common expression, and then evaluate the subexpression once for each array element, where the subexpression can make repeated reference to the value of the common expression. With this change, we will instead evaluate the common expression once for each iteration of the loop. I would expect this change will cause us to miscompile this example:

struct X {
    int arr[3];
};
constexpr X f(int &r) {
    return {++r, ++r, ++r};
}
constexpr int g() {
    int n = 0;
    auto [a, b, c] = f(n).arr;
    return a + b + c;
}
static_assert(g() == 6)

... because we'll interpret the initializer of [a, b, c] as {f(n).arr[0], f(n).arr[1], f(n).arr[2]} instead of as <common> = f(n).arr; {<common>[0], <common>[1], <common>[2]}.

@tbaederr
Copy link
Contributor Author

Ah, didn't realize that. We should have a test case for this.

@tbaederr tbaederr closed this Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang] Assertion failure in case of nested ArrayInitLoopExpr using -std=c++17
3 participants