[lldb] Fix type conversion in the Scalar getters

[labath]

Summary

The Scalar class claims to follow the C type conversion rules. This is
true for the Promote function, but it is not true for the implicit
conversions done in the getter methods.

These functions had a subtle bug: when extending the type, they used the
signedness of the *target* type in order to determine whether to do
sign-extension or zero-extension. This is not how things work in C,
which uses the signedness of the *source* type. I.e., C does
(sign-)extension before it does signed->unsigned conversion, and not the
other way around.

This means that: (unsigned long)(int)-1

is equal to (unsigned long)0xffffffffffffffff
and not (unsigned long)0x00000000ffffffff

Unsurprisingly, we have accumulated code which depended on this
inconsistent behavior. It mainly manifested itself as code calling
"ULongLong/SLongLong" as a way to get the value of the Scalar object in
a primitive type that is "large enough". Previously, the ULongLong
conversion did not do sign-extension, but now it does.

This patch makes the Scalar getters consistent with the declared
semantics, and fixes the couple of call sites that were using it
incorrectly.

[JDevlieghere]

+ Ismail as he looked at this recently.

[labath]

Another possibility would be to rename [US]LongLong to something like Get[SZ]ExtInteger (similar to APInt::get[SZ]ExtValue) to make it clear what it does, and ditch the other getter functions. I've mean meaning to move this class away from using host types at some point (it's not appropriate or useful for most of our uses), but I've thought it would be good to clean it up first. Now I think it may be better to do it the other way around....

[mib]

Hey Pavel! Thanks for taking care of this. I started looking at it no long ago but I got sidetracked working on other stuff.
I don't see anything wrong with the patch, looks good to me.

[labath]

Thanks for looking at this. Could you also tell me what you think of D82864? That's sort of a more safe/NFC-ish alternative to this patch, as it doesn't change behavior -- it just makes the current behavior more obvious. (It's technically possible for the removal of the [US]Short and co. accessors to change behavior, but that would require a pretty complex set of conditions).

[teemperor]

Beside some minor things this LGTM.

lldb/source/Utility/Scalar.cpp

838

Seems unrelated to the patch? Also it would be inconsistent that this is removed here and not below. FWIW this is used to suppress -Wdouble-promotion warnings, so it does have a purpose.

lldb/unittests/Utility/ScalarTest.cpp

97

If you change this to CheckConversion<int>(0x87654321); then that's one C-style cast less (which will make me very happy) and if someone accidentally makes this a long literal or so we get a compiler warning (instead of the compiler just silently truncating the thing).

Also I guess short and char is missing?

[JDevlieghere]

LGTM with Raphael's and clang-format's comments addressed :-)

[labath]

lldb/source/Utility/Scalar.cpp

838

Yeah, I think this ended up being copied from the function above it.

lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py

66

FTR, this was working correctly before already. I'm just creating a more explicit test for it.

lldb/unittests/Utility/ScalarTest.cpp

97

Changed to (T) to <T>. short and char aren't interesting to test because Scalar does not have first-class support for them. They would get promoted to int before they reach the Scalar constructor. Long double is also missing -- that's because the relevant constructor is currently a booby-trap.

lldb/include/lldb/Utility/Scalar.h

protected:
   typedef double double_t;
   typedef long double long_double_t;
 
   // Classes that inherit from Scalar can see and modify these
   Scalar::Type m_type;
   llvm::APInt m_integer;
   llvm::APFloat m_float;
 
-  template <typename T> T GetAsSigned(T fail_value) const;
-  template <typename T> T GetAsUnsigned(T fail_value) const;
+  template <typename T> T GetAs(T fail_value) const;
 
 private:
   friend const Scalar operator+(const Scalar &lhs, const Scalar &rhs);
   friend const Scalar operator-(const Scalar &lhs, const Scalar &rhs);
   friend const Scalar operator/(const Scalar &lhs, const Scalar &rhs);
   friend const Scalar operator*(const Scalar &lhs, const Scalar &rhs);
   friend const Scalar operator&(const Scalar &lhs, const Scalar &rhs);
   friend const Scalar operator|(const Scalar &lhs, const Scalar &rhs);

lldb/source/Core/ValueObject.cpp

 // address values out of pointers
 uint64_t ValueObject::GetValueAsUnsigned(uint64_t fail_value, bool *success) {
   // If our byte size is zero this is an aggregate type that has children
   if (CanProvideValue()) {
     Scalar scalar;
     if (ResolveValue(scalar)) {
       if (success)
         *success = true;
+      scalar.MakeUnsigned();
       return scalar.ULongLong(fail_value);
     }
     // fallthrough, otherwise...
   }
 
   if (success)
     *success = false;
   return fail_value;
 }
 
 int64_t ValueObject::GetValueAsSigned(int64_t fail_value, bool *success) {
   // If our byte size is zero this is an aggregate type that has children
   if (CanProvideValue()) {
     Scalar scalar;
     if (ResolveValue(scalar)) {
       if (success)
         *success = true;
+      scalar.MakeSigned();
       return scalar.SLongLong(fail_value);
     }
     // fallthrough, otherwise...
   }
 
   if (success)
     *success = false;
   return fail_value;

lldb/source/Expression/IRInterpreter.cpp

if (i != m_values.end()) {
       lldb::addr_t addr = i->second;
 
       ss.Printf(" 0x%llx", (unsigned long long)addr);
     }
 
     return std::string(ss.GetString());
   }
 
-  bool AssignToMatchType(lldb_private::Scalar &scalar, uint64_t u64value,
+  bool AssignToMatchType(lldb_private::Scalar &scalar, llvm::APInt value,
                          Type *type) {
     size_t type_size = m_target_data.getTypeStoreSize(type);
 
     if (type_size > 8)
       return false;
 
     if (type_size != 1)
       type_size = PowerOf2Ceil(type_size);
 
-    scalar = llvm::APInt(type_size*8, u64value);
+    scalar = value.zextOrTrunc(type_size * 8);
     return true;
   }
 
   bool EvaluateValue(lldb_private::Scalar &scalar, const Value *value,
                      Module &module) {
     const Constant *constant = dyn_cast<Constant>(value);
 
     if (constant) {
       APInt value_apint;
 
       if (!ResolveConstantValue(value_apint, constant))
         return false;
 
-      return AssignToMatchType(scalar, value_apint.getLimitedValue(),
-                               value->getType());
+      return AssignToMatchType(scalar, value_apint, value->getType());
     }
 
     lldb::addr_t process_address = ResolveValue(value, module);
     size_t value_size = m_target_data.getTypeStoreSize(value->getType());
 
     lldb_private::DataExtractor value_extractor;
     lldb_private::Status extract_error;
 
     m_execution_unit.GetMemoryData(value_extractor, process_address,
                                    value_size, extract_error);
 
     if (!extract_error.Success())
       return false;
 
     lldb::offset_t offset = 0;
     if (value_size <= 8) {
       uint64_t u64value = value_extractor.GetMaxU64(&offset, value_size);
-      return AssignToMatchType(scalar, u64value, value->getType());
+      return AssignToMatchType(scalar, llvm::APInt(64, u64value),
+                               value->getType());
     }
 
     return false;
   }
 
-  bool AssignValue(const Value *value, lldb_private::Scalar &scalar,
+  bool AssignValue(const Value *value, lldb_private::Scalar scalar,
                    Module &module) {
     lldb::addr_t process_address = ResolveValue(value, module);
 
     if (process_address == LLDB_INVALID_ADDRESS)
       return false;
 
     lldb_private::Scalar cast_scalar;
 
-    if (!AssignToMatchType(cast_scalar, scalar.ULongLong(), value->getType()))
+    scalar.MakeUnsigned();
+    if (!AssignToMatchType(cast_scalar, scalar.UInt128(llvm::APInt()),
+                           value->getType()))
       return false;
 
     size_t value_byte_size = m_target_data.getTypeStoreSize(value->getType());
 
     lldb_private::DataBufferHeap buf(value_byte_size, 0);
 
     lldb_private::Status get_data_error;
 

lldb/source/Utility/Scalar.cpp

bool Scalar::MakeUnsigned() {
   case e_long_double:
     success = true;
     break;
   }
 
   return success;
 }
 
-template <typename T> T Scalar::GetAsSigned(T fail_value) const {
+template <typename T> T Scalar::GetAs(T fail_value) const {
   switch (GetCategory(m_type)) {
   case Category::Void:
     break;
   case Category::Integral:
+    if (IsSigned(m_type))
       return m_integer.sextOrTrunc(sizeof(T) * 8).getSExtValue();
-
-  case Category::Float: {
-    llvm::APSInt result(sizeof(T) * 8, /*isUnsigned=*/false);
-    bool isExact;
-    m_float.convertToInteger(result, llvm::APFloat::rmTowardZero, &isExact);
-    return result.getSExtValue();
-  }
-  }
-  return fail_value;
-}
-
-template <typename T> T Scalar::GetAsUnsigned(T fail_value) const {
-  switch (GetCategory(m_type)) {
-  case Category::Void:
-    break;
-  case Category::Integral:
     return m_integer.zextOrTrunc(sizeof(T) * 8).getZExtValue();
   case Category::Float: {
-    llvm::APSInt result(sizeof(T) * 8, /*isUnsigned=*/true);
+    llvm::APSInt result(sizeof(T) * 8, std::is_unsigned<T>::value);
     bool isExact;
     m_float.convertToInteger(result, llvm::APFloat::rmTowardZero, &isExact);
-    return result.getZExtValue();
+    return result.getSExtValue();
   }
   }
   return fail_value;
 }
 
 signed char Scalar::SChar(signed char fail_value) const {
-  return GetAsSigned<signed char>(fail_value);
+  return GetAs<signed char>(fail_value);
 }
 
 unsigned char Scalar::UChar(unsigned char fail_value) const {
-  return GetAsUnsigned<unsigned char>(fail_value);
+  return GetAs<unsigned char>(fail_value);
 }
 
 short Scalar::SShort(short fail_value) const {
-  return GetAsSigned<short>(fail_value);
+  return GetAs<short>(fail_value);
 }
 
 unsigned short Scalar::UShort(unsigned short fail_value) const {
-  return GetAsUnsigned<unsigned short>(fail_value);
+  return GetAs<unsigned short>(fail_value);
 }
 
-int Scalar::SInt(int fail_value) const { return GetAsSigned<int>(fail_value); }
+int Scalar::SInt(int fail_value) const { return GetAs<int>(fail_value); }
 
 unsigned int Scalar::UInt(unsigned int fail_value) const {
-  return GetAsUnsigned<unsigned int>(fail_value);
+  return GetAs<unsigned int>(fail_value);
 }
 
-long Scalar::SLong(long fail_value) const { return GetAsSigned<long>(fail_value); }
+long Scalar::SLong(long fail_value) const { return GetAs<long>(fail_value); }
 
 unsigned long Scalar::ULong(unsigned long fail_value) const {
-  return GetAsUnsigned<unsigned long>(fail_value);
+  return GetAs<unsigned long>(fail_value);
 }
 
 long long Scalar::SLongLong(long long fail_value) const {
-  return GetAsSigned<long long>(fail_value);
+  return GetAs<long long>(fail_value);
 }
 
 unsigned long long Scalar::ULongLong(unsigned long long fail_value) const {
-  return GetAsUnsigned<unsigned long long>(fail_value);
+  return GetAs<unsigned long long>(fail_value);
 }
 
 llvm::APInt Scalar::SInt128(const llvm::APInt &fail_value) const {
   switch (m_type) {
   case e_void:
     break;
   case e_sint:
   case e_uint:
llvm::APInt Scalar::UInt128(const llvm::APInt &fail_value) const {
   return fail_value;
 }
 
 float Scalar::Float(float fail_value) const {
   switch (m_type) {
   case e_void:
     break;
   case e_sint:
-  case e_uint:
   case e_slong:
-  case e_ulong:
   case e_slonglong:
-  case e_ulonglong:
   case e_sint128:
-  case e_uint128:
   case e_sint256:
-  case e_uint256:
   case e_sint512:
+    return llvm::APIntOps::RoundSignedAPIntToFloat(m_integer);
+
+  case e_uint:
+  case e_ulong:
+  case e_ulonglong:
+  case e_uint128:
+  case e_uint256:
   case e_uint512:
     return llvm::APIntOps::RoundAPIntToFloat(m_integer);
+
   case e_float:
     return m_float.convertToFloat();
   case e_double:
     return static_cast<float_t>(m_float.convertToDouble());
   case e_long_double:
     llvm::APInt ldbl_val = m_float.bitcastToAPInt();
     return ldbl_val.bitsToFloat();
   }
   return fail_value;
 }
 
 double Scalar::Double(double fail_value) const {
   switch (m_type) {
   case e_void:
     break;
   case e_sint:
-  case e_uint:
   case e_slong:
-  case e_ulong:
   case e_slonglong:
-  case e_ulonglong:
   case e_sint128:
-  case e_uint128:
   case e_sint256:
-  case e_uint256:
   case e_sint512:
+    return llvm::APIntOps::RoundSignedAPIntToDouble(m_integer);
+
+  case e_uint:
+  case e_ulong:
+  case e_ulonglong:
+  case e_uint128:
+  case e_uint256:
   case e_uint512:
     return llvm::APIntOps::RoundAPIntToDouble(m_integer);
+
   case e_float:
     return static_cast<double_t>(m_float.convertToFloat());
   case e_double:
     return m_float.convertToDouble();
   case e_long_double:
     llvm::APInt ldbl_val = m_float.bitcastToAPInt();
     return ldbl_val.bitsToFloat();
   }
   return fail_value;
 }
 
 long double Scalar::LongDouble(long double fail_value) const {
   switch (m_type) {
   case e_void:
     break;
   case e_sint:
-  case e_uint:
   case e_slong:
-  case e_ulong:
   case e_slonglong:
-  case e_ulonglong:
   case e_sint128:
-  case e_uint128:
   case e_sint256:
-  case e_uint256:
   case e_sint512:
+    return static_cast<long_double_t>(
+        llvm::APIntOps::RoundSignedAPIntToDouble(m_integer));
+
+  case e_uint:
+  case e_ulong:
+  case e_ulonglong:
+  case e_uint128:
+  case e_uint256:
   case e_uint512:
     return static_cast<long_double_t>(
         llvm::APIntOps::RoundAPIntToDouble(m_integer));
+
   case e_float:
     return static_cast<long_double_t>(m_float.convertToFloat());
   case e_double:
     return static_cast<long_double_t>(m_float.convertToDouble());
   case e_long_double:
     llvm::APInt ldbl_val = m_float.bitcastToAPInt();
     return static_cast<long_double_t>(ldbl_val.bitsToDouble());
   }

lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py

@expectedFailureAll(
         bugnumber="http://llvm.org/pr21765")
     @expectedFailureNetBSD
     def test_ir_interpreter(self):
         self.build_and_run()
 
         options = lldb.SBExpressionOptions()
         options.SetLanguage(lldb.eLanguageTypeC_plus_plus)
 
-        set_up_expressions = ["int $i = 9", "int $j = 3", "int $k = 5"]
+        set_up_expressions = ["int $i = 9", "int $j = 3", "int $k = 5",
+            "unsigned long long $ull = -1", "unsigned $u = -1"]
 
         expressions = ["$i + $j",
                        "$i - $j",
                        "$i * $j",
                        "$i / $j",
                        "$i % $k",
                        "$i << $j",
                        "$i & $j",
                        "$i | $j",
-                       "$i ^ $j"]
+                       "$i ^ $j",
+                       "($ull & -1) == $u"]
 
         for expression in set_up_expressions:
             self.frame().EvaluateExpression(expression, options)
 
         for expression in expressions:
             interp_expression = expression
             jit_expression = "(int)getpid(); " + expression
 

lldb/unittests/Utility/ScalarTest.cpp

TEST(ScalarTest, ComparisonFloat) {
   auto s1 = Scalar(23.0f);
   auto s2 = Scalar(46.0f);
   ASSERT_TRUE(s1 < s2);
   ASSERT_TRUE(s1 <= s2);
   ASSERT_TRUE(s2 > s1);
   ASSERT_TRUE(s2 >= s1);
 }
 
+template <typename T1, typename T2>
+static T2 ConvertHost(T1 val, T2 (Scalar::*)(T2) const) {
+  return T2(val);
+}
+
+template <typename T1, typename T2>
+static T2 ConvertScalar(T1 val, T2 (Scalar::*conv)(T2) const) {
+  return (Scalar(val).*conv)(T2());
+}
+
+template <typename T> static void CheckConversion(T val) {
+  SCOPED_TRACE("val = " + std::to_string(val));
+  EXPECT_EQ((signed char)val, Scalar(val).SChar());
+  EXPECT_EQ((unsigned char)val, Scalar(val).UChar());
+  EXPECT_EQ((short)val, Scalar(val).SShort());
+  EXPECT_EQ((unsigned short)val, Scalar(val).UShort());
+  EXPECT_EQ((int)val, Scalar(val).SInt());
+  EXPECT_EQ((unsigned)val, Scalar(val).UInt());
+  EXPECT_EQ((long)val, Scalar(val).SLong());
+  EXPECT_EQ((unsigned long)val, Scalar(val).ULong());
+  EXPECT_EQ((long long)val, Scalar(val).SLongLong());
+  EXPECT_EQ((unsigned long long)val, Scalar(val).ULongLong());
+  EXPECT_NEAR((float)val, Scalar(val).Float(), std::abs(val / 1e6));
+  EXPECT_NEAR((double)val, Scalar(val).Double(), std::abs(val / 1e12));
+  EXPECT_NEAR((long double)val, Scalar(val).LongDouble(), std::abs(val / 1e12));
+}
+
+TEST(ScalarTest, Getters) {
+  CheckConversion<int>(0x87654321);
+  CheckConversion<unsigned int>(0x87654321u);
+  CheckConversion<long>(0x87654321l);
+  CheckConversion<unsigned long>(0x87654321ul);
+  CheckConversion<long long>(0x8765432112345678ll);
+  CheckConversion<unsigned long long>(0x8765432112345678ull);
+  CheckConversion<float>(42.25f);
+  CheckConversion<double>(42.25);
+}
+
 TEST(ScalarTest, RightShiftOperator) {
   int a = 0x00001000;
   int b = 0xFFFFFFFF;
   int c = 4;
   Scalar a_scalar(a);
   Scalar b_scalar(b);
   Scalar c_scalar(c);
   ASSERT_EQ(a >> c, a_scalar >> c_scalar);
TEST(ScalarTest, Scalar_512) {
   ASSERT_TRUE(S.MakeSigned());
   EXPECT_EQ(S.GetType(), Scalar::e_sint512);
   EXPECT_EQ(S.GetByteSize(), 64U);
 }
 
 TEST(ScalarTest, TruncOrExtendTo) {
   Scalar S(0xffff);
   S.TruncOrExtendTo(12, true);
-  EXPECT_EQ(S.ULong(), 0xfffu);
+  EXPECT_EQ(S.UInt128(APInt()), APInt(12, 0xfffu));
   S.TruncOrExtendTo(20, true);
-  EXPECT_EQ(S.ULong(), 0xfffffu);
+  EXPECT_EQ(S.UInt128(APInt()), APInt(20, 0xfffffu));
   S.TruncOrExtendTo(24, false);
-  EXPECT_EQ(S.ULong(), 0x0fffffu);
+  EXPECT_EQ(S.UInt128(APInt()), APInt(24, 0x0fffffu));
   S.TruncOrExtendTo(16, false);
-  EXPECT_EQ(S.ULong(), 0xffffu);
+  EXPECT_EQ(S.UInt128(APInt()), APInt(16, 0xffffu));
 }