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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
19 changes: 19 additions & 0 deletions lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,13 @@ void ManualDWARFIndex::IndexUnitImpl(DWARFUnit &unit,
case DW_TAG_variable:
break;

case DW_TAG_member:
// Only in DWARF 4 and earlier `static const` members of a struct, a class
// or a union have an entry tag `DW_TAG_member`
if (unit.GetVersion() >= 5)
continue;
break;

default:
continue;
}
Expand Down Expand Up @@ -362,6 +369,18 @@ 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_declaration`, but otherwise follow the same rules as
// `DW_TAG_variable`.
bool parent_is_class_type = false;
if (auto parent = die.GetParent())
parent_is_class_type = DWARFDIE(&unit, parent).IsStructUnionOrClass();
Copy link
Member

Choose a reason for hiding this comment

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

Can this just be:

Suggested change
parent_is_class_type = DWARFDIE(&unit, parent).IsStructUnionOrClass();
parent_is_class_type = parent->IsStructUnionOrClass();

?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, because parent is a DWARFDebugInfoEntry, not a DWARFDIE :P

It's a (not entirely fortunate) result of trying to avoid any high level (expensive) operations, and of the fact that lldb's DWARFDebugInfoEntry (unlike the llvm) fork actually provides some functionality instead of being a simple data holder.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, yea that's unfortunate

if (!parent_is_class_type || !is_declaration)
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
6 changes: 3 additions & 3 deletions lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2402,7 +2402,7 @@ 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;

auto *dwarf_cu = llvm::dyn_cast<DWARFCompileUnit>(die.GetCU());
Expand Down Expand Up @@ -3490,7 +3490,7 @@ 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

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

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

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;

DWARFAttributes attributes = die.GetAttributes();
Expand Down Expand Up @@ -3796,7 +3796,7 @@ 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;

// Check to see if we have already parsed this variable or constant?
Expand Down
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
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