Skip to content

[lldb] Index static const members of classes, structs and unions as global variables in DWARF 4 and earlier #111859

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 7, 2024
Merged

[lldb] Index static const members of classes, structs and unions as global variables in DWARF 4 and earlier #111859

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4c394ec
[lldb] Lookup static const members in FindGlobalVariables
kuilpd Jul 30, 2024
8f7c944
Implemented manual indexing of DW_TAG_member in DWARF 4, combined the…
kuilpd Oct 24, 2024
e34cd8f
Remove unused code and test
kuilpd Oct 29, 2024
86390ef
Assert that const static DW_TAG_member doesn't have a location
kuilpd Oct 29, 2024
dd67524
Move DWARF version check, remove the check for DW_AT_external, remove…
kuilpd Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,7 @@ DWARFDebugInfoEntry::GetAbbreviationDeclarationPtr(const DWARFUnit *cu) const {
}

bool DWARFDebugInfoEntry::IsGlobalOrStaticScopeVariable() const {
if (Tag() != DW_TAG_variable)
if (Tag() != DW_TAG_variable && Tag() != DW_TAG_member)
return false;
const DWARFDebugInfoEntry *parent_die = GetParent();
while (parent_die != nullptr) {
Expand Down
24 changes: 24 additions & 0 deletions lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ void ManualDWARFIndex::IndexUnitImpl(DWARFUnit &unit,
case DW_TAG_typedef:
case DW_TAG_union_type:
case DW_TAG_unspecified_type:
case DW_TAG_member:
case DW_TAG_variable:
break;

Expand All @@ -228,6 +229,7 @@ void ManualDWARFIndex::IndexUnitImpl(DWARFUnit &unit,

const char *name = nullptr;
const char *mangled_cstr = nullptr;
bool is_external = false;
bool is_declaration = false;
bool has_address = false;
bool has_location_or_const_value = false;
Expand All @@ -246,6 +248,11 @@ void ManualDWARFIndex::IndexUnitImpl(DWARFUnit &unit,
name = form_value.AsCString();
break;

case DW_AT_external:
if (attributes.ExtractFormValueAtIndex(i, form_value))
is_external = form_value.Unsigned() != 0;
break;

case DW_AT_declaration:
if (attributes.ExtractFormValueAtIndex(i, form_value))
is_declaration = form_value.Unsigned() != 0;
Expand Down Expand Up @@ -362,6 +369,23 @@ void ManualDWARFIndex::IndexUnitImpl(DWARFUnit &unit,
set.namespaces.Insert(ConstString(name), ref);
break;

case DW_TAG_member: {
// In DWARF 4 and earlier `static const` members of a struct, a class or a
// union have an entry tag `DW_TAG_member`, and are also tagged as
// `DW_AT_external` and `DW_AT_declaration`, but otherwise follow the
// same rules as `DW_TAG_variable`.
if (unit.GetVersion() >= 5)
break;
Copy link
Collaborator

@labath labath Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put this into the first switch statement (something like case DW_TAG_member: if (unit.GetVersion() >= 5) continue; else break;), so that we avoid parsing the attributes when we know we're not going to use them.

bool parent_is_class_type = false;
if (auto parent = die.GetParent()) {
parent_is_class_type = parent->Tag() == DW_TAG_structure_type ||
parent->Tag() == DW_TAG_class_type ||
parent->Tag() == DW_TAG_union_type;
}
Copy link
Collaborator

@labath labath Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool parent_is_class_type = false;
if (auto parent = die.GetParent()) {
parent_is_class_type = parent->Tag() == DW_TAG_structure_type ||
parent->Tag() == DW_TAG_class_type ||
parent->Tag() == DW_TAG_union_type;
}
bool parent_is_class_type = false;
if (auto parent = die.GetParent())
parent_is_class_type = DWARFDIE(&unit, parent).IsStructUnionOrClass();

if (!parent_is_class_type || !is_external || !is_declaration)
Copy link
Member

@Michael137 Michael137 Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to note that we have this block here in DWARFASTParserClang.cpp:

llvm-project/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

Lines 2824 to 2839 in d2e7ee7

// Handle static members, which are typically members without
// locations. However, GCC doesn't emit DW_AT_data_member_location
// for any union members (regardless of linkage).
// 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.
// The following block is only necessary to support DWARFv4 and earlier.
// Starting with 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.is_declaration) {
CreateStaticMemberVariable(die, attrs, class_clang_type);
return;
}

Which describes an edge-case with how GCC emits static data members (i.e., it doesn't consistently emit the DW_AT_external flag). Not sure we want to account for that, but also, it would be nice if we had a single place where we did the "isPreDWARFv5StaticDataMember` check. But feel free to ignore

Copy link
Collaborator

@labath labath Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good finding. I think it would be good to match whatever that code does. (making it a single function for it would be somewhat tricky, as the code here needs to be very fast, so we don't want to have the helper function do another lookup for the attributes and stuff).

break;
[[fallthrough]];
}
case DW_TAG_variable:
if (name && has_location_or_const_value && is_global_or_static_variable) {
set.globals.Insert(ConstString(name), ref);
Expand Down
10 changes: 7 additions & 3 deletions lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2402,8 +2402,10 @@ void SymbolFileDWARF::FindGlobalVariables(
sc.module_sp = m_objfile_sp->GetModule();
assert(sc.module_sp);

if (die.Tag() != DW_TAG_variable)
if (die.Tag() != DW_TAG_variable && die.Tag() != DW_TAG_member)
return true;
assert(
!die.GetAttributeValueAsOptionalUnsigned(DW_AT_data_member_location));

auto *dwarf_cu = llvm::dyn_cast<DWARFCompileUnit>(die.GetCU());
if (!dwarf_cu)
Expand Down Expand Up @@ -3490,8 +3492,9 @@ VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
ModuleSP module = GetObjectFile()->GetModule();

if (tag != DW_TAG_variable && tag != DW_TAG_constant &&
(tag != DW_TAG_formal_parameter || !sc.function))
tag != DW_TAG_member && (tag != DW_TAG_formal_parameter || !sc.function))
Copy link
Member

@Michael137 Michael137 Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we bail out (or at least assert) if we detect a DW_AT_data_member_location? Though I guess technically nothing should be calling ParseVariableDIE with such a DIE?

Copy link
Contributor Author

@kuilpd kuilpd Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an assert, but maybe I should just add that as a condition for detecting a static const member to begin with?

Copy link
Collaborator

@labath labath Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that should be an assert unless some piece of code (where?) alredy makes sure that these kinds of DIEs don't make their way here. Debug info can come from all kinds of compilers (including those from the past and the future), so we shouldn't be crashing just because we've ran into debug info (aka "user input") that we don't understand. Logging something or reporting a warning might be more appropriate.

Checking for this member when detecting a static const member might be okay, but it doesn't really count as the check I mention above, since it only works for the manual index. The other indexes come straight from the compiler (past, future, etc.) so they can't be relied upon in the same way. For this reason (and because we want to make the indexing step as fast as possible), I'd put the check into the indexing step only if it's necessary to properly detect static members.

return nullptr;
assert(!die.GetAttributeValueAsOptionalUnsigned(DW_AT_data_member_location));

DWARFAttributes attributes = die.GetAttributes();
const char *name = nullptr;
Expand Down Expand Up @@ -3796,8 +3799,9 @@ void SymbolFileDWARF::ParseAndAppendGlobalVariable(
return;

dw_tag_t tag = die.Tag();
if (tag != DW_TAG_variable && tag != DW_TAG_constant)
if (tag != DW_TAG_variable && tag != DW_TAG_constant && tag != DW_TAG_member)
return;
assert(!die.GetAttributeValueAsOptionalUnsigned(DW_AT_data_member_location));

// Check to see if we have already parsed this variable or constant?
VariableSP var_sp = GetDIEToVariable()[die.GetDIE()];
Expand Down
35 changes: 25 additions & 10 deletions lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,17 +120,15 @@ def check_global_var(self, name: str, expect_type, expect_val):
self.assertEqual(varobj.type.name, expect_type)
self.assertEqual(varobj.value, expect_val)

@expectedFailureAll(dwarf_version=["<", "5"])
# On linux this passes due to the manual index
@expectedFailureDarwin(debug_info=no_match(["dsym"]))
def test_inline_static_members(self):
self.build()
def check_inline_static_members(self, flags):
self.build(dictionary={"CXXFLAGS_EXTRAS": flags})
lldbutil.run_to_source_breakpoint(
self, "// break here", lldb.SBFileSpec("main.cpp")
)

self.check_global_var("A::int_val", "const int", "1")
self.check_global_var("A::int_val_with_address", "const int", "2")
self.check_global_var("A::inline_int_val", "const int", "3")
self.check_global_var("A::bool_val", "const bool", "true")
self.check_global_var("A::enum_val", "Enum", "enum_case2")
self.check_global_var("A::enum_bool_val", "EnumBool", "enum_bool_case1")
Expand All @@ -144,6 +142,16 @@ def test_inline_static_members(self):
"ClassWithConstexprs::scoped_enum_val", "ScopedEnum", "scoped_enum_case2"
)

# On linux this passes due to the manual index
@expectedFailureDarwin(debug_info=no_match(["dsym"]))
def test_inline_static_members_dwarf5(self):
self.check_inline_static_members("-gdwarf-5")

# On linux this passes due to the manual index
@expectedFailureDarwin(debug_info=no_match(["dsym"]))
def test_inline_static_members_dwarf4(self):
self.check_inline_static_members("-gdwarf-4")

# With older versions of Clang, LLDB fails to evaluate classes with only
# constexpr members when dsymutil is enabled
@expectedFailureAll(
Expand All @@ -170,15 +178,12 @@ def test_class_with_only_constexpr_static(self):
"ClassWithEnumAlias::enum_alias_alias", result_value="scoped_enum_case1"
)

@expectedFailureAll(dwarf_version=["<", "5"])
# On linux this passes due to the manual index
@expectedFailureDarwin(debug_info=no_match(["dsym"]))
def test_shadowed_static_inline_members(self):
def check_shadowed_static_inline_members(self, flags):
"""Tests that the expression evaluator and SBAPI can both
correctly determine the requested inline static variable
in the presence of multiple variables of the same name."""

self.build()
self.build(dictionary={"CXXFLAGS_EXTRAS": flags})
lldbutil.run_to_name_breakpoint(self, "bar")

self.check_global_var("ns::Foo::mem", "const int", "10")
Expand All @@ -188,6 +193,16 @@ def test_shadowed_static_inline_members(self):
self.expect_expr("ns::Foo::mem", result_value="10")
self.expect_expr("::Foo::mem", result_value="-29")

# On linux this passes due to the manual index
@expectedFailureDarwin(debug_info=no_match(["dsym"]))
def test_shadowed_static_inline_members_dwarf5(self):
self.check_shadowed_static_inline_members("-gdwarf-5")

# On linux this passes due to the manual index
@expectedFailureDarwin(debug_info=no_match(["dsym"]))
def test_shadowed_static_inline_members_dwarf4(self):
self.check_shadowed_static_inline_members("-gdwarf-4")

@expectedFailureAll(bugnumber="target var doesn't honour global namespace")
def test_shadowed_static_inline_members_xfail(self):
self.build()
Expand Down
1 change: 1 addition & 0 deletions lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ enum class ScopedLongLongEnum : long long {
struct A {
const static int int_val = 1;
const static int int_val_with_address = 2;
inline const static int inline_int_val = 3;
const static bool bool_val = true;

const static auto char_max = std::numeric_limits<char>::max();
Expand Down
Loading