Skip to content

Commit 5153a90

Browse files
authored
[lldb][DWARF] Change GetAttributes to always visit current DIE before recursing (#123261)
`GetAttributes` returns all attributes on a given DIE, including any attributes that the DIE references via `DW_AT_abstract_origin` and `DW_AT_specification`. However, if an attribute exists on both the referring DIE and the referenced DIE, the first one encountered will be the one that takes precendence when querying the returned `DWARFAttributes`. But there was no guarantee in which order those attributes get visited. That means there's no convenient way of ensuring that an attribute of a definition doesn't get shadowed by one found on the declaration. One use-case where we don't want this to happen is for `DW_AT_object_pointer` (which can exist on both definitions and declarations, see #123089). This patch makes sure we visit the current DIE's attributes before following DIE references. I tried keeping as much of the original `GetAttributes` unchanged and just add an outer `GetAttributes` that keeps track of the DIEs we need to visit next. There's precendent for this iteration order in `llvm::DWARFDie::findRecursively` and also `lldb_private::ElaboratingDIEIterator`. We could use the latter to implement `GetAttributes`, though it also follows `DW_AT_signature` so I decided to leave it for follow-up.
1 parent 41f430a commit 5153a90

File tree

4 files changed

+727
-33
lines changed

4 files changed

+727
-33
lines changed

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp

Lines changed: 61 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -281,22 +281,34 @@ bool DWARFDebugInfoEntry::GetDIENamesAndRanges(
281281
return !ranges.empty();
282282
}
283283

284-
// Get all attribute values for a given DIE, including following any
285-
// specification or abstract origin attributes and including those in the
286-
// results. Any duplicate attributes will have the first instance take
287-
// precedence (this can happen for declaration attributes).
288-
void DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu,
289-
DWARFAttributes &attributes,
290-
Recurse recurse,
291-
uint32_t curr_depth) const {
292-
const auto *abbrevDecl = GetAbbreviationDeclarationPtr(cu);
293-
if (!abbrevDecl) {
294-
attributes.Clear();
295-
return;
296-
}
284+
/// Helper for the public \ref DWARFDebugInfoEntry::GetAttributes API.
285+
/// Adds all attributes of the DIE at the top of the \c worklist to the
286+
/// \c attributes list. Specifcations and abstract origins are added
287+
/// to the \c worklist if the referenced DIE has not been seen before.
288+
static bool GetAttributes(llvm::SmallVector<DWARFDIE> &worklist,
289+
llvm::SmallSet<DWARFDebugInfoEntry const *, 3> &seen,
290+
DWARFAttributes &attributes) {
291+
assert(!worklist.empty() && "Need at least one DIE to visit.");
292+
assert(seen.size() >= 1 &&
293+
"Need to have seen at least the currently visited entry.");
294+
295+
DWARFDIE current = worklist.pop_back_val();
296+
297+
const auto *cu = current.GetCU();
298+
assert(cu);
299+
300+
const auto *entry = current.GetDIE();
301+
assert(entry);
302+
303+
const auto *abbrevDecl =
304+
entry->GetAbbreviationDeclarationPtr(current.GetCU());
305+
if (!abbrevDecl)
306+
return false;
297307

298308
const DWARFDataExtractor &data = cu->GetData();
299-
lldb::offset_t offset = GetFirstAttributeOffset();
309+
lldb::offset_t offset = current.GetDIE()->GetFirstAttributeOffset();
310+
311+
const bool is_first_die = seen.size() == 1;
300312

301313
for (const auto &attribute : abbrevDecl->attributes()) {
302314
DWARFFormValue form_value(cu);
@@ -309,10 +321,10 @@ void DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu,
309321
switch (attr) {
310322
case DW_AT_sibling:
311323
case DW_AT_declaration:
312-
if (curr_depth > 0) {
324+
if (!is_first_die) {
313325
// This attribute doesn't make sense when combined with the DIE that
314326
// references this DIE. We know a DIE is referencing this DIE because
315-
// curr_depth is not zero
327+
// we've visited more than one DIE already.
316328
break;
317329
}
318330
[[fallthrough]];
@@ -321,13 +333,12 @@ void DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu,
321333
break;
322334
}
323335

324-
if (recurse == Recurse::yes &&
325-
((attr == DW_AT_specification) || (attr == DW_AT_abstract_origin))) {
336+
if (attr == DW_AT_specification || attr == DW_AT_abstract_origin) {
326337
if (form_value.ExtractValue(data, &offset)) {
327-
DWARFDIE spec_die = form_value.Reference();
328-
if (spec_die)
329-
spec_die.GetDIE()->GetAttributes(spec_die.GetCU(), attributes,
330-
recurse, curr_depth + 1);
338+
if (DWARFDIE spec_die = form_value.Reference()) {
339+
if (seen.insert(spec_die.GetDIE()).second)
340+
worklist.push_back(spec_die);
341+
}
331342
}
332343
} else {
333344
const dw_form_t form = form_value.Form();
@@ -339,6 +350,34 @@ void DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu,
339350
DWARFFormValue::SkipValue(form, data, &offset, cu);
340351
}
341352
}
353+
354+
return true;
355+
}
356+
357+
DWARFAttributes DWARFDebugInfoEntry::GetAttributes(const DWARFUnit *cu,
358+
Recurse recurse) const {
359+
// FIXME: use ElaboratingDIEIterator to follow specifications/abstract origins
360+
// instead of maintaining our own worklist/seen list.
361+
362+
DWARFAttributes attributes;
363+
364+
llvm::SmallVector<DWARFDIE, 3> worklist;
365+
worklist.emplace_back(cu, this);
366+
367+
// Keep track if DIEs already seen to prevent infinite recursion.
368+
// Value of '3' was picked for the same reason that
369+
// DWARFDie::findRecursively does.
370+
llvm::SmallSet<DWARFDebugInfoEntry const *, 3> seen;
371+
seen.insert(this);
372+
373+
do {
374+
if (!::GetAttributes(worklist, seen, attributes)) {
375+
attributes.Clear();
376+
break;
377+
}
378+
} while (!worklist.empty() && recurse == Recurse::yes);
379+
380+
return attributes;
342381
}
343382

344383
// GetAttributeValue

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -52,12 +52,28 @@ class DWARFDebugInfoEntry {
5252
lldb::offset_t *offset_ptr);
5353

5454
using Recurse = DWARFBaseDIE::Recurse;
55-
DWARFAttributes GetAttributes(DWARFUnit *cu,
56-
Recurse recurse = Recurse::yes) const {
57-
DWARFAttributes attrs;
58-
GetAttributes(cu, attrs, recurse, 0 /* curr_depth */);
59-
return attrs;
60-
}
55+
56+
/// Get all attribute values for a given DIE, optionally following any
57+
/// specifications and abstract origins and including their attributes
58+
/// in the result too.
59+
///
60+
/// When following specifications/abstract origins, the attributes
61+
/// on the referring DIE are guaranteed to be visited before the attributes of
62+
/// the referenced DIE.
63+
///
64+
/// \param[in] cu DWARFUnit that this entry belongs to.
65+
///
66+
/// \param[in] recurse If set to \c Recurse::yes, will include attributes
67+
/// on DIEs referenced via \c DW_AT_specification and \c DW_AT_abstract_origin
68+
/// (including across multiple levels of indirection).
69+
///
70+
/// \returns DWARFAttributes that include all attributes found on this DIE
71+
/// (and possibly referenced DIEs). Attributes may appear multiple times
72+
/// (e.g., if a declaration and definition both specify the same attribute).
73+
/// On failure, the returned DWARFAttributes will be empty.
74+
///
75+
DWARFAttributes GetAttributes(const DWARFUnit *cu,
76+
Recurse recurse = Recurse::yes) const;
6177

6278
dw_offset_t GetAttributeValue(const DWARFUnit *cu, const dw_attr_t attr,
6379
DWARFFormValue &formValue,
@@ -178,10 +194,6 @@ class DWARFDebugInfoEntry {
178194
/// A copy of the DW_TAG value so we don't have to go through the compile
179195
/// unit abbrev table
180196
dw_tag_t m_tag = llvm::dwarf::DW_TAG_null;
181-
182-
private:
183-
void GetAttributes(DWARFUnit *cu, DWARFAttributes &attrs, Recurse recurse,
184-
uint32_t curr_depth) const;
185197
};
186198
} // namespace dwarf
187199
} // namespace lldb_private::plugin

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3414,7 +3414,10 @@ VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
34143414
mangled = form_value.AsCString();
34153415
break;
34163416
case DW_AT_type:
3417-
type_die_form = form_value;
3417+
// DW_AT_type on declaration may be less accurate than
3418+
// that of definition, so don't overwrite it.
3419+
if (!type_die_form.IsValid())
3420+
type_die_form = form_value;
34183421
break;
34193422
case DW_AT_external:
34203423
is_external = form_value.Boolean();

0 commit comments

Comments
 (0)