[Lldb-commits] [PATCH] D82772: [lldb] Fix type conversion in the Scalar getters

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb725142c8db8: [lldb] Fix type conversion in the Scalar 
getters (authored by labath).

Changed prior to commit:
  https://reviews.llvm.org/D82772?vs=274127=275143#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82772/new/

https://reviews.llvm.org/D82772

Files:
  lldb/include/lldb/Utility/Scalar.h
  lldb/source/Core/ValueObject.cpp
  lldb/source/Expression/IRInterpreter.cpp
  lldb/source/Utility/Scalar.cpp
  lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py
  lldb/unittests/Utility/ScalarTest.cpp

Index: lldb/unittests/Utility/ScalarTest.cpp
===
--- lldb/unittests/Utility/ScalarTest.cpp
+++ lldb/unittests/Utility/ScalarTest.cpp
@@ -66,6 +66,44 @@
   ASSERT_TRUE(s2 >= s1);
 }
 
+template 
+static T2 ConvertHost(T1 val, T2 (Scalar::*)(T2) const) {
+  return T2(val);
+}
+
+template 
+static T2 ConvertScalar(T1 val, T2 (Scalar::*conv)(T2) const) {
+  return (Scalar(val).*conv)(T2());
+}
+
+template  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(0x87654321);
+  CheckConversion(0x87654321u);
+  CheckConversion(0x87654321l);
+  CheckConversion(0x87654321ul);
+  CheckConversion(0x8765432112345678ll);
+  CheckConversion(0x8765432112345678ull);
+  CheckConversion(42.25f);
+  CheckConversion(42.25);
+}
+
 TEST(ScalarTest, RightShiftOperator) {
   int a = 0x1000;
   int b = 0x;
@@ -336,11 +374,11 @@
 TEST(ScalarTest, TruncOrExtendTo) {
   Scalar S(0x);
   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(), 0xfu);
+  EXPECT_EQ(S.UInt128(APInt()), APInt(20, 0xfu));
   S.TruncOrExtendTo(24, false);
-  EXPECT_EQ(S.ULong(), 0x0fu);
+  EXPECT_EQ(S.UInt128(APInt()), APInt(24, 0x0fu));
   S.TruncOrExtendTo(16, false);
-  EXPECT_EQ(S.ULong(), 0xu);
+  EXPECT_EQ(S.UInt128(APInt()), APInt(16, 0xu));
 }
Index: lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py
===
--- lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py
+++ lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py
@@ -51,7 +51,8 @@
 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",
@@ -61,7 +62,8 @@
"$i << $j",
"$i & $j",
"$i | $j",
-   "$i ^ $j"]
+   "$i ^ $j",
+   "($ull & -1) == $u"]
 
 for expression in set_up_expressions:
 self.frame().EvaluateExpression(expression, options)
Index: lldb/source/Utility/Scalar.cpp
===
--- lldb/source/Utility/Scalar.cpp
+++ lldb/source/Utility/Scalar.cpp
@@ -644,73 +644,58 @@
   return success;
 }
 
-template  T Scalar::GetAsSigned(T fail_value) const {
-  switch (GetCategory(m_type)) {
-  case Category::Void:
-break;
-  case Category::Integral:
-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, );
-return result.getSExtValue();
-  }
-  }
-  return fail_value;
-}
-
-template  T Scalar::GetAsUnsigned(T fail_value) const {
+template  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();
 return 

[Lldb-commits] [PATCH] D82772: [lldb] Fix type conversion in the Scalar getters

[Pavel Labath via Phabricator via lldb-commits]

labath marked 3 inline comments as done.
labath added inline comments.



Comment at: lldb/source/Utility/Scalar.cpp:864
   case e_uint512:
-return static_cast(
-llvm::APIntOps::RoundAPIntToDouble(m_integer));

teemperor wrote:
> 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.
Yeah, I think this ended up being copied from the function above it.



Comment at: 
lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py:66
+   "$i ^ $j",
+   "($ull & -1) == $u"]
 

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



Comment at: lldb/unittests/Utility/ScalarTest.cpp:97
+TEST(ScalarTest, Getters) {
+  CheckConversion((int)0x87654321);
+  CheckConversion((unsigned int)0x87654321u);

teemperor wrote:
> If you change this to `CheckConversion(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?
Changed to `(T)` to ``. `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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82772/new/

https://reviews.llvm.org/D82772



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D82772: [lldb] Fix type conversion in the Scalar getters

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82772/new/

https://reviews.llvm.org/D82772



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D82772: [lldb] Fix type conversion in the Scalar getters

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

Beside some minor things this LGTM.




Comment at: lldb/source/Utility/Scalar.cpp:864
   case e_uint512:
-return static_cast(
-llvm::APIntOps::RoundAPIntToDouble(m_integer));

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.



Comment at: lldb/unittests/Utility/ScalarTest.cpp:97
+TEST(ScalarTest, Getters) {
+  CheckConversion((int)0x87654321);
+  CheckConversion((unsigned int)0x87654321u);

If you change this to `CheckConversion(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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82772/new/

https://reviews.llvm.org/D82772



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D82772: [lldb] Fix type conversion in the Scalar getters

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82772/new/

https://reviews.llvm.org/D82772



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D82772: [lldb] Fix type conversion in the Scalar getters

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82772/new/

https://reviews.llvm.org/D82772



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D82772: [lldb] Fix type conversion in the Scalar getters

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82772/new/

https://reviews.llvm.org/D82772



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D82772: [lldb] Fix type conversion in the Scalar getters

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

+ Ismail as he looked at this recently.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82772/new/

https://reviews.llvm.org/D82772



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D82772: [lldb] Fix type conversion in the Scalar getters

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: teemperor, JDevlieghere.
Herald added a project: LLDB.

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)0x
  and not (unsigned long)0x

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82772

Files:
  lldb/include/lldb/Utility/Scalar.h
  lldb/source/Core/ValueObject.cpp
  lldb/source/Expression/IRInterpreter.cpp
  lldb/source/Utility/Scalar.cpp
  lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py
  lldb/unittests/Utility/ScalarTest.cpp

Index: lldb/unittests/Utility/ScalarTest.cpp
===
--- lldb/unittests/Utility/ScalarTest.cpp
+++ lldb/unittests/Utility/ScalarTest.cpp
@@ -66,6 +66,44 @@
   ASSERT_TRUE(s2 >= s1);
 }
 
+template 
+static T2 ConvertHost(T1 val, T2 (Scalar::*)(T2) const) {
+  return T2(val);
+}
+
+template 
+static T2 ConvertScalar(T1 val, T2 (Scalar::*conv)(T2) const) {
+  return (Scalar(val).*conv)(T2());
+}
+
+template
+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));
+}
+
+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 = 0x1000;
   int b = 0x;
@@ -324,11 +362,11 @@
 TEST(ScalarTest, TruncOrExtendTo) {
   Scalar S(0x);
   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(), 0xfu);
+  EXPECT_EQ(S.UInt128(APInt()), APInt(20, 0xfu));
   S.TruncOrExtendTo(24, false);
-  EXPECT_EQ(S.ULong(), 0x0fu);
+  EXPECT_EQ(S.UInt128(APInt()), APInt(24, 0x0fu));
   S.TruncOrExtendTo(16, false);
-  EXPECT_EQ(S.ULong(), 0xu);
+  EXPECT_EQ(S.UInt128(APInt()), APInt(16, 0xu));
 }
Index: lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py
===
--- lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py
+++ lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py
@@ -51,7 +51,8 @@
 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",
@@ -61,7 +62,8 @@
"$i << $j",
"$i & $j",
"$i | $j",
-   "$i ^ $j"]
+   "$i ^ $j",
+   "($ull & -1) == $u"]
 
 for expression in set_up_expressions:
 self.frame().EvaluateExpression(expression, options)
Index: lldb/source/Utility/Scalar.cpp