[llvm-debuginfo-analyzer] Remove LVScope::Children container
#144750
Conversation
|
@llvm/pr-subscribers-llvm-binary-utilities @llvm/pr-subscribers-debuginfo Author: Javier Lopez-Gomez (jalopezg-git) ChangesRemove the Fixes #69160. Full diff: https://github.com/llvm/llvm-project/pull/144750.diff 5 Files Affected:
diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h
index eea06cfb99ba2..6dd9b5a2430d2 100644
--- a/llvm/include/llvm/ADT/STLExtras.h
+++ b/llvm/include/llvm/ADT/STLExtras.h
@@ -1030,14 +1030,15 @@ class concat_iterator
std::forward_iterator_tag, ValueT> {
using BaseT = typename concat_iterator::iterator_facade_base;
- static constexpr bool ReturnsByValue =
- !(std::is_reference_v<decltype(*std::declval<IterTs>())> && ...);
+ static constexpr bool ReturnsValueOrPointer =
+ !(std::is_reference_v<decltype(*std::declval<IterTs>())> && ...) ||
+ (std::is_pointer_v<IterTs> && ...);
using reference_type =
- typename std::conditional_t<ReturnsByValue, ValueT, ValueT &>;
+ typename std::conditional_t<ReturnsValueOrPointer, ValueT, ValueT &>;
using handle_type =
- typename std::conditional_t<ReturnsByValue, std::optional<ValueT>,
+ typename std::conditional_t<ReturnsValueOrPointer, std::optional<ValueT>,
ValueT *>;
/// We store both the current and end iterators for each concatenated
@@ -1088,7 +1089,7 @@ class concat_iterator
if (Begin == End)
return {};
- if constexpr (ReturnsByValue)
+ if constexpr (ReturnsValueOrPointer)
return *Begin;
else
return &*Begin;
diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
index 5715a37185b2b..afd30a24d0f8d 100644
--- a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
+++ b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
@@ -14,6 +14,7 @@
#ifndef LLVM_DEBUGINFO_LOGICALVIEW_CORE_LVSCOPE_H
#define LLVM_DEBUGINFO_LOGICALVIEW_CORE_LVSCOPE_H
+#include "llvm/ADT/STLExtras.h"
#include "llvm/DebugInfo/LogicalView/Core/LVElement.h"
#include "llvm/DebugInfo/LogicalView/Core/LVLocation.h"
#include "llvm/DebugInfo/LogicalView/Core/LVSort.h"
@@ -94,6 +95,11 @@ class LLVM_ABI LVScope : public LVElement {
LVProperties<LVScopeKind> Kinds;
LVProperties<Property> Properties;
static LVScopeDispatch Dispatch;
+ // Empty containers used in `getChildren()` in case there is no Types,
+ // Symbols, or Scopes.
+ static const LVTypes EmptyTypes;
+ static const LVSymbols EmptySymbols;
+ static const LVScopes EmptyScopes;
// Size in bits if this scope represents also a compound type.
uint32_t BitSize = 0;
@@ -128,14 +134,6 @@ class LLVM_ABI LVScope : public LVElement {
std::unique_ptr<LVLines> Lines;
std::unique_ptr<LVLocations> Ranges;
- // Vector of elements (types, scopes and symbols).
- // It is the union of (*Types, *Symbols and *Scopes) to be used for
- // the following reasons:
- // - Preserve the order the logical elements are read in.
- // - To have a single container with all the logical elements, when
- // the traversal does not require any specific element kind.
- std::unique_ptr<LVElements> Children;
-
// Resolve the template parameters/arguments relationship.
void resolveTemplate();
void printEncodedArgs(raw_ostream &OS, bool Full) const;
@@ -213,7 +211,11 @@ class LLVM_ABI LVScope : public LVElement {
const LVScopes *getScopes() const { return Scopes.get(); }
const LVSymbols *getSymbols() const { return Symbols.get(); }
const LVTypes *getTypes() const { return Types.get(); }
- const LVElements *getChildren() const { return Children.get(); }
+ auto getChildren() const {
+ return llvm::concat<LVElement *const>(Types ? *Types : EmptyTypes,
+ Symbols ? *Symbols : EmptySymbols,
+ Scopes ? *Scopes : EmptyScopes);
+ }
void addElement(LVElement *Element);
void addElement(LVLine *Line);
@@ -222,7 +224,6 @@ class LLVM_ABI LVScope : public LVElement {
void addElement(LVType *Type);
void addObject(LVLocation *Location);
void addObject(LVAddress LowerAddress, LVAddress UpperAddress);
- void addToChildren(LVElement *Element);
// Add the missing elements from the given 'Reference', which is the
// scope associated with any DW_AT_specification, DW_AT_abstract_origin.
diff --git a/llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp b/llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
index 55880fab5e88e..70422555ea27e 100644
--- a/llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
+++ b/llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
@@ -107,11 +107,9 @@ LVScopeDispatch LVScope::Dispatch = {
{LVScopeKind::IsTryBlock, &LVScope::getIsTryBlock},
{LVScopeKind::IsUnion, &LVScope::getIsUnion}};
-void LVScope::addToChildren(LVElement *Element) {
- if (!Children)
- Children = std::make_unique<LVElements>();
- Children->push_back(Element);
-}
+const LVTypes LVScope::EmptyTypes{};
+const LVSymbols LVScope::EmptySymbols{};
+const LVScopes LVScope::EmptyScopes{};
void LVScope::addElement(LVElement *Element) {
assert(Element && "Invalid element.");
@@ -175,7 +173,6 @@ void LVScope::addElement(LVScope *Scope) {
// Add it to parent.
Scopes->push_back(Scope);
- addToChildren(Scope);
Scope->setParent(this);
// Notify the reader about the new element being added.
@@ -202,7 +199,6 @@ void LVScope::addElement(LVSymbol *Symbol) {
// Add it to parent.
Symbols->push_back(Symbol);
- addToChildren(Symbol);
Symbol->setParent(this);
// Notify the reader about the new element being added.
@@ -229,7 +225,6 @@ void LVScope::addElement(LVType *Type) {
// Add it to parent.
Types->push_back(Type);
- addToChildren(Type);
Type->setParent(this);
// Notify the reader about the new element being added.
@@ -277,15 +272,12 @@ bool LVScope::removeElement(LVElement *Element) {
if (Element->getIsLine())
return RemoveElement(Lines);
- if (RemoveElement(Children)) {
- if (Element->getIsSymbol())
- return RemoveElement(Symbols);
- if (Element->getIsType())
- return RemoveElement(Types);
- if (Element->getIsScope())
- return RemoveElement(Scopes);
- llvm_unreachable("Invalid element.");
- }
+ if (Element->getIsSymbol())
+ return RemoveElement(Symbols);
+ if (Element->getIsType())
+ return RemoveElement(Types);
+ if (Element->getIsScope())
+ return RemoveElement(Scopes);
return false;
}
@@ -356,8 +348,8 @@ void LVScope::updateLevel(LVScope *Parent, bool Moved) {
setLevel(Parent->getLevel() + 1);
// Update the children.
- if (Children)
- for (LVElement *Element : *Children)
+ if (auto Elements = getChildren(); Elements.begin() != Elements.end())
+ for (LVElement *Element : Elements)
Element->updateLevel(this, Moved);
// Update any lines.
@@ -374,8 +366,8 @@ void LVScope::resolve() {
LVElement::resolve();
// Resolve the children.
- if (Children)
- for (LVElement *Element : *Children) {
+ if (auto Elements = getChildren(); Elements.begin() != Elements.end())
+ for (LVElement *Element : Elements) {
if (getIsGlobalReference())
// If the scope is a global reference, mark all its children as well.
Element->setIsGlobalReference();
@@ -633,8 +625,9 @@ Error LVScope::doPrint(bool Split, bool Match, bool Print, raw_ostream &OS,
options().getPrintFormatting() &&
getLevel() < options().getOutputLevel()) {
// Print the children.
- if (Children)
- for (const LVElement *Element : *Children) {
+ if (const auto Elements = getChildren();
+ Elements.begin() != Elements.end())
+ for (const LVElement *Element : Elements) {
if (Match && !Element->getHasPattern())
continue;
if (Error Err =
@@ -692,7 +685,6 @@ void LVScope::sort() {
Traverse(Parent->Symbols, SortFunction);
Traverse(Parent->Scopes, SortFunction);
Traverse(Parent->Ranges, compareRange);
- Traverse(Parent->Children, SortFunction);
if (Parent->Scopes)
for (LVScope *Scope : *Parent->Scopes)
@@ -978,8 +970,8 @@ bool LVScope::equals(const LVScopes *References, const LVScopes *Targets) {
void LVScope::report(LVComparePass Pass) {
getComparator().printItem(this, Pass);
getComparator().push(this);
- if (Children)
- for (LVElement *Element : *Children)
+ if (auto Elements = getChildren(); Elements.begin() != Elements.end())
+ for (LVElement *Element : Elements)
Element->report(Pass);
if (Lines)
@@ -1656,8 +1648,9 @@ void LVScopeCompileUnit::printMatchedElements(raw_ostream &OS,
// Print the view for the matched scopes.
for (const LVScope *Scope : MatchedScopes) {
Scope->print(OS);
- if (const LVElements *Elements = Scope->getChildren())
- for (LVElement *Element : *Elements)
+ if (const auto Elements = Scope->getChildren();
+ Elements.begin() != Elements.end())
+ for (LVElement *Element : Elements)
Element->print(OS);
}
}
diff --git a/llvm/unittests/ADT/STLExtrasTest.cpp b/llvm/unittests/ADT/STLExtrasTest.cpp
index 286cfa745fd14..98c13a33d35eb 100644
--- a/llvm/unittests/ADT/STLExtrasTest.cpp
+++ b/llvm/unittests/ADT/STLExtrasTest.cpp
@@ -398,6 +398,8 @@ struct some_struct {
std::string swap_val;
};
+struct derives_from_some_struct : some_struct {};
+
std::vector<int>::const_iterator begin(const some_struct &s) {
return s.data.begin();
}
@@ -532,6 +534,21 @@ TEST(STLExtrasTest, ConcatRangeADL) {
EXPECT_THAT(concat<const int>(S0, S1), ElementsAre(1, 2, 3, 4));
}
+TEST(STLExtrasTest, ConcatRangePtrToDerivedClass) {
+ auto S0 = std::make_unique<some_namespace::some_struct>();
+ auto S1 = std::make_unique<some_namespace::derives_from_some_struct>();
+ SmallVector<some_namespace::some_struct *> V0{S0.get()};
+ SmallVector<some_namespace::derives_from_some_struct *> V1{S1.get(),
+ S1.get()};
+
+ // Use concat over ranges of pointers to different (but related) types.
+ EXPECT_THAT(
+ concat<some_namespace::some_struct *>(V0, V1),
+ ElementsAre(S0.get(),
+ static_cast<some_namespace::some_struct *>(S1.get()),
+ static_cast<some_namespace::some_struct *>(S1.get())));
+}
+
TEST(STLExtrasTest, MakeFirstSecondRangeADL) {
// Make sure that we use the `begin`/`end` functions from `some_namespace`,
// using ADL.
diff --git a/llvm/unittests/DebugInfo/LogicalView/DWARFReaderTest.cpp b/llvm/unittests/DebugInfo/LogicalView/DWARFReaderTest.cpp
index 544c39a3c7b2e..ba6df7489b750 100644
--- a/llvm/unittests/DebugInfo/LogicalView/DWARFReaderTest.cpp
+++ b/llvm/unittests/DebugInfo/LogicalView/DWARFReaderTest.cpp
@@ -163,13 +163,12 @@ void checkUnspecifiedParameters(LVReader *Reader) {
LVPublicNames::const_iterator IterNames = PublicNames.cbegin();
LVScope *Function = (*IterNames).first;
EXPECT_EQ(Function->getName(), "foo_printf");
- const LVElements *Elements = Function->getChildren();
- ASSERT_NE(Elements, nullptr);
+ const auto Elements = Function->getChildren();
// foo_printf is a variadic function whose prototype is
// `int foo_printf(const char *, ...)`, where the '...' is represented by a
// DW_TAG_unspecified_parameters, i.e. we expect to find at least one child
// for which getIsUnspecified() returns true.
- EXPECT_TRUE(llvm::any_of(*Elements, [](const LVElement *elt) {
+ EXPECT_TRUE(llvm::any_of(Elements, [](const LVElement *elt) {
return elt->getIsSymbol() &&
static_cast<const LVSymbol *>(elt)->getIsUnspecified();
}));
@@ -183,10 +182,9 @@ void checkScopeModule(LVReader *Reader) {
EXPECT_EQ(Root->getFileFormatName(), "Mach-O 64-bit x86-64");
EXPECT_EQ(Root->getName(), DwarfClangModule);
- ASSERT_NE(CompileUnit->getChildren(), nullptr);
- LVElement *FirstChild = *(CompileUnit->getChildren()->begin());
- EXPECT_EQ(FirstChild->getIsScope(), 1);
- LVScopeModule *Module = static_cast<LVScopeModule *>(FirstChild);
+ ASSERT_NE(CompileUnit->getScopes(), nullptr);
+ LVElement *FirstScope = *(CompileUnit->getScopes()->begin());
+ LVScopeModule *Module = static_cast<LVScopeModule *>(FirstScope);
EXPECT_EQ(Module->getIsModule(), 1);
EXPECT_EQ(Module->getName(), "DebugModule");
}
|
|
This PR should yield further memory footprint improvements after #144399 is merged. As suggested in #69160, removing |
|
This required a minor change to |
From #69160 (comment) The idea behind the |
@CarlosAlbertoEnciso If order is important, then...
I'm thinking about this, but likely we could go for approach (1), and if overhead is a concern, one should use |
jalopezg-git
force-pushed
the
jalopezg-69160
branch
2 times, most recently
from
5f913f3 to
5a31ab8
Compare
|
@jalopezg-git I think the base branch for this patch should be #144744. |
@CarlosAlbertoEnciso That is right -- I can probably just rebase it atop |
jalopezg-git
force-pushed
the
jalopezg-69160
branch
3 times, most recently
from
ce0fb39 to
63fb246
Compare
| @@ -35,8 +35,8 @@ | |||
| ; ONE-NEXT: [002] 1 {TypeAlias} 'INTPTR' -> '* const int' | |||
| ; ONE-NEXT: [002] 2 {Function} extern not_inlined 'foo' -> 'int' | |||
| ; ONE-NEXT: [003] {Block} | |||
| ; ONE-NEXT: [004] 5 {Variable} 'CONSTANT' -> 'const INTEGER' | |||
| ; ONE-NEXT: +[004] 4 {TypeAlias} 'INTEGER' -> 'int' | |||
There was a problem hiding this comment.
Hmmm... that is weird, given that for comparison, sort mode is line (see LVOptions.cpp:219), unless I am mistaken, INTEGER should have come before CONSTANT, but it was not the case. Was this (partially) broken before?
FYI, @CarlosAlbertoEnciso.
; ONE-NEXT: [004] 5 {Variable} 'CONSTANT' -> 'const INTEGER'
; ONE-NEXT: +[004] 4 {TypeAlias} 'INTEGER' -> 'int'
llvm/test/tools/llvm-debuginfo-analyzer/DWARF/01-dwarf-select-logical-elements.test
Outdated
Show resolved
Hide resolved
| @@ -55,8 +55,8 @@ | |||
| ; ONE-NEXT: [002] 1 {TypeAlias} 'INTPTR' -> '* const int' | |||
| ; ONE-NEXT: [002] 2 {Function} extern not_inlined 'foo' -> 'int' | |||
| ; ONE-NEXT: [003] {Block} | |||
| ; ONE-NEXT: [004] 5 {Variable} 'CONSTANT' -> 'const INTEGER' | |||
There was a problem hiding this comment.
Same as before (see my other comment) -- also seen in llvm/test/tools/llvm-debuginfo-analyzer/WebAssembly/01-wasm-compare-logical-elements.test.
Fix llvm::concat_iterator for the case of `ValueT` being a pointer to a common base class to which the result of dereferencing any iterator in `ItersT` can be casted to.
Remove the `LVScope::Children` container and use `llvm::concat()` instead to return a view over the types, symbols, and sub-scopes contained in a given `LVScope`.
jalopezg-git
force-pushed
the
jalopezg-69160
branch
from
63fb246 to
2e4bf83
Compare
|
The changes that introduce the |
jalopezg-git
force-pushed
the
jalopezg-69160
branch
from
2e4bf83 to
c5cea09
Compare
I removed the change from this PR.
|
Remove the
LVScope::Childrencontainer and usellvm::concat()instead to return a view over the types, symbols, and sub-scopes contained in a givenLVScope.Fixes #69160.