[Lldb-commits] [lldb] f74aaca - [lldb][DWARFASTParserClang] Check DW_AT_declaration to determine static data members (#68300)
via lldb-commits
lldb-commits at lists.llvm.org
Fri Oct 6 01:07:23 PDT 2023
Author: Michael Buch
Date: 2023-10-06T09:07:20+01:00
New Revision: f74aaca63202cabb512c78fe19196ff348d436a8
URL: https://github.com/llvm/llvm-project/commit/f74aaca63202cabb512c78fe19196ff348d436a8
DIFF: https://github.com/llvm/llvm-project/commit/f74aaca63202cabb512c78fe19196ff348d436a8.diff
LOG: [lldb][DWARFASTParserClang] Check DW_AT_declaration to determine static data members (#68300)
**Background**
Prior to DWARFv4, there was no clear normative text on how to handle
static data members. Non-normative text suggested that compilers should
use `DW_AT_external` to mark static data members of structrues/unions.
Clang does this consistently. However, GCC doesn't, e.g., when the
structure/union is in an anonymous namespace (which is C++ standard
conformant). Additionally, GCC never emits `DW_AT_data_member_location`s
for union members (regardless of storage linkage and storage duration).
Since DWARFv5 (issue 161118.1), static data members get emitted as
`DW_TAG_variable`.
LLDB used to differentiate between static and non-static members by
checking the `DW_AT_external` flag and the absence of
`DW_AT_data_member_location`. With
[D18008](https://reviews.llvm.org/D18008) LLDB started to pretend that
union members always have a `0` `DW_AT_data_member_location` by default
(because GCC never emits these locations).
In [D124409](https://reviews.llvm.org/D124409) LLDB stopped checking the
`DW_AT_external` flag to account for the case where GCC doesn't emit the
flag for types in anonymous namespaces; instead we only check for
presence of `DW_AT_data_member_location`s.
The combination of these changes then meant that LLDB would never
correctly detect that a union has static data members.
**Solution**
Instead of unconditionally initializing the `member_byte_offset` to `0`
specifically for union members, this patch proposes to check for both
the absence of `DW_AT_data_member_location` and `DW_AT_declaration`,
which consistently gets emitted for static data members on GCC and
Clang.
We initialize the `member_byte_offset` to `0` anyway if we determine it
wasn't a static. So removing the special case for unions makes this code
simpler to reason about.
Long-term, we should just use DWARFv5's new representation for static
data members.
Fixes #68135
Added:
lldb/test/API/lang/cpp/union-static-data-members/Makefile
lldb/test/API/lang/cpp/union-static-data-members/TestCppUnionStaticMembers.py
lldb/test/API/lang/cpp/union-static-data-members/main.cpp
Modified:
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
Removed:
################################################################################
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 005711d6f488c7f..6e13626d2894313 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2492,8 +2492,9 @@ struct MemberAttributes {
DWARFFormValue encoding_form;
/// Indicates the byte offset of the word from the base address of the
/// structure.
- uint32_t member_byte_offset;
+ uint32_t member_byte_offset = UINT32_MAX;
bool is_artificial = false;
+ bool is_declaration = false;
};
/// Parsed form of all attributes that are relevant for parsing Objective-C
@@ -2627,8 +2628,6 @@ DiscriminantValue &VariantPart::discriminant() { return this->_discriminant; }
MemberAttributes::MemberAttributes(const DWARFDIE &die,
const DWARFDIE &parent_die,
ModuleSP module_sp) {
- member_byte_offset = (parent_die.Tag() == DW_TAG_union_type) ? 0 : UINT32_MAX;
-
DWARFAttributes attributes = die.GetAttributes();
for (size_t i = 0; i < attributes.Size(); ++i) {
const dw_attr_t attr = attributes.AttributeAtIndex(i);
@@ -2669,6 +2668,9 @@ MemberAttributes::MemberAttributes(const DWARFDIE &die,
case DW_AT_artificial:
is_artificial = form_value.Boolean();
break;
+ case DW_AT_declaration:
+ is_declaration = form_value.Boolean();
+ break;
default:
break;
}
@@ -2875,10 +2877,18 @@ void DWARFASTParserClang::ParseSingleMember(
if (class_is_objc_object_or_interface)
attrs.accessibility = eAccessNone;
- // Handle static members, which is any member that doesn't have a bit or a
- // byte member offset.
+ // Handle static members, which are typically members without
+ // locations. However, GCC *never* emits DW_AT_data_member_location
+ // for static data members of unions.
+ // Non-normative text pre-DWARFv5 recommends marking static
+ // data members with an DW_AT_external flag. Clang emits this consistently
+ // whereas GCC emits it only for static data members if not part of an
+ // anonymous namespace. The flag that is consistently emitted for static
+ // data members is DW_AT_declaration, so we check it instead.
+ // FIXME: Since DWARFv5, static data members are marked DW_AT_variable so we
+ // can consistently detect them on both GCC and Clang without below heuristic.
if (attrs.member_byte_offset == UINT32_MAX &&
- attrs.data_bit_offset == UINT64_MAX) {
+ attrs.data_bit_offset == UINT64_MAX && attrs.is_declaration) {
Type *var_type = die.ResolveTypeUID(attrs.encoding_form.Reference());
if (var_type) {
diff --git a/lldb/test/API/lang/cpp/union-static-data-members/Makefile b/lldb/test/API/lang/cpp/union-static-data-members/Makefile
new file mode 100644
index 000000000000000..99998b20bcb0502
--- /dev/null
+++ b/lldb/test/API/lang/cpp/union-static-data-members/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/lang/cpp/union-static-data-members/TestCppUnionStaticMembers.py b/lldb/test/API/lang/cpp/union-static-data-members/TestCppUnionStaticMembers.py
new file mode 100644
index 000000000000000..47166636b12647c
--- /dev/null
+++ b/lldb/test/API/lang/cpp/union-static-data-members/TestCppUnionStaticMembers.py
@@ -0,0 +1,43 @@
+"""
+Tests that frame variable and expr work for
+C++ unions and their static data members.
+"""
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+import lldbsuite.test.lldbutil as lldbutil
+
+class CppUnionStaticMembersTestCase(TestBase):
+ def test(self):
+ """Tests that frame variable and expr work
+ for union static data members"""
+ self.build()
+
+ (target, process, main_thread, _) = lldbutil.run_to_source_breakpoint(
+ self, "return 0", lldb.SBFileSpec("main.cpp")
+ )
+
+ self.expect("frame variable foo", substrs=["val = 42"])
+ self.expect("frame variable bar", substrs=["val = 137"])
+
+ self.expect_expr("foo", result_type="Foo", result_children=[ValueCheck(
+ name="val", value="42"
+ )])
+ self.expect_expr("bar", result_type="Bar", result_children=[ValueCheck(
+ name="val", value="137"
+ )])
+
+ self.expect_expr("Foo::sVal1", result_type="const int", result_value="-42")
+ self.expect_expr("Foo::sVal2", result_type="Foo", result_children=[ValueCheck(
+ name="val", value="42"
+ )])
+
+ @expectedFailureAll
+ def test_union_in_anon_namespace(self):
+ """Tests that frame variable and expr work
+ for union static data members in anonymous
+ namespaces"""
+ self.expect_expr("Bar::sVal1", result_type="const int", result_value="-137")
+ self.expect_expr("Bar::sVal2", result_type="Bar", result_children=[ValueCheck(
+ name="val", value="137"
+ )])
diff --git a/lldb/test/API/lang/cpp/union-static-data-members/main.cpp b/lldb/test/API/lang/cpp/union-static-data-members/main.cpp
new file mode 100644
index 000000000000000..8ba0312cd3a618b
--- /dev/null
+++ b/lldb/test/API/lang/cpp/union-static-data-members/main.cpp
@@ -0,0 +1,25 @@
+union Foo {
+ int val = 42;
+ static const int sVal1 = -42;
+ static Foo sVal2;
+};
+
+Foo Foo::sVal2{};
+
+namespace {
+union Bar {
+ int val = 137;
+ static const int sVal1 = -137;
+ static Bar sVal2;
+};
+
+Bar Bar::sVal2{};
+} // namespace
+
+int main() {
+ Foo foo;
+ Bar bar;
+ auto sum = Bar::sVal1 + Foo::sVal1 + Foo::sVal2.val + Bar::sVal2.val;
+
+ return 0;
+}
More information about the lldb-commits
mailing list