Evaluate both operands of floating-point <=> exactly once. [GH #520]

src/lower_il.c

@@ -16328,6 +16328,7 @@ Floating-point types require additional run-time checking for NaN (see below).
   an_expr_node_ptr  op2 = op1->next;
   an_expr_node_ptr  temp1, temp2, one, zero, minus_one, cmp, unordered, eptr;
   a_boolean         op1_has_side_effects, op2_has_side_effects;
+  a_boolean         temp1_used, temp2_used;
   a_type_kind       type_kind = expr->variant.operation.type_kind;
 
   one = spaceship_result_constant_expr(1, expr->type);
@@ -16360,15 +16361,24 @@ Floating-point types require additional run-time checking for NaN (see below).
     minus_one = spaceship_result_constant_expr(-1, expr->type);
     op1_has_side_effects = node_has_side_effects(op1, (a_boolean *)NULL);
     op2_has_side_effects = node_has_side_effects(op2, (a_boolean *)NULL);
-    temp1 = make_reusable_copy(op1, op1_has_side_effects);
-    temp2 = make_reusable_copy(op2, op2_has_side_effects);
+    temp1 = make_reusable_copy_full(op1, op1_has_side_effects, &temp1_used,
+                                    /*treat_as_potential_prvalue=*/FALSE);
+    temp2 = make_reusable_copy_full(op2, op2_has_side_effects, &temp2_used,
+                                    /*treat_as_potential_prvalue=*/FALSE);
     if (type_kind_is_simple_float_like(type_kind)) {
       /* If either operand is a NaN, then return
-         std::partial_ordering::unordered:
-        (__isnan(temp1 = x) || __isnan(temp2 = y)) ? RT(unordered) :
+         std::partial_ordering::unordered.  Each operand is evaluated
+         exactly once, and the NaN test and the comparisons use the same
+         operand values:
+        ((temp1 = x), (temp2 = y), __isnan(temp1) | __isnan(temp2))
+                                                   ? RT(unordered) :
                                      temp1 < temp2 ? RT(-1) :
                                      temp2 < temp1 ? RT(1)
                                                    : RT(0)
+         An operand that needs a temporary is assigned to it before the NaN
+         test.  An operand that needs no temporary is used directly, and its
+         assignment to temp1 or temp2 is omitted; it is read only by the NaN
+         test and the comparisons.
       */
       /* Note that the value of std::partial_ordering::unordered can vary
          based on the implementation.  Look for its definition in the headers
@@ -16386,6 +16396,22 @@ Floating-point types require additional run-time checking for NaN (see below).
       check_assertion(!ovflo);
       unordered = spaceship_result_constant_expr((int)unordered_val,
                                                  expr->type);
+      /* Make "__isnan(temp1) | __isnan(temp2)". */
+      eptr = make_isnan_call(make_reusable_copy(temp1,
+                                                /*vars_can_change=*/FALSE));
+      eptr->next = make_isnan_call(make_reusable_copy(temp2,
+                                                /*vars_can_change=*/FALSE));
+      eptr = make_operator_node(eok_or, integer_type(ik_int), eptr);
+      /* Evaluate the operands that were copied to temporaries before the
+         NaN test. */
+      op1->next = NULL;
+      op2->next = NULL;
+      if (temp2_used) {
+        eptr = make_comma_node(op2, eptr);
+      }  /* if */
+      if (temp1_used) {
+        eptr = make_comma_node(op1, eptr);
+      }  /* if */
       temp2->next = temp1;
       cmp = make_operator_node(eok_lt, integer_type(ik_int), temp2);
       cmp->next = one;
@@ -16398,11 +16424,6 @@ Floating-point types require additional run-time checking for NaN (see below).
       cmp = make_operator_node(eok_lt, integer_type(ik_int), temp1);
       cmp->next = minus_one;
       cmp = make_operator_node(eok_question, expr->type, cmp);
-      op2->next = NULL;
-      eptr = make_isnan_call(op2);
-      op1->next = NULL;
-      eptr->next = make_isnan_call(op1);
-      eptr = make_operator_node(eok_lor, integer_type(ik_int), eptr);
       unordered->next = cmp;
       eptr->next = unordered;
       set_node_operator(expr, eok_question, expr->type, expr->is_lvalue, eptr);

src/pending_changes/gh-520-spaceship-nan-eval-order.txt

@@ -0,0 +1,25 @@
+//allowed_words: NaN
+[GH #520]
+Lowering of <=> and the evaluation of its operands
+
+For floating-point operands, the lowering of <=> for the C-generating back
+end tested the right operand for a NaN before evaluating the left operand,
+using a short-circuit "||".  When the right operand was a NaN, the left
+operand was therefore not evaluated, and its side effects were lost.  Also,
+an operand without side effects could be read by the NaN test before the
+other operand was evaluated, and by the comparisons after it.  If the other
+operand changed it, the result could then be "equivalent" for a NaN.  For
+example, with --c++20:
+
+  #include <compare>
+
+  float x(), y();
+  auto f() { return x() <=> y(); }   // Previously x() was not called when
+                                     // y() returned a NaN.
+
+  float g, h();   // h() modifies g.
+  auto k() { return h() <=> g; }     // Previously could be "equivalent"
+                                     // when h() stores a NaN into g.
+
+Each operand is now evaluated exactly once, and the NaN test and the
+comparisons use the same operand values.

tests/tests/regressions/.spaceship-nan-eval-order.rto/default.1.1.txt

@@ -0,0 +1 @@
+eccp -DTEST_NUMBER=1 --c++20 --g++ Test_name.c

tests/tests/regressions/.spaceship-nan-eval-order.rto/default.1.2.txt

@@ -0,0 +1 @@
+./a.out

tests/tests/regressions/spaceship-nan-eval-order.sft.cpp

@@ -0,0 +1,59 @@
+//type:rp
+//options_all:--c++20 --g++
+
+// Lowering of <=> must evaluate both operands exactly once, and must compare
+// the same operand values in the NaN test and in the ordering tests.
+
+#include <compare>
+
+float nan_value() { return __builtin_nanf(""); }
+
+int calls;
+float one() { ++calls; return 1.0f; }
+
+float g;
+float set_g_to_nan() { g = nan_value(); return 0.0f; }
+
+int main() {
+  int failures = 0;
+
+  // The left operand is evaluated when the right operand is a NaN.
+  calls = 0;
+  if ((one() <=> nan_value()) != std::partial_ordering::unordered) {
+    failures |= 1;
+  }  // if
+  if (calls != 1) failures |= 2;
+
+  // The right operand is evaluated when the left operand is a NaN.
+  calls = 0;
+  if ((nan_value() <=> one()) != std::partial_ordering::unordered) {
+    failures |= 4;
+  }  // if
+  if (calls != 1) failures |= 8;
+
+  // g is read either before set_g_to_nan() (less) or after it (unordered),
+  // never both.
+  g = 1.0f;
+  std::partial_ordering r = set_g_to_nan() <=> g;
+  if (r != std::partial_ordering::less &&
+      r != std::partial_ordering::unordered) {
+    failures |= 16;
+  }  // if
+  g = 1.0f;
+  r = g <=> set_g_to_nan();
+  if (r != std::partial_ordering::greater &&
+      r != std::partial_ordering::unordered) {
+    failures |= 32;
+  }  // if
+
+  // Finite operands give less, equivalent, and greater, both with operands
+  // that are copied to temporaries (one()) and with one that is not (g).
+  calls = 0;
+  g = 2.0f;
+  if ((one() <=> g) != std::partial_ordering::less) failures |= 64;
+  if ((one() <=> one()) != std::partial_ordering::equivalent) failures |= 64;
+  if ((g <=> one()) != std::partial_ordering::greater) failures |= 64;
+  if (calls != 4) failures |= 128;
+
+  return failures;
+}