[lldb][bytecode] Make formatter bytecode comparisons sign-correct for Int/UInt

[kastiglione]

op_eq/neq/lt/gt/le/ge previously required both operands to be the same variant
(Int or UInt), producing an error on mismatches. Replace the shared BINOP_IMPL
with a comparison-specific CMPOP that accepts any combination of Int/UInt and
compares by true mathematical value, instead of by reinterpreting one operand's
bits as the other's type. A negative Int now always compares less than any
UInt, and a UInt with the high bit set (>= 2^63) always compares greater than
any Int, matching ordinary integer comparison semantics. The comparison result
is always pushed as UInt.

[llvmorg-github-actions]

@llvm/pr-subscribers-lldb

Author: Dave Lee (kastiglione)

Changes

op_eq/neq/lt/gt/le/ge previously required both operands to be the same variant
(Int or UInt), producing an error on mismatches. Replace the shared BINOP_IMPL
with a comparison-specific CMPOP that accepts any combination of Int/UInt and
compares by true mathematical value, instead of by reinterpreting one operand's
bits as the other's type. A negative Int now always compares less than any
UInt, and a UInt with the high bit set (>= 2^63) always compares greater than
any Int, matching ordinary integer comparison semantics. The comparison result
is always pushed as UInt.

---

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

2 Files Affected:

• (modified) lldb/source/DataFormatters/FormatterBytecode.cpp (+51-6)
• (modified) lldb/unittests/DataFormatter/FormatterBytecodeTest.cpp (+51)

diff --git a/lldb/source/DataFormatters/FormatterBytecode.cpp b/lldb/source/DataFormatters/FormatterBytecode.cpp
index 2d79641f516f1..91fb037ea870a 100644
--- a/lldb/source/DataFormatters/FormatterBytecode.cpp
+++ b/lldb/source/DataFormatters/FormatterBytecode.cpp
@@ -17,6 +17,7 @@
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormatProviders.h"
 #include "llvm/Support/FormatVariadicDetails.h"
+#include <optional>
 
 using namespace lldb;
 namespace lldb_private {
@@ -183,6 +184,40 @@ static llvm::Error TypeCheck(llvm::ArrayRef<DataStackElement> data,
   return TypeCheck(data.drop_back(1), type2, type1);
 }
 
+/// Three-way compare two 64-bit integers as mathematical integers, i.e. without
+/// reinterpreting one's bit pattern as the other's type. A negative Int always
+/// compares less than any UInt, and a UInt with the high bit set (>= 2^63)
+/// always compares greater than any Int: neither is truncated or reinterpreted.
+static std::optional<int> Compare(const DataStackElement &x,
+                                  const DataStackElement &y) {
+  if (auto *ux = std::get_if<uint64_t>(&x)) {
+    if (auto *uy = std::get_if<uint64_t>(&y))
+      return Compare(*ux, *uy);
+    if (auto *sy = std::get_if<int64_t>(&y))
+      return Compare(*ux, *sy);
+  } else if (auto *sx = std::get_if<int64_t>(&x)) {
+    if (auto *sy = std::get_if<int64_t>(&y))
+      return Compare(*sx, *sy);
+    if (auto *uy = std::get_if<uint64_t>(&y))
+      return Compare(*sx, *uy);
+  }
+  return std::nullopt;
+}
+
+static int Compare(uint64_t x, uint64_t y) {
+
+  return x < y ? -1 : (x > y ? 1 : 0);
+}
+static int Compare(int64_t x, int64_t y) {
+  return x < y ? -1 : (x > y ? 1 : 0);
+}
+static int Compare(int64_t x, uint64_t y) {
+  if (x < 0)
+    return -1;
+  return Compare((uint64_t)x, y);
+}
+static int Compare(uint64_t x, int64_t y) { return -Compare(y, x); }
+
 llvm::Error Interpret(ControlStack &control, DataStack &data, Signatures sig) {
   if (control.empty())
     return llvm::Error::success();
@@ -423,22 +458,32 @@ llvm::Error Interpret(ControlStack &control, DataStack &data, Signatures sig) {
       data.Push(~data.Pop<uint64_t>());
       continue;
     case op_eq:
-      BINOP(==);
+#define CMPOP(OP)                                                              \
+  {                                                                            \
+    TYPE_CHECK(Any, Any);                                                      \
+    auto y = data.PopAny();                                                    \
+    auto x = data.PopAny();                                                    \
+    std::optional<int> cmp = Compare(x, y);                                    \
+    if (!cmp)                                                                  \
+      return error("invalid comparison data types");                           \
+    data.Push((uint64_t)(*cmp OP 0));                                          \
+  }
+      CMPOP(==);
       continue;
     case op_neq:
-      BINOP(!=);
+      CMPOP(!=);
       continue;
     case op_lt:
-      BINOP(<);
+      CMPOP(<);
       continue;
     case op_gt:
-      BINOP(>);
+      CMPOP(>);
       continue;
     case op_le:
-      BINOP(<=);
+      CMPOP(<=);
       continue;
     case op_ge:
-      BINOP(>=);
+      CMPOP(>=);
       continue;
     case op_call: {
       TYPE_CHECK(Selector);
diff --git a/lldb/unittests/DataFormatter/FormatterBytecodeTest.cpp b/lldb/unittests/DataFormatter/FormatterBytecodeTest.cpp
index 20c90bfe6fb60..21a9b02251d4b 100644
--- a/lldb/unittests/DataFormatter/FormatterBytecodeTest.cpp
+++ b/lldb/unittests/DataFormatter/FormatterBytecodeTest.cpp
@@ -249,6 +249,57 @@ TEST_F(FormatterBytecodeTest, ArithOps) {
     ASSERT_TRUE(Interpret({op_lit_uint, 1, op_lit_uint, 1, op_ge}, data));
     ASSERT_EQ(data.Pop<uint64_t>(), 1u);
   }
+  // Comparisons between a negative Int and a UInt must compare by mathematical
+  // value, not by reinterpreting one's bits as the other's type: a negative Int
+  // is always less than any UInt, and a UInt with the high bit set is always
+  // greater than any Int.
+  {
+    // -1 > 5u == 0u
+    DataStack data;
+    data.Push((int64_t)-1);
+    data.Push((uint64_t)5);
+    ASSERT_TRUE(Interpret({op_gt}, data));
+    ASSERT_EQ(data.Pop<uint64_t>(), 0u);
+  }
+  {
+    // -1 < 5u == 1u
+    DataStack data;
+    data.Push((int64_t)-1);
+    data.Push((uint64_t)5);
+    ASSERT_TRUE(Interpret({op_lt}, data));
+    ASSERT_EQ(data.Pop<uint64_t>(), 1u);
+  }
+  {
+    // 5u > -1 == 1u
+    DataStack data;
+    data.Push((uint64_t)5);
+    data.Push((int64_t)-1);
+    ASSERT_TRUE(Interpret({op_gt}, data));
+    ASSERT_EQ(data.Pop<uint64_t>(), 1u);
+  }
+  {
+    // UINT64_MAX > INT64_MAX == 1u
+    DataStack data;
+    data.Push((uint64_t)UINT64_MAX); // UINT64_MAX, >= 2^63.
+    data.Push((int64_t)INT64_MAX);
+    ASSERT_TRUE(Interpret({op_gt}, data));
+    ASSERT_EQ(data.Pop<uint64_t>(), 1u);
+  }
+  {
+    // INT64_MAX < UINT64_MAX == 1u
+    DataStack data;
+    data.Push((int64_t)INT64_MAX);
+    data.Push((uint64_t)UINT64_MAX); // UINT64_MAX, >= 2^63.
+    ASSERT_TRUE(Interpret({op_lt}, data));
+    ASSERT_EQ(data.Pop<uint64_t>(), 1u);
+  }
+  {
+    DataStack data;
+    data.Push((int64_t)5);
+    data.Push((uint64_t)5);
+    ASSERT_TRUE(Interpret({op_eq}, data));
+    ASSERT_EQ(data.Pop<uint64_t>(), 1u);
+  }
 }
 
 TEST_F(FormatterBytecodeTest, CallOps) {

[Michael137]

Basically a reimplementation of the C++20 cmp_less/etc.? Shame we can't use those here. But maybe a FIXME to replace this once it's available? Didn't find anything in the LLVM support library. Closest was the comparison on ScaledNumber

[Michael137]

Could this be a helper function instead of an inline macro?