From 461b1cc7154172be73aca68b01d1c73e4d2b3d19 Mon Sep 17 00:00:00 2001 From: Pietro Fezzardi Date: Mon, 3 May 2021 16:47:33 +0200 Subject: [PATCH] DLATypeSystem: simplify ownership, lookup, removal Before this commit DLATypeSystem used a std::set to hold nodes, allowing heterogenous lookup with bare pointers, fast removal being a set, and providing address stability without requiring an ordering between LayoutTypeSystemNodes. The same requirements is now obtained more cleanly using with a BumpPtrAllocator for nodes, and a set of naked pointers to them to enable fast lookup and removal. --- include/revng-c/ADT/HeterogeneousPtrCompare.h | 39 ----------- lib/Decompiler/DLATypeSystem.cpp | 69 ++++++++++--------- lib/Decompiler/DLATypeSystem.h | 29 +++----- 3 files changed, 47 insertions(+), 90 deletions(-) delete mode 100644 include/revng-c/ADT/HeterogeneousPtrCompare.h diff --git a/include/revng-c/ADT/HeterogeneousPtrCompare.h b/include/revng-c/ADT/HeterogeneousPtrCompare.h deleted file mode 100644 index af07d1573..000000000 --- a/include/revng-c/ADT/HeterogeneousPtrCompare.h +++ /dev/null @@ -1,39 +0,0 @@ -#pragma once - -// -// Copyright rev.ng Srls. See LICENSE.md for details. -// - -#include - -/// \brief Utility class template for heterogeneous comparison of pointers with -/// unique pointers. -// -// This is particularly useful when you have a std::set> or -// a std::map, T2>. In general you cannot perform lookups -// using raw pointers into such containers. -// Using this class template you can transform the into -// std::set, HeterogeneousPtrCompare> and -// std::map, T2, HeterogeneousPtrCompare>, which allows -// lookup using raw pointers. -template -struct HeterogeneousPtrCompare { - using is_transparent = std::true_type; - -private: - struct Helper { - const T *P; - Helper() = default; - ~Helper() = default; - Helper(const Helper &) = default; - Helper(Helper &&) = default; - Helper &operator=(const Helper &) = default; - Helper &operator=(Helper &&) = default; - Helper(const T *Ptr) : P(Ptr) {} - template - Helper(const std::unique_ptr &Ptr) : Helper(Ptr.get()) {} - }; - -public: - bool operator()(const Helper A, const Helper B) const { return A.P < B.P; } -}; diff --git a/lib/Decompiler/DLATypeSystem.cpp b/lib/Decompiler/DLATypeSystem.cpp index 33c46fe3f..5acd93af7 100644 --- a/lib/Decompiler/DLATypeSystem.cpp +++ b/lib/Decompiler/DLATypeSystem.cpp @@ -24,6 +24,12 @@ using namespace llvm; +using NodeAllocatorT = SpecificBumpPtrAllocator; + +void *operator new(size_t, NodeAllocatorT &NodeAllocator) { + return NodeAllocator.Allocate(); +} + namespace dla { void OffsetExpression::print(llvm::raw_ostream &OS) const { @@ -236,12 +242,12 @@ void LayoutTypeSystem::dumpDotOnFile(const char *FName) const { LayoutTypeSystemNode *LayoutTypeSystem::createArtificialLayoutType() { using LTSN = LayoutTypeSystemNode; - // Create a new layout - const auto &[LayoutIt, Success] = Layouts.insert(std::make_unique(NID)); + LTSN *New = new (NodeAllocator) LayoutTypeSystemNode(NID); + revng_assert(New); + ++NID; + bool Success = Layouts.insert(New).second; revng_assert(Success); - if (Success) - ++NID; - return LayoutIt->get(); + return New; } static void assertGetLayoutTypePreConditions(const Value *V, unsigned Id) { @@ -608,8 +614,6 @@ void LayoutTypeSystem::mergeNodes(const LayoutTypeSystemNodePtrVec &ToMerge) { for (LayoutTypeSystemNode *From : llvm::drop_begin(ToMerge, 1)) { revng_assert(From != Into); revng_log(MergeLog, "Merging: " << From << " Into: " << Into); - auto LayoutIt = Layouts.find(From); - revng_assert(LayoutIt != Layouts.end()); auto ToMergeLayoutToTypePtrsIt = LayoutToTypePtrsMap.find(From); revng_assert(ToMergeLayoutToTypePtrsIt != LayoutToTypePtrsMap.end()); @@ -635,40 +639,43 @@ void LayoutTypeSystem::mergeNodes(const LayoutTypeSystemNodePtrVec &ToMerge) { LayoutToTypePtrsMap.erase(ToMergeLayoutToTypePtrsIt); // Remove From from Layouts - Layouts.erase(LayoutIt); + bool Erased = Layouts.erase(From); + revng_assert(Erased); + __asan_poison_memory_region(From, sizeof(LayoutTypeSystemNode)); } } -void LayoutTypeSystem::removeNode(LayoutTypeSystemNode *N) { - auto It = LayoutToTypePtrsMap.find(N); +void LayoutTypeSystem::removeNode(LayoutTypeSystemNode *ToRemove) { + revng_assert(ToRemove); + auto It = LayoutToTypePtrsMap.find(ToRemove); revng_assert(It != LayoutToTypePtrsMap.end()); for (auto P : It->second) TypePtrToLayoutMap.erase(P); LayoutToTypePtrsMap.erase(It); - auto LayoutIt = Layouts.find(N); - revng_assert(LayoutIt != Layouts.end()); - const auto IsN = [N](const LayoutTypeSystemNode::Link &L) { - return L.first == N; + const auto IsToRemove = [ToRemove](const LayoutTypeSystemNode::Link &L) { + return L.first == ToRemove; }; - for (auto &[Neighbor, Tag] : LayoutIt->get()->Successors) { + for (auto &[Neighbor, Tag] : ToRemove->Successors) { auto PredBegin = Neighbor->Predecessors.begin(); auto PredEnd = Neighbor->Predecessors.end(); - auto It = std::find_if(PredBegin, PredEnd, IsN); - auto End = std::find_if_not(It, PredEnd, IsN); + auto It = std::find_if(PredBegin, PredEnd, IsToRemove); + auto End = std::find_if_not(It, PredEnd, IsToRemove); Neighbor->Predecessors.erase(It, End); } - for (auto &[Neighbor, Tag] : LayoutIt->get()->Predecessors) { + for (auto &[Neighbor, Tag] : ToRemove->Predecessors) { auto SuccBegin = Neighbor->Successors.begin(); auto SuccEnd = Neighbor->Successors.end(); - auto It = std::find_if(SuccBegin, SuccEnd, IsN); - auto End = std::find_if_not(It, SuccEnd, IsN); + auto It = std::find_if(SuccBegin, SuccEnd, IsToRemove); + auto End = std::find_if_not(It, SuccEnd, IsToRemove); Neighbor->Successors.erase(It, End); } - Layouts.erase(LayoutIt); + bool Erased = Layouts.erase(ToRemove); + revng_assert(Erased); + __asan_poison_memory_region(ToRemove, sizeof(LayoutTypeSystemNode)); } static void moveEdgesWithoutSumming(LayoutTypeSystemNode *OldSrc, @@ -798,14 +805,14 @@ void LayoutTypeSystem::moveEdges(LayoutTypeSystemNode *OldSrc, static Logger<> VerifyDLALog("dla-verify-strict"); bool LayoutTypeSystem::verifyConsistency() const { - for (auto &NodeUPtr : Layouts) { - if (NodeUPtr.get() == nullptr) { + for (LayoutTypeSystemNode *NodePtr : Layouts) { + if (not NodePtr) { if (VerifyDLALog.isEnabled()) revng_check(false); return false; } // Check that predecessors and successors are consistent - for (auto &P : NodeUPtr->Predecessors) { + for (auto &P : NodePtr->Predecessors) { if (P.first == nullptr) { if (VerifyDLALog.isEnabled()) revng_check(false); @@ -813,14 +820,14 @@ bool LayoutTypeSystem::verifyConsistency() const { } // same edge with same tag - auto It = P.first->Successors.find({ NodeUPtr.get(), P.second }); + auto It = P.first->Successors.find({ NodePtr, P.second }); if (It == P.first->Successors.end()) { if (VerifyDLALog.isEnabled()) revng_check(false); return false; } } - for (auto &P : NodeUPtr->Successors) { + for (auto &P : NodePtr->Successors) { if (P.first == nullptr) { if (VerifyDLALog.isEnabled()) revng_check(false); @@ -828,7 +835,7 @@ bool LayoutTypeSystem::verifyConsistency() const { } // same edge with same tag - auto It = P.first->Predecessors.find({ NodeUPtr.get(), P.second }); + auto It = P.first->Predecessors.find({ NodePtr, P.second }); if (It == P.first->Predecessors.end()) { if (VerifyDLALog.isEnabled()) revng_check(false); @@ -837,18 +844,18 @@ bool LayoutTypeSystem::verifyConsistency() const { } // Check that there are no self-edges - for (auto &P : NodeUPtr->Predecessors) { + for (auto &P : NodePtr->Predecessors) { LayoutTypeSystemNode *Pred = P.first; - if (Pred == NodeUPtr.get()) { + if (Pred == NodePtr) { if (VerifyDLALog.isEnabled()) revng_check(false); return false; } } - for (auto &P : NodeUPtr->Successors) { + for (auto &P : NodePtr->Successors) { LayoutTypeSystemNode *Succ = P.first; - if (Succ == NodeUPtr.get()) { + if (Succ == NodePtr) { if (VerifyDLALog.isEnabled()) revng_check(false); return false; diff --git a/lib/Decompiler/DLATypeSystem.h b/lib/Decompiler/DLATypeSystem.h index 04df18686..364b0633e 100644 --- a/lib/Decompiler/DLATypeSystem.h +++ b/lib/Decompiler/DLATypeSystem.h @@ -22,7 +22,6 @@ #include "revng/ADT/FilteredGraphTraits.h" #include "revng/Support/Assert.h" -#include "revng-c/ADT/HeterogeneousPtrCompare.h" #include "revng-c/Decompiler/DLALayouts.h" namespace dla { @@ -234,8 +233,7 @@ public: auto getNumLayouts() const { return Layouts.size(); } auto getLayoutsRange() const { - return llvm::make_range(llvm::map_iterator(Layouts.begin(), getNodePtr), - llvm::map_iterator(Layouts.end(), getNodePtr)); + return llvm::make_range(Layouts.begin(), Layouts.end()); } public: @@ -263,9 +261,12 @@ private: uint64_t NID = 0ULL; // Holds all the LayoutTypeSystemNode - std::set, - HeterogeneousPtrCompare> - Layouts = {}; + llvm::SpecificBumpPtrAllocator NodeAllocator = {}; + std::set Layouts = {}; + + // Holds the link tags, so that they can be deduplicated and referred to using + // TypeLinkTag * in the links inside LayoutTypeSystemNode + std::set LinkTags = {}; // Maps llvm::Value to layout types. // This map is updated along the way when the DLA algorithm merges @@ -277,10 +278,6 @@ private: std::map> LayoutToTypePtrsMap = {}; - // Holds the link tags, so that they can be deduplicated and referred to using - // TypeLinkTag * in the links inside LayoutTypeSystemNode - std::set LinkTags = {}; - public: // Checks that is valid, and returns true if it is, false otherwise bool verifyConsistency() const; @@ -437,13 +434,9 @@ public: template<> struct llvm::GraphTraits : public llvm::GraphTraits { -protected: - using NodeSetItT = std::set::iterator; - using NodeUniquePtr = dla::LayoutTypeSystem::NodeUniquePtr; - using GetPtrT = dla::LayoutTypeSystem::NodePtr (*)(const NodeUniquePtr &); public: - using nodes_iterator = llvm::mapped_iterator; + using nodes_iterator = std::set::iterator; static NodeRef getEntryNode(const dla::LayoutTypeSystem *) { return nullptr; } @@ -463,13 +456,9 @@ public: template<> struct llvm::GraphTraits : public llvm::GraphTraits { -protected: - using NodeSetItT = std::set::iterator; - using NodeUniquePtr = dla::LayoutTypeSystem::NodeUniquePtr; - using GetPtrT = dla::LayoutTypeSystem::NodePtr (*)(const NodeUniquePtr &); public: - using nodes_iterator = llvm::mapped_iterator; + using nodes_iterator = std::set::iterator; static NodeRef getEntryNode(const dla::LayoutTypeSystem *) { return nullptr; }