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

[tbaeder]

Summary

We need to evaluated the RHS before the LHS.

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

623–625

In C, the evaluation of the operands are unsequenced. C doesn't currently have constexpr functions, only constexpr objects, but that eliminates mutating operations like compound assignment... for the moment. Perhaps a FIXME comment for figuring out how to handle C?

(The situation I'm worried about in C is with UB dealing with unsequenced operations, like rejecting: https://godbolt.org/z/W11jchrKc)

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

623–625

Could C make them sequenced when introducing constexpr functions? :)

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

623–625

Since we already emit a warning for this, we could in the future just check if the statement is in a constexpr function and emit an error instead? We're emitting the warning for c++ pre-17 as well but we don't make it an error, I guess because it's not UB there?

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

623–625

If we're actually leaving the operations unsequenced before C++17, then we should reject that code because it is UB: http://eel.is/c++draft/basic#intro.execution-10

The wording in C++14 for assignment operations is:

In all cases, the assignment is sequenced after the value computation of the right and left operands, and before the value computation of the assignment expression.

So the left and right operands are unsequenced relative to one another.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

623–625

I was looking at the existing implementation when writing this patch: https://github.com/llvm/llvm-project/blob/1a7a00bdc99fa2b2ca19ecd2d1069991b3c1006b/clang/lib/AST/ExprConstant.cpp#L8545-L8568 which always seems to evaluate RHS first (and actually abort for C++ <= 14, but for unrelated reasons, probably because this statement is just not supported in a constexpr context there).

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

623–625

I think the existing implementation is incorrect to accept this in C++14 and earlier:

https://eel.is/c++draft/basic.exec#intro.execution-10.sentence-4
http://eel.is/c++draft/expr.const#5.8

[shafik]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

623–625

I filed a bug on this a while ago that we don't catch unsequenced modifications in constant expressions contexts: https://github.com/llvm/llvm-project/issues/37768

I am almost sure I had a conversation with @rsmith about this.

[tbaeder]

Ping

[aaron.ballman]

LGTM with an extra comment added.

clang/lib/AST/Interp/ByteCodeExprGen.cpp

623–625

Let's add the FIXME comment here as well and come back to address the issue with sequencing later.

clang/lib/AST/Interp/ByteCodeExprGen.cpp

bool ByteCodeExprGen<Emitter>::VisitFloatCompoundAssignOperator(
   QualType LHSComputationType = E->getComputationLHSType();
   QualType ResultType = E->getComputationResultType();
   std::optional<PrimType> LT = classify(LHSComputationType);
   std::optional<PrimType> RT = classify(ResultType);
 
   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;
 
   // If necessary, convert LHS to its computation type.
   if (LHS->getType() != LHSComputationType) {
     const auto *TargetSemantics = &Ctx.getFloatSemantics(LHSComputationType);
 
     if (!this->emitCastFP(TargetSemantics, RM, E))
       return false;
   }
 
   // Now load RHS.
-  if (!visit(RHS))
+  if (!this->emitGetLocal(*RT, TempOffset, E))
     return false;
 
   switch (E->getOpcode()) {
   case BO_AddAssign:
     if (!this->emitAddf(RM, E))
       return false;
     break;
   case BO_SubAssign:
bool ByteCodeExprGen<Emitter>::VisitCompoundAssignOperator(
   std::optional<PrimType> ResultT = classify(E->getType());
 
   if (!LT || !RT || !ResultT || !LHSComputationT)
     return false;
 
   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.
   switch (E->getOpcode()) {
   case BO_AddAssign:
     if (!this->emitAdd(*LHSComputationT, E))
       return false;
     break;

clang/test/AST/Interp/floats.cpp

namespace compound {
   static_assert(f1() == 2, "");
 
   constexpr float f2() {
     float f = __FLT_MAX__;
     f += 1.0;
     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 {
   constexpr float a() {
     float f = 0.0;
     assert(++f == 1.0);
     assert(f == 1.0);
     ++f;

clang/test/AST/Interp/literals.cpp

namespace IncDec {
 
   constexpr int bug1Dec() {
     int a = 3;
     int b = a--;
     return b;
   }
   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
 
 namespace CompoundLiterals {
   constexpr int get5() {
     return (int[]){1,2,3,4,5}[4];
   }
   static_assert(get5() == 5, "");