[lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Michael137]

Summary

The motivating issue was the following:

$ cat main.cpp
enum class EnumVals : uint16_t {
    VAL1 = 0
};

struct Foo {
    EnumVals b1 : 4;
};

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). The reason why sign
extension causes the wraparound is that the value we're assigning
to the bit-field is out-of-range, which causes Scalar::sext to left-fill
the Scalar with 1s.

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

[Michael137]

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

[DavidSpickett]

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.

[DavidSpickett]

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.

[Michael137]

• Add API test

[Michael137]

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

[labath]

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.

[Michael137]

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

[Michael137]

• Fix test description

[Michael137]

• Reword commit message

[labath]

Ok, sounds good then.

[werat]

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 <stdint.h>
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
>>>

[labath]

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.

[werat]

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.

[Michael137]

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

[shafik]

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

#include <iostream>
#include <cstdint>

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

[werat]

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.

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

case clang::Type::Complex: {
     return encoding;
   }
 
   case clang::Type::ObjCInterface:
     break;
   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:
   case clang::Type::Attributed:
   case clang::Type::BTFTagAttributed:
   case clang::Type::TemplateTypeParm:
   case clang::Type::SubstTemplateTypeParm:
   case clang::Type::SubstTemplateTypeParmPack:

lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py

+"""
+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())

lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp

+#include <stdint.h>
+
+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<EnumVals>(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+            //% val = b.GetValueAsUnsigned()
+            //% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}