[Lldb-commits] [lldb] 3c30f22 - [lldb][DWARFASTParserClang] Don't create unnamed bitfields to account for vtable pointer
Michael Buch via lldb-commits
lldb-commits at lists.llvm.org
Tue May 16 03:18:21 PDT 2023
Author: Michael Buch
Date: 2023-05-16T11:18:09+01:00
New Revision: 3c30f224005e3749c89e00592bd2a43aba2aaf75
URL: https://github.com/llvm/llvm-project/commit/3c30f224005e3749c89e00592bd2a43aba2aaf75
DIFF: https://github.com/llvm/llvm-project/commit/3c30f224005e3749c89e00592bd2a43aba2aaf75.diff
LOG: [lldb][DWARFASTParserClang] Don't create unnamed bitfields to account for vtable pointer
**Summary**
When filling out the LayoutInfo for a structure with the offsets
from DWARF, LLDB fills gaps in the layout by creating unnamed
bitfields and adding them to the AST. If we don't do this correctly
and our layout has overlapping fields, we will hat an assertion
in `clang::CGRecordLowering::lower()`. Specifically, if we have
a derived class with a VTable and a bitfield immediately following
the vtable pointer, we create a layout with overlapping fields.
This is an oversight in some of the previous cleanups done around this
area.
In `D76808`, we prevented LLDB from creating unnamed bitfields if there
was a gap between the last field of a base class and the start of a bitfield
in the derived class.
In `D112697`, we started accounting for the vtable pointer. The intention
there was to make sure the offset bookkeeping accounted for the
existence of a vtable pointer (but we didn't actually want to create
any AST nodes for it). Now that `last_field_info.bit_size` was being
set even for artifical fields, the previous fix `D76808` broke
specifically for cases where the bitfield was the first member of a
derived class with a vtable (this scenario wasn't tested so we didn't
notice it). I.e., we started creating redundant unnamed bitfields for
where the vtable pointer usually sits. This confused the lowering logic
in clang.
This patch adds a condition to `ShouldCreateUnnamedBitfield` which
checks whether the first field in the derived class is a vtable ptr.
**Testing**
* Added API test case
Differential Revision: https://reviews.llvm.org/D150591
Added:
Modified:
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
lldb/test/API/lang/cpp/bitfields/main.cpp
Removed:
################################################################################
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index db3213aebdfb6..922f50d33581e 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2934,8 +2934,10 @@ void DWARFASTParserClang::ParseSingleMember(
// artificial member with (unnamed bitfield) padding.
// FIXME: This check should verify that this is indeed an artificial member
// we are supposed to ignore.
- if (attrs.is_artificial)
+ if (attrs.is_artificial) {
+ last_field_info.SetIsArtificial(true);
return;
+ }
if (!member_clang_type.IsCompleteType())
member_clang_type.GetCompleteType();
@@ -3699,17 +3701,23 @@ bool DWARFASTParserClang::ShouldCreateUnnamedBitfield(
return false;
// If we have a base class, we assume there is no unnamed
- // bit-field if this is the first field since the gap can be
- // attributed to the members from the base class. This assumption
- // is not correct if the first field of the derived class is
- // indeed an unnamed bit-field. We currently do not have the
- // machinary to track the offset of the last field of classes we
- // have seen before, so we are not handling this case.
+ // bit-field if either of the following is true:
+ // (a) this is the first field since the gap can be
+ // attributed to the members from the base class.
+ // FIXME: This assumption is not correct if the first field of
+ // the derived class is indeed an unnamed bit-field. We currently
+ // do not have the machinary to track the offset of the last field
+ // of classes we have seen before, so we are not handling this case.
+ // (b) Or, the first member of the derived class was a vtable pointer.
+ // In this case we don't want to create an unnamed bitfield either
+ // since those will be inserted by clang later.
const bool have_base = layout_info.base_offsets.size() != 0;
const bool this_is_first_field =
last_field_info.bit_offset == 0 && last_field_info.bit_size == 0;
+ const bool first_field_is_vptr =
+ last_field_info.bit_offset == 0 && last_field_info.IsArtificial();
- if (have_base && this_is_first_field)
+ if (have_base && (this_is_first_field || first_field_is_vptr))
return false;
return true;
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
index 9edf718ba9215..042075b7df5f0 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -223,12 +223,16 @@ class DWARFASTParserClang : public DWARFASTParser {
uint64_t bit_size = 0;
uint64_t bit_offset = 0;
bool is_bitfield = false;
+ bool is_artificial = false;
FieldInfo() = default;
void SetIsBitfield(bool flag) { is_bitfield = flag; }
bool IsBitfield() { return is_bitfield; }
+ void SetIsArtificial(bool flag) { is_artificial = flag; }
+ bool IsArtificial() const { return is_artificial; }
+
bool NextBitfieldOffsetIsValid(const uint64_t next_bit_offset) const {
// Any subsequent bitfields must not overlap and must be at a higher
// bit offset than any previous bitfield + size.
diff --git a/lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py b/lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
index f38d0f6c7edf4..b85f23eb4bd0a 100644
--- a/lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
+++ b/lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
@@ -168,3 +168,14 @@ def test_bitfield_behind_vtable_ptr(self):
result_children=with_vtable_and_unnamed_children)
self.expect_var_path("with_vtable_and_unnamed",
children=with_vtable_and_unnamed_children)
+
+ derived_with_vtable_children = [
+ ValueCheck(name="Base", children=[
+ ValueCheck(name="b_a", value="2", type="uint32_t")
+ ]),
+ ValueCheck(name="a", value="1", type="unsigned int:1")
+ ]
+ self.expect_expr("derived_with_vtable",
+ result_children=derived_with_vtable_children)
+ self.expect_var_path("derived_with_vtable",
+ children=derived_with_vtable_children)
diff --git a/lldb/test/API/lang/cpp/bitfields/main.cpp b/lldb/test/API/lang/cpp/bitfields/main.cpp
index eb9db7271aae6..6e1d4ba63bf6d 100644
--- a/lldb/test/API/lang/cpp/bitfields/main.cpp
+++ b/lldb/test/API/lang/cpp/bitfields/main.cpp
@@ -113,6 +113,12 @@ struct HasBaseWithVTable : BaseWithVTable {
};
HasBaseWithVTable base_with_vtable;
+struct DerivedWithVTable : public Base {
+ virtual ~DerivedWithVTable() {}
+ unsigned a : 1;
+};
+DerivedWithVTable derived_with_vtable;
+
int main(int argc, char const *argv[]) {
lba.a = 2;
@@ -153,5 +159,8 @@ int main(int argc, char const *argv[]) {
base_with_vtable.b = 0;
base_with_vtable.c = 5;
+ derived_with_vtable.b_a = 2;
+ derived_with_vtable.a = 1;
+
return 0; // break here
}
More information about the lldb-commits
mailing list