[lldb][DWARFASTParserClang] Prevent unnamed bitfield creation in the presence of overlapping fields #108343

Michael137 · 2024-09-12T08:19:32Z

This bug surfaced after #105865 (currently reverted, but blocked on this to be relanded).

Because Clang doesn't emit DW_TAG_members for unnamed bitfields, LLDB has to make an educated guess about whether they existed in the source. It does so by checking whether there is a gap between where the last field ended and the currently parsed field starts. In the example test case, the empty field padding was folded into the storage of data. Because the bit_offset of padding is 0x0 and its DW_AT_byte_size is 0x1, LLDB thinks the field ends at 0x1 (not quite because we first round the size to a word size, but this is an implementation detail), erroneously deducing that there's a gap between flag and padding.

This patch adds the notion of "effective field end", which accounts for fields that share storage. It is set to the end of the storage that the two fields occupy. Then we use this to check for gaps in the unnamed bitfield creation logic.

…presence of overlapping fields

llvmbot · 2024-09-12T08:20:05Z

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

This bug surfaced after #105865 (currently reverted, but blocked on this to be relanded).

Because Clang doesn't emit DW_TAG_members for unnamed bitfields, LLDB has to make an educated guess about whether they existed in the source. It does so by checking whether there is a gap between where the last field began and the currently parsed field starts. In the example test case, the empty field padding was folded into the storage of data. Because the bit_offset of padding is 0x0 and its DW_AT_byte_size is 0x1, LLDB thinks the field ends at 0x1 (not quite because we first round the size to a word size, but this is an implementation detail), erroneously deducing that there's a gap between flag and padding.

This patch adds the notion of "effective field end", which accounts for fields that share storage. Its value is the end of the storage that the two fields occupy. Then we use this to check for gaps in the unnamed bitfield creation logic.

Full diff: https://github.com/llvm/llvm-project/pull/108343.diff

3 Files Affected:

(modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+10-4)
(modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h (+31)
(modified) lldb/test/Shell/SymbolFile/DWARF/no_unique_address-with-bitfields.cpp (-6)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 5b9de6f1566b05..88511e036fd8c2 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2932,14 +2932,20 @@ void DWARFASTParserClang::ParseSingleMember(
     last_field_info = this_field_info;
     last_field_info.SetIsBitfield(true);
   } else {
-    last_field_info.bit_offset = field_bit_offset;
+    FieldInfo this_field_info{.is_bitfield = false};
+    this_field_info.bit_offset = field_bit_offset;
 
+    // TODO: we shouldn't silently ignore the bit_size if we fail
+    //       to GetByteSize.
     if (std::optional<uint64_t> clang_type_size =
             member_type->GetByteSize(nullptr)) {
-      last_field_info.bit_size = *clang_type_size * character_width;
+      this_field_info.bit_size = *clang_type_size * character_width;
     }
 
-    last_field_info.SetIsBitfield(false);
+    if (this_field_info.GetFieldEnd() < last_field_info.GetFieldEnd())
+      this_field_info.SetEffectiveFieldEnd(last_field_info.GetFieldEnd());
+
+    last_field_info = this_field_info;
   }
 
   // Don't turn artificial members such as vtable pointers into real FieldDecls
@@ -3738,7 +3744,7 @@ void DWARFASTParserClang::AddUnnamedBitfieldToRecordTypeIfNeeded(
     const FieldInfo &current_field) {
   // TODO: get this value from target
   const uint64_t word_width = 32;
-  uint64_t last_field_end = previous_field.bit_offset + previous_field.bit_size;
+  uint64_t last_field_end = previous_field.GetEffectiveFieldEnd();
 
   if (!previous_field.IsBitfield()) {
     // The last field was not a bit-field...
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
index 3809ee912cec6f..4ff464ce26c548 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -258,9 +258,27 @@ class DWARFASTParserClang : public lldb_private::plugin::dwarf::DWARFASTParser {
 
 private:
   struct FieldInfo {
+    ///< Size in bits that this field occopies. Can but
+    ///< need not be the DW_AT_bit_size of the field.
     uint64_t bit_size = 0;
+
+    ///< Offset of this field in bits from the beginning
+    ///< of the containing struct. Can but need not
+    ///< be the DW_AT_data_bit_offset of the field.
     uint64_t bit_offset = 0;
+
+    ///< In case this field is folded into the storage
+    ///< of a previous member's storage (for example
+    ///< with [[no_unique_address]]), the effective field
+    ///< end is the offset in bits from the beginning of
+    ///< the containing struct where the field we were
+    ///< folded into ended.
+    std::optional<uint64_t> effective_field_end;
+
+    ///< Set to 'true' if this field is a bit-field.
     bool is_bitfield = false;
+
+    ///< Set to 'true' if this field is DW_AT_artificial.
     bool is_artificial = false;
 
     FieldInfo() = default;
@@ -276,6 +294,19 @@ class DWARFASTParserClang : public lldb_private::plugin::dwarf::DWARFASTParser {
       // bit offset than any previous bitfield + size.
       return (bit_size + bit_offset) <= next_bit_offset;
     }
+
+    /// Returns the offset in bits of where the storage this field
+    /// occupies ends.
+    uint64_t GetFieldEnd() const { return bit_size + bit_offset; }
+
+    void SetEffectiveFieldEnd(uint64_t val) { effective_field_end = val; }
+
+    /// If this field was folded into storage of a previous field,
+    /// returns the offset in bits of where that storage ends. Otherwise,
+    /// returns the regular field end (see \ref GetFieldEnd).
+    uint64_t GetEffectiveFieldEnd() const {
+      return effective_field_end.value_or(GetFieldEnd());
+    }
   };
 
   /// Parsed form of all attributes that are relevant for parsing type members.
diff --git a/lldb/test/Shell/SymbolFile/DWARF/no_unique_address-with-bitfields.cpp b/lldb/test/Shell/SymbolFile/DWARF/no_unique_address-with-bitfields.cpp
index 1c9cc36a711b47..b1672a384c9fe4 100644
--- a/lldb/test/Shell/SymbolFile/DWARF/no_unique_address-with-bitfields.cpp
+++ b/lldb/test/Shell/SymbolFile/DWARF/no_unique_address-with-bitfields.cpp
@@ -1,7 +1,3 @@
-// LLDB currently erroneously adds an unnamed bitfield
-// into the AST when an overlapping no_unique_address
-// field precedes a bitfield.
-
 // RUN: %clang --target=x86_64-apple-macosx -c -gdwarf -o %t %s
 // RUN: %lldb %t \
 // RUN:   -o "target var global" \
@@ -12,8 +8,6 @@
 // CHECK:      CXXRecordDecl {{.*}} struct Foo definition
 // CHECK:      |-FieldDecl {{.*}} data 'char[5]'
 // CHECK-NEXT: |-FieldDecl {{.*}} padding 'Empty'
-// CHECK-NEXT: |-FieldDecl {{.*}} 'int'
-// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 8
 // CHECK-NEXT: `-FieldDecl {{.*}} sloc> flag 'unsigned long'
 // CHECK-NEXT:   `-IntegerLiteral {{.*}} 'int' 1

Michael137 · 2024-09-12T08:20:32Z

This logic is getting very finicky, so I wouldn't be surprised if there are cases not covered by this. Will have a think

github-actions · 2024-09-12T13:18:34Z

✅ With the latest revision this PR passed the C/C++ code formatter.

adrian-prantl · 2024-09-12T15:41:02Z

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h

@@ -258,9 +258,27 @@ class DWARFASTParserClang : public lldb_private::plugin::dwarf::DWARFASTParser {

 private:
  struct FieldInfo {
+    ///< Size in bits that this field occupies. Can but


/// comment decl d;

or

decl d; ///<comment

adrian-prantl

Once we're in heuristic territory I think we can only get a hold of this problem with lots of tests.

Michael137 · 2024-09-12T15:47:06Z

Once we're in heuristic territory I think we can only get a hold of this problem with lots of tests.

Added more test in the latest iteration (the ones marked FIXME were still misbehaving on top-of-tree, although in other ways...but we still asserted in the expression evaluator, so don't think we really regress with this patch).

There are some cases where it's pretty much impossible to differentiate between tail padding and unnamed bitfields. So as long as we don't emit those bitfields into DWARF I think we have to resort to this.

adrian-prantl · 2024-09-12T16:08:40Z

Why don't we just make the unnamed fields explicit in DWARF and solve the problem the correct way?
(We'll still needs this hack to support other/older compilers though)

pranavk · 2024-09-12T17:20:25Z

Hmm, this seems to be breaking LLVM build:

error: initialization of non-aggregate type 'FieldInfo' with a designated initializer list
 |     FieldInfo this_field_info{.is_bitfield = false};

kazutakahirata · 2024-09-12T17:28:21Z

Hmm, this seems to be breaking LLVM build:

error: initialization of non-aggregate type 'FieldInfo' with a designated initializer list
 |     FieldInfo this_field_info{.is_bitfield = false};

FWIW, with clang-16.0.6 as the host compiler, I get:

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2935:31: error: designated initializers are a C++20 extension [-Werror,-Wc++20-designator]
    FieldInfo this_field_info{.is_bitfield = false};
                              ^
1 error generated.

kazutakahirata · 2024-09-12T17:44:08Z

I've fixed the warning with 5d17293.

Michael137 · 2024-09-12T18:04:28Z

I've fixed the warning with 5d17293.

Thanks!

…ng layout (#108375) Depends on #108362 and #108343. Adds new layout for #105865.

…ng layout (llvm#108375) Depends on llvm#108362 and llvm#108343. Adds new layout for llvm#105865.

[lldb][DWARFASTParserClang] Prevent unnamed bitfield creation in the …

51bca43

…presence of overlapping fields

Michael137 requested review from adrian-prantl, labath and mysterymath September 12, 2024 08:19

Michael137 requested a review from JDevlieghere as a code owner September 12, 2024 08:19

llvmbot added the lldb label Sep 12, 2024

Michael137 mentioned this pull request Sep 12, 2024

[libc++][string] Remove potential non-trailing 0-length array #105865

Merged

fixup! fix comment typo

bae2a96

Michael137 mentioned this pull request Sep 12, 2024

[lldb][test] TestDataFormatterLibcxxStringSimulator.py: add new padding layout #108375

Merged

Michael137 added 2 commits September 12, 2024 14:12

fixup! fix case with consecutive overlapping fields

5d25eeb

fixup! clang-format

567fcce

adrian-prantl reviewed Sep 12, 2024

View reviewed changes

adrian-prantl approved these changes Sep 12, 2024

View reviewed changes

Michael137 added 2 commits September 12, 2024 16:44

fixup! add more test-cases

8416d05

fixup! fix doxygen comments

584a2d8

Michael137 merged commit a6a547f into llvm:main Sep 12, 2024
7 checks passed

Michael137 added a commit that referenced this pull request Oct 3, 2024

[lldb][test] TestDataFormatterLibcxxStringSimulator.py: add new paddi…

d5f6e88

…ng layout (#108375) Depends on #108362 and #108343. Adds new layout for #105865.

xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024

[lldb][test] TestDataFormatterLibcxxStringSimulator.py: add new paddi…

b5bde9a

…ng layout (llvm#108375) Depends on llvm#108362 and llvm#108343. Adds new layout for llvm#105865.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lldb][DWARFASTParserClang] Prevent unnamed bitfield creation in the presence of overlapping fields #108343

[lldb][DWARFASTParserClang] Prevent unnamed bitfield creation in the presence of overlapping fields #108343

Michael137 commented Sep 12, 2024 •

edited

Loading

llvmbot commented Sep 12, 2024

Michael137 commented Sep 12, 2024

github-actions bot commented Sep 12, 2024 •

edited

Loading

adrian-prantl Sep 12, 2024

adrian-prantl left a comment

Michael137 commented Sep 12, 2024

adrian-prantl commented Sep 12, 2024

pranavk commented Sep 12, 2024

kazutakahirata commented Sep 12, 2024

kazutakahirata commented Sep 12, 2024

Michael137 commented Sep 12, 2024

[lldb][DWARFASTParserClang] Prevent unnamed bitfield creation in the presence of overlapping fields #108343

[lldb][DWARFASTParserClang] Prevent unnamed bitfield creation in the presence of overlapping fields #108343

Conversation

Michael137 commented Sep 12, 2024 • edited Loading

llvmbot commented Sep 12, 2024

Michael137 commented Sep 12, 2024

github-actions bot commented Sep 12, 2024 • edited Loading

adrian-prantl Sep 12, 2024

Choose a reason for hiding this comment

adrian-prantl left a comment

Choose a reason for hiding this comment

Michael137 commented Sep 12, 2024

adrian-prantl commented Sep 12, 2024

pranavk commented Sep 12, 2024

kazutakahirata commented Sep 12, 2024

kazutakahirata commented Sep 12, 2024

Michael137 commented Sep 12, 2024

Michael137 commented Sep 12, 2024 •

edited

Loading

github-actions bot commented Sep 12, 2024 •

edited

Loading