[clang] a1c0e3b - [clang][Interp] Fix compound assign operator evaluation order

[Timm Bäder via cfe-commits]

Author: Timm Bäder
Date: 2023-07-20T15:58:01+02:00
New Revision: a1c0e3be6f09be1315c21911b646eb9ddc5048c4

URL: https://github.com/llvm/llvm-project/commit/a1c0e3be6f09be1315c21911b646eb9ddc5048c4
DIFF: https://github.com/llvm/llvm-project/commit/a1c0e3be6f09be1315c21911b646eb9ddc5048c4.diff

LOG: [clang][Interp] Fix compound assign operator evaluation order

We need to evaluated the RHS before the LHS.

Differential Revision: https://reviews.llvm.org/D149550

Added: 
    

Modified: 
    clang/lib/AST/Interp/ByteCodeExprGen.cpp
    clang/test/AST/Interp/floats.cpp
    clang/test/AST/Interp/literals.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index ad838755c47a58..b5df00e245aaf9 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -620,10 +620,19 @@ bool ByteCodeExprGen<Emitter>::VisitFloatCompoundAssignOperator(
   if (!LT || !RT)
     return false;
 
+  // C++17 onwards require that we evaluate the RHS first.
+  // Compute RHS and save it in a temporary variable so we can
+  // load it again later.
+  if (!visit(RHS))
+    return false;
+
+  unsigned TempOffset = this->allocateLocalPrimitive(E, *RT, /*IsConst=*/true);
+  if (!this->emitSetLocal(*RT, TempOffset, E))
+    return false;
+
   // First, visit LHS.
   if (!visit(LHS))
     return false;
-
   if (!this->emitLoad(*LT, E))
     return false;
 
@@ -636,7 +645,7 @@ bool ByteCodeExprGen<Emitter>::VisitFloatCompoundAssignOperator(
   }
 
   // Now load RHS.
-  if (!visit(RHS))
+  if (!this->emitGetLocal(*RT, TempOffset, E))
     return false;
 
   switch (E->getOpcode()) {
@@ -734,18 +743,32 @@ bool ByteCodeExprGen<Emitter>::VisitCompoundAssignOperator(
   assert(!E->getType()->isPointerType() && "Handled above");
   assert(!E->getType()->isFloatingType() && "Handled above");
 
-  // Get LHS pointer, load its value and get RHS value.
+  // C++17 onwards require that we evaluate the RHS first.
+  // Compute RHS and save it in a temporary variable so we can
+  // load it again later.
+  // FIXME: Compound assignments are unsequenced in C, so we might
+  //   have to figure out how to reject them.
+  if (!visit(RHS))
+    return false;
+
+  unsigned TempOffset = this->allocateLocalPrimitive(E, *RT, /*IsConst=*/true);
+
+  if (!this->emitSetLocal(*RT, TempOffset, E))
+    return false;
+
+  // Get LHS pointer, load its value and cast it to the
+  // computation type if necessary.
   if (!visit(LHS))
     return false;
   if (!this->emitLoad(*LT, E))
     return false;
-  // If necessary, cast LHS to its computation type.
   if (*LT != *LHSComputationT) {
     if (!this->emitCast(*LT, *LHSComputationT, E))
       return false;
   }
 
-  if (!visit(RHS))
+  // Get the RHS value on the stack.
+  if (!this->emitGetLocal(*RT, TempOffset, E))
     return false;
 
   // Perform operation.

diff  --git a/clang/test/AST/Interp/floats.cpp b/clang/test/AST/Interp/floats.cpp
index fa1f52317988db..05081c7516877a 100644
--- a/clang/test/AST/Interp/floats.cpp
+++ b/clang/test/AST/Interp/floats.cpp
@@ -86,6 +86,18 @@ namespace compound {
     return f;
   }
   static_assert(f2() == __FLT_MAX__, "");
+
+  constexpr float ff() {
+    float a[] = {1,2};
+    int i = 0;
+
+    // RHS should be evaluated before LHS, so this should
+    // write to a[1];
+    a[i++] += ++i;
+
+    return a[1];
+  }
+  static_assert(ff() == 3, "");
 }
 
 namespace unary {

diff  --git a/clang/test/AST/Interp/literals.cpp b/clang/test/AST/Interp/literals.cpp
index 882742b2314b4c..b9d1d2d59d11a3 100644
--- a/clang/test/AST/Interp/literals.cpp
+++ b/clang/test/AST/Interp/literals.cpp
@@ -764,6 +764,17 @@ namespace IncDec {
   }
   static_assert(bug1Dec() == 3);
 
+  constexpr int f() {
+    int a[] = {1,2};
+    int i = 0;
+
+    // RHS should be evaluated before LHS, so this should
+    // write to a[1];
+    a[i++] += ++i;
+
+    return a[1];
+  }
+  static_assert(f() == 3, "");
 };
 #endif