From a6a547f18d99f0b0bf5ffac55443d687200f972d Mon Sep 17 00:00:00 2001 From: Michael Buch Date: Thu, 12 Sep 2024 17:26:17 +0100 Subject: [PATCH] [lldb][DWARFASTParserClang] Prevent unnamed bitfield creation in the presence of overlapping fields (#108343) This bug surfaced after https://github.com/llvm/llvm-project/pull/105865 (currently reverted, but blocked on this to be relanded). Because Clang doesn't emit `DW_TAG_member`s 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. --- .../SymbolFile/DWARF/DWARFASTParserClang.cpp | 15 ++- .../SymbolFile/DWARF/DWARFASTParserClang.h | 31 ++++++ .../no_unique_address-with-bitfields.cpp | 96 +++++++++++++++++-- 3 files changed, 131 insertions(+), 11 deletions(-) diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp index 5b9de6f1566b05..5b679bd5a613e3 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp @@ -2932,14 +2932,21 @@ 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 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.GetEffectiveFieldEnd()) + this_field_info.SetEffectiveFieldEnd( + last_field_info.GetEffectiveFieldEnd()); + + last_field_info = this_field_info; } // Don't turn artificial members such as vtable pointers into real FieldDecls @@ -3738,7 +3745,7 @@ void DWARFASTParserClang::AddUnnamedBitfieldToRecordTypeIfNeeded( const FieldInfo ¤t_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..1ffb09b2fc1942 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 occupies. 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 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..980180e7be9aef 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,10 +1,10 @@ -// 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" \ +// RUN: -o "target var global2" \ +// RUN: -o "target var global3" \ +// RUN: -o "target var global4" \ +// RUN: -o "target var global5" \ // RUN: -o "image dump ast" \ // RUN: -o exit | FileCheck %s @@ -12,12 +12,12 @@ // 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: `-FieldDecl {{.*}} flag 'unsigned long' // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1 struct Empty {}; +struct Empty2 {}; +struct Empty3 {}; struct Foo { char data[5]; @@ -26,3 +26,85 @@ struct Foo { }; Foo global; + +// CHECK: CXXRecordDecl {{.*}} struct ConsecutiveOverlap definition +// CHECK: |-FieldDecl {{.*}} data 'char[5]' +// CHECK-NEXT: |-FieldDecl {{.*}} p1 'Empty' +// CHECK-NEXT: |-FieldDecl {{.*}} p2 'Empty2' +// CHECK-NEXT: |-FieldDecl {{.*}} p3 'Empty3' +// CHECK-NEXT: `-FieldDecl {{.*}} flag 'unsigned long' +// CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1 + +struct ConsecutiveOverlap { + char data[5]; + [[no_unique_address]] Empty p1; + [[no_unique_address]] Empty2 p2; + [[no_unique_address]] Empty3 p3; + unsigned long flag : 1; +}; + +ConsecutiveOverlap global2; + +// FIXME: we fail to deduce the unnamed bitfields here. +// +// CHECK: CXXRecordDecl {{.*}} struct MultipleAtOffsetZero definition +// CHECK: |-FieldDecl {{.*}} data 'char[5]' +// CHECK-NEXT: |-FieldDecl {{.*}} p1 'Empty' +// CHECK-NEXT: |-FieldDecl {{.*}} f1 'unsigned long' +// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 1 +// CHECK-NEXT: |-FieldDecl {{.*}} p2 'Empty2' +// CHECK-NEXT: `-FieldDecl {{.*}} f2 'unsigned long' +// CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1 + +struct MultipleAtOffsetZero { + char data[5]; + [[no_unique_address]] Empty p1; + int : 4; + unsigned long f1 : 1; + [[no_unique_address]] Empty2 p2; + int : 4; + unsigned long f2 : 1; +}; + +MultipleAtOffsetZero global3; + +// FIXME: we fail to deduce the unnamed bitfields here. +// +// CHECK: CXXRecordDecl {{.*}} struct MultipleEmpty definition +// CHECK: |-FieldDecl {{.*}} data 'char[5]' +// CHECK-NEXT: |-FieldDecl {{.*}} p1 'Empty' +// CHECK-NEXT: |-FieldDecl {{.*}} f1 'unsigned long' +// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 1 +// CHECK-NEXT: |-FieldDecl {{.*}} p2 'Empty' +// CHECK-NEXT: `-FieldDecl {{.*}} f2 'unsigned long' +// CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1 + +struct MultipleEmpty { + char data[5]; + [[no_unique_address]] Empty p1; + int : 4; + unsigned long f1 : 1; + [[no_unique_address]] Empty p2; + int : 4; + unsigned long f2 : 1; +}; + +MultipleEmpty global4; + +// CHECK: CXXRecordDecl {{.*}} struct FieldBitfieldOverlap definition +// CHECK: |-FieldDecl {{.*}} a 'int' +// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 3 +// CHECK-NEXT: |-FieldDecl {{.*}} p1 'Empty' +// CHECK-NEXT: |-FieldDecl {{.*}} b 'int' +// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 6 +// CHECK-NEXT: `-FieldDecl {{.*}} c 'int' +// CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1 + +struct FieldBitfieldOverlap { + int a : 3; + [[no_unique_address]] Empty p1; + int b : 6; + int c : 1; +}; + +FieldBitfieldOverlap global5;