[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Andy Yankovsky via Phabricator via lldb-commits]

werat added a comment.



In D134493#3819710 , @shafik wrote:

> Note, in both C and C++ converting a `-1` to unsigned will always result in 
> the max unsigned value e.g.:

When you convert to "unsigned" then yeah, but here the API is `uint64_t 
GetAsUnsigned()`. I would expect this to be the equivalent of converting to 
`uint64_t`, not `unsigned`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

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

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

Note, in both C and C++ converting a `-1` to unsigned will always result in the 
max unsigned value e.g.:

  #include 
  #include 
  
  int main() {
int8_t i8 = -1;
int32_t i32 = -1;
  
unsigned x = i8;
std::cout << x << "\n";
  
x = i32;
std::cout << x << "\n";
  }

output:

  4294967295
  4294967295


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

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

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

In D134493#3814761 , @werat wrote:

> In D134493#3811253 , @labath wrote:
>
>> lol, I knew about the last two parts (not that I agree with them, but I 
>> think we have about an equal amount of code which relies on it, and that 
>> which tries to work around it), but the first one is really weird. I think 
>> we have invented a middle ground between sign- and zero-extension.
>
> haha, so this mean no chance of fixing this? I have a workaround for this as 
> well -- 
> https://github.com/google/lldb-eval/blob/04a73616c012c3dac7fb11206511bd2a9fe16db4/lldb-eval/value.cc#L146
> I can live with that, but current behaviour does look like a bug.

That does look like a bug, thanks for reporting. Was going to open an issue and 
take a look at it this week


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

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

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Andy Yankovsky via Phabricator via lldb-commits]

werat added a comment.

In D134493#3811253 , @labath wrote:

> lol, I knew about the last two parts (not that I agree with them, but I think 
> we have about an equal amount of code which relies on it, and that which 
> tries to work around it), but the first one is really weird. I think we have 
> invented a middle ground between sign- and zero-extension.

haha, so this mean no chance of fixing this? I have a workaround for this as 
well -- 
https://github.com/google/lldb-eval/blob/04a73616c012c3dac7fb11206511bd2a9fe16db4/lldb-eval/value.cc#L146
I can live with that, but current behaviour does look like a bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

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

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

lol, I knew about the last two parts (not that I agree with them, but I think 
we have about an equal amount of code which relies on it, and that which tries 
to work around it), but the first one is really weird. I think we have invented 
a middle ground between sign- and zero-extension.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

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

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Andy Yankovsky via Phabricator via lldb-commits]

werat added a comment.

On a somewhat related note, is it expected that `GetValueAsUnsigned()` performs 
an overflow as `int32` if the type is smaller that `int64`?

  ❯ cat overflow.cc 
  #include 
  int main() {
int8_t i8 = -1;
int32_t i32 = -1;
int64_t i64 = -1;
return 0;
  }
  
  (lldb) script
  
  >>> lldb.frame.FindVariable("i8").GetValueAsUnsigned()
  4294967295
  >>> lldb.frame.FindVariable("i32").GetValueAsUnsigned()
  4294967295
  >>> lldb.frame.FindVariable("i64").GetValueAsUnsigned()
  18446744073709551615
  >>>




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

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

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Michael Buch via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd5d904285008: [lldb][TypeSystemClang] Deduce 
lldb::eEncodingUint for unsigned enum types (authored by Michael137).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
  lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp


Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}
Index: 
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
===
--- /dev/null
+++ 
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+We test this by assigning to a bit-field a value
+that is out-of-range of it's signed counterpart.
+I.e., with a bit-field of width 4, assigning
+8 to it would be out-of-range if we treated it
+as a signed. If LLDB were to sign-extend the Scalar
+(which shouldn't happen for unsigned bit-fields)
+it would left-fill the result with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:


Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}
Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+We test this by assigning to a bit-field a value
+that is out-of-range of it's signed counterpart.
+I.e., with a bit-field of width 4, assigning
+8 to it would be out-of-range if we treated it
+as a signed. If LLDB were to sign-extend the Scalar
+(which shouldn't happen for unsigned bit-fields)
+it would left-fill the result with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Pavel Labath via Phabricator via lldb-commits]

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

Ok, sounds good then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

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

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Michael Buch via Phabricator via lldb-commits]

Michael137 updated this revision to Diff 462435.
Michael137 added a comment.

- Reword commit message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
  lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp


Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}
Index: 
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
===
--- /dev/null
+++ 
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+We test this by assigning to a bit-field a value
+that is out-of-range of it's signed counterpart.
+I.e., with a bit-field of width 4, assigning
+8 to it would be out-of-range if we treated it
+as a signed. If LLDB were to sign-extend the Scalar
+(which shouldn't happen for unsigned bit-fields)
+it would left-fill the result with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:


Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}
Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+We test this by assigning to a bit-field a value
+that is out-of-range of it's signed counterpart.
+I.e., with a bit-field of width 4, assigning
+8 to it would be out-of-range if we treated it
+as a signed. If LLDB were to sign-extend the Scalar
+(which shouldn't happen for unsigned bit-fields)
+it would left-fill the result with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:
___
lldb-commits mailing list
lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Michael Buch via Phabricator via lldb-commits]

Michael137 updated this revision to Diff 462434.
Michael137 added a comment.

- Fix test description


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
  lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp


Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}
Index: 
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
===
--- /dev/null
+++ 
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+We test this by assigning to a bit-field a value
+that is out-of-range of it's signed counterpart.
+I.e., with a bit-field of width 4, assigning
+8 to it would be out-of-range if we treated it
+as a signed. If LLDB were to sign-extend the Scalar
+(which shouldn't happen for unsigned bit-fields)
+it would left-fill the result with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:


Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}
Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+We test this by assigning to a bit-field a value
+that is out-of-range of it's signed counterpart.
+I.e., with a bit-field of width 4, assigning
+8 to it would be out-of-range if we treated it
+as a signed. If LLDB were to sign-extend the Scalar
+(which shouldn't happen for unsigned bit-fields)
+it would left-fill the result with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:
___
lldb-commits mailing list
lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

In D134493#3811097 , @labath wrote:

> In D134493#3810290 , @Michael137 
> wrote:
>
>> Wasn't sure how to properly test this since the original reproducer 
>> technically relies on implementation-defined behaviour (i.e., initialising a 
>> bitfield with an out-of-range value). Suggestions are welcome
>
> I'm probably missing something, but what exactly is undefined about that test 
> program? The number eight fits comfortably in four bits, and afaik it is a 
> valid value for the `EnumVals` type because it has a fixed underlying type.
>
> Other than that, this seems fine to me.

Ah yes, absolutely true! Misinterpreted it from some of the other reproducers.
It's enough for the value we assign (here `8`) to be out of range of the 
bit-field (if it were signed). Since we previously treated
all bit-fields of enum type as signed, that would incorrectly left-fill with 1s.
I.e., if the bit-field were of width 16, then 32768 would sign-extend 
incorrectly. I'll update the test description accordingly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

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

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134493#3810290 , @Michael137 
wrote:

> Wasn't sure how to properly test this since the original reproducer 
> technically relies on implementation-defined behaviour (i.e., initialising a 
> bitfield with an out-of-range value). Suggestions are welcome

I'm probably missing something, but what exactly is undefined about that test 
program? The number eight fits comfortably in four bits, and afaik it is a 
valid value for the `EnumVals` type because it has a fixed underlying type.

Other than that, this seems fine to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

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

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

In D134493#3810991 , @DavidSpickett 
wrote:

> It wouldn't need to be same across platforms either really. Can always 
> `@skipifnotarchwhatever` and as long as it's tested somewhere that's fine. Ok 
> clang could change its mind so we mitigate that by making sure the test would 
> fail if it does, then update it as needed.

Fair enough, I added a test for this. Can skip/fix if compilers ever change 
their mind


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

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

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Michael Buch via Phabricator via lldb-commits]

Michael137 updated this revision to Diff 462427.
Michael137 added a comment.

- Add API test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
  lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp


Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}
Index: 
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
===
--- /dev/null
+++ 
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+This test relies on implementation-defined behaviour
+of initializing a bit-field with an out-of-range value.
+On clang this will do the expected thing of writing
+the full value into the bit-field and the trailing
+padding bits. Then during sign-extension (which
+shouldn't occur for unsigned bit-fields) it would
+left-fill the resulting Scalar with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:


Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}
Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+This test relies on implementation-defined behaviour
+of initializing a bit-field with an out-of-range value.
+On clang this will do the expected thing of writing
+the full value into the bit-field and the trailing
+padding bits. Then during sign-extension (which
+shouldn't occur for unsigned bit-fields) it would
+left-fill the resulting Scalar with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:
___
lldb-commits mailing list

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

It wouldn't need to be same across platforms either really. Can always 
`@skipifnotarchwhatever` and as long as it's tested somewhere that's fine. Ok 
clang could change its mind so we mitigate that by making sure the test would 
fail if it does, then update it as needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

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

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> Wasn't sure how to properly test this since the original reproducer 
> technically relies on implementation-defined behaviour (i.e., initialising a 
> bitfield with an out-of-range value). Suggestions are welcome

Is this undefined behaviour defined at least for clang only across platforms? 
I'm guessing most of the time the test suite programs are built with clang, 
though you can change that with a cmake var. Perhaps you could do something 
like `@skipifnotclang` in that case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

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

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

Wasn't sure how to properly test this since the original reproducer technically 
relies on implementation-defined behaviour (i.e., initialising a bitfield with 
an out-of-range value).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

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

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Michael Buch via Phabricator via lldb-commits]

Michael137 created this revision.
Michael137 added reviewers: aprantl, jingham.
Herald added a project: All.
Michael137 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The motivating issue was the following:

  $ cat main.cpp
  enum class EnumVals : uint16_t {
  VAL1 = 0
  };
  
  struct Foo {
  EnumVals b1 : 8;
  };
  
  int main() {
  Foo f{.b1 = (EnumVals)8};
  
  return 0; // Break here
  }
  
  (lldb) script
  >>> 
lldb.frame.FindVariable("f").GetChildMemberWithName("b1").GetValueAsUnsigned()
  4294967288

In the above example we observe a unsigned integer wrap-around
because we sign-extended the bit-fields underlying Scalar value
before casting it to an unsigned. The sign extension occurs because
we don't mark `APSInt::IsUnsigned == true` correctly when extracting
the value from memory (in Value::ResolveValue).

This patch corrects `GetEncoding` to account for unsigned enum types.
With this change the Scalar would be zero-extended instead.

This is mainly a convenience fix which well-formed code wouldn't
encounter.

rdar://99785324


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134493

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp


Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:


Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits