[lldb] Fix bitfield "frame var" for pointers (pr47743)

[labath]

Summary

Displaying large packed bitfields did not work if one was accessing them
through a pointer, and he used the "->" notation ("[0]." notation is
fine). The reason for that is that implicit dereference in -> is plumbed
all the way down to ValueObjectChild::UpdateValue, where the process of
fetching the child value was forked for this flag. The bitfield
"sliding" code was implemented only for the branch which did not require
dereferencing.

This patch restructures the function to avoid this mistake. Processing
now happens in two stages.

• first the parent is dereferenced (if needed)
• then the child value is computed (this step includes sliding and is common for both branches)

[cameron314]

This fixes https://bugs.llvm.org/show_bug.cgi?id=47743 for me.

[mamai]

LGTM

[fpichet]

What is the status of this patch?
Can it be committed?
It helps for our out of tree fork of lldb.

[labath]

@shafik, @teemperor, any thoughts on this?

[teemperor]

LGTM

lldb/source/Core/ValueObjectChild.cpp

bool ValueObjectChild::UpdateValue() {
   ValueObject *parent = m_parent;
   if (parent) {
     if (parent->UpdateValueIfNeeded(false)) {
       m_value.SetCompilerType(GetCompilerType());
 
       CompilerType parent_type(parent->GetCompilerType());
       // Copy the parent scalar value and the scalar value type
       m_value.GetScalar() = parent->GetValue().GetScalar();
-      Value::ValueType value_type = parent->GetValue().GetValueType();
-      m_value.SetValueType(value_type);
+      m_value.SetValueType(parent->GetValue().GetValueType());
 
       Flags parent_type_flags(parent_type.GetTypeInfo());
       const bool is_instance_ptr_base =
           ((m_is_base_class) &&
            (parent_type_flags.AnySet(lldb::eTypeInstanceIsPointer)));
 
       if (parent->GetCompilerType().ShouldTreatScalarValueAsAddress()) {
-        lldb::addr_t addr = parent->GetPointerValue();
-        m_value.GetScalar() = addr;
+        m_value.GetScalar() = parent->GetPointerValue();
 
-        if (addr == LLDB_INVALID_ADDRESS) {
-          m_error.SetErrorString("parent address is invalid.");
-        } else if (addr == 0) {
-          m_error.SetErrorString("parent is NULL");
-        } else {
-          m_value.GetScalar() += m_byte_offset;
-          AddressType addr_type = parent->GetAddressTypeOfChildren();
-
-          switch (addr_type) {
-          case eAddressTypeFile: {
-            lldb::ProcessSP process_sp(GetProcessSP());
-            if (process_sp && process_sp->IsAlive())
-              m_value.SetValueType(Value::eValueTypeLoadAddress);
-            else
-              m_value.SetValueType(Value::eValueTypeFileAddress);
-          } break;
-          case eAddressTypeLoad:
-            m_value.SetValueType(is_instance_ptr_base
-                                     ? Value::eValueTypeScalar
-                                     : Value::eValueTypeLoadAddress);
-            break;
-          case eAddressTypeHost:
-            m_value.SetValueType(Value::eValueTypeHostAddress);
-            break;
-          case eAddressTypeInvalid:
-            // TODO: does this make sense?
-            m_value.SetValueType(Value::eValueTypeScalar);
-            break;
-          }
-        }
-      } else {
-        switch (value_type) {
-        case Value::eValueTypeLoadAddress:
-        case Value::eValueTypeFileAddress:
-        case Value::eValueTypeHostAddress: {
-          lldb::addr_t addr =
-              m_value.GetScalar().ULongLong(LLDB_INVALID_ADDRESS);
-          if (addr == LLDB_INVALID_ADDRESS) {
-            m_error.SetErrorString("parent address is invalid.");
-          } else if (addr == 0) {
-            m_error.SetErrorString("parent is NULL");
-          } else {
-            // If a bitfield doesn't fit into the child_byte_size'd
-            // window at child_byte_offset, move the window forward
-            // until it fits.  The problem here is that Value has no
-            // notion of bitfields and thus the Value's DataExtractor
-            // is sized like the bitfields CompilerType; a sequence of
-            // bitfields, however, can be larger than their underlying
-            // type.
-            if (m_bitfield_bit_offset) {
-              const bool thread_and_frame_only_if_stopped = true;
-              ExecutionContext exe_ctx(GetExecutionContextRef().Lock(
-                  thread_and_frame_only_if_stopped));
-              if (auto type_bit_size = GetCompilerType().GetBitSize(
-                      exe_ctx.GetBestExecutionContextScope())) {
-                uint64_t bitfield_end =
-                    m_bitfield_bit_size + m_bitfield_bit_offset;
-                if (bitfield_end > *type_bit_size) {
-                  uint64_t overhang_bytes =
-                      (bitfield_end - *type_bit_size + 7) / 8;
-                  m_byte_offset += overhang_bytes;
-                  m_bitfield_bit_offset -= overhang_bytes * 8;
-                }
-              }
-            }
+        switch (parent->GetAddressTypeOfChildren()) {
+      switch (m_value.GetValueType()) {
+        lldb::addr_t addr = m_value.GetScalar().ULongLong(LLDB_INVALID_ADDRESS);
+          // If a bitfield doesn't fit into the child_byte_size'd window at
+          // child_byte_offset, move the window forward until it fits.  The
+          // problem here is that Value has no notion of bitfields and thus the
+          // Value's DataExtractor is sized like the bitfields CompilerType; a
+          // sequence of bitfields, however, can be larger than their underlying
 
-            // Set this object's scalar value to the address of its value by
-            // adding its byte offset to the parent address
-            m_value.GetScalar() += m_byte_offset;
-          }
-        } break;
 
-        case Value::eValueTypeScalar:
-          // try to extract the child value from the parent's scalar value
-          {
-            Scalar scalar(m_value.GetScalar());
-            scalar.ExtractBitfield(8 * m_byte_size, 8 * m_byte_offset);
-            m_value.GetScalar() = scalar;
-          }
-          break;
-        default:
-          m_error.SetErrorString("parent has invalid value.");
-          break;
-        }
-      }
 
       if (m_error.Success()) {
         const bool thread_and_frame_only_if_stopped = true;
         ExecutionContext exe_ctx(
             GetExecutionContextRef().Lock(thread_and_frame_only_if_stopped));
         if (GetCompilerType().GetTypeInfo() & lldb::eTypeHasValue) {
           Value &value = is_instance_ptr_base ? m_parent->GetValue() : m_value;
           m_error =

lldb/test/API/lang/c/bitfields/TestBitfields.py

def test_and_run_command(self):
                 '(uint8_t:1) b15 = \'\\0\'',
                 '(uint8_t:1) b16 = \'\\0\'',
                 '(uint8_t:1) b17 = \'\\0\'',
                 ])
 
         self.expect("v/x large_packed", VARIABLES_DISPLAYED_CORRECTLY,
                     substrs=["a = 0x0000000cbbbbaaaa", "b = 0x0000000dffffeee"])
 
+        # Check reading a bitfield through a pointer in various ways (PR47743)
+        self.expect("v/x large_packed_ptr->b",
+                substrs=["large_packed_ptr->b = 0x0000000dffffeeee"])
+        self.expect("v/x large_packed_ptr[0].b",
+                substrs=["large_packed_ptr[0].b = 0x0000000dffffeeee"])
+
     # BitFields exhibit crashes in record layout on Windows
     # (http://llvm.org/pr21800)
     @skipIfWindows
     def test_expression_bug(self):
         # Ensure evaluating (emulating) an expression does not break bitfield
         # values for already parsed variables. The expression is run twice
         # because the very first expression can resume a target (to allocate
         # memory, etc.) even if it is not being jitted.

lldb/test/API/lang/c/bitfields/main.c

#pragma pack()
 
     struct LargePackedBits {
         uint64_t a: 36;
         uint64_t b: 36;
     } __attribute__((packed));
 
     struct LargePackedBits large_packed =
       (struct LargePackedBits){ 0xcbbbbaaaa, 0xdffffeeee };
+    struct LargePackedBits *large_packed_ptr = &large_packed;
     
     return 0;               //// Set break point at this line.
 
 }