From cb51965f6eb50b20a56b792246d03b68a6ffa91c Mon Sep 17 00:00:00 2001 From: Pietro Fezzardi Date: Tue, 1 Mar 2022 12:01:09 +0100 Subject: [PATCH] DLAComputeNonInterferingComponents: fix moveEdges This DLAStep was moving edges improperly before this commit. In particular, edges were detected solely looking at source and target edge, not looking at the edge itself. This was leading to wrong results whenever a node N1 had many outgoing edges to a child node N2, at different offsets, where all the edges were moved instead of just the correct ones. In order to fix this, this commit: - reworks the logic of `moveEdges`, switching to iterator-based logic - reworks the struct OrderedChild used internally by DLAComputeNonInterferingComponents, so that it is also iterator-based - re-uses common code for field size computation --- .../DataLayoutAnalysis/DLATypeSystem.h | 8 +- lib/DataLayoutAnalysis/DLATypeSystem.cpp | 129 +++++++----------- .../DLAComputeNonInterferingComponents.cpp | 125 ++++++++--------- 3 files changed, 110 insertions(+), 152 deletions(-) diff --git a/include/revng-c/DataLayoutAnalysis/DLATypeSystem.h b/include/revng-c/DataLayoutAnalysis/DLATypeSystem.h index 89c0b89f6..959cfef0a 100644 --- a/include/revng-c/DataLayoutAnalysis/DLATypeSystem.h +++ b/include/revng-c/DataLayoutAnalysis/DLATypeSystem.h @@ -289,10 +289,10 @@ public: void removeNode(LayoutTypeSystemNode *N); - void moveEdges(LayoutTypeSystemNode *OldSrc, - LayoutTypeSystemNode *NewSrc, - LayoutTypeSystemNode *Tgt, - int64_t OffsetToSum); + void moveEdge(LayoutTypeSystemNode *OldSrc, + LayoutTypeSystemNode *NewSrc, + LayoutTypeSystemNode::NeighborsSet::iterator EdgeIt, + int64_t OffsetToSum); private: uint64_t NID = 0ULL; diff --git a/lib/DataLayoutAnalysis/DLATypeSystem.cpp b/lib/DataLayoutAnalysis/DLATypeSystem.cpp index 37fa6b38d..df342512c 100644 --- a/lib/DataLayoutAnalysis/DLATypeSystem.cpp +++ b/lib/DataLayoutAnalysis/DLATypeSystem.cpp @@ -289,6 +289,8 @@ void LayoutTypeSystem::mergeNodes(const LayoutTypeSystemNodePtrVec &ToMerge) { fixPredSucc(From, Into); Into->InterferingInfo = Unknown; + revng_assert(not Into->Size or From->Size <= Into->Size); + Into->Size = std::max(Into->Size, From->Size); // Remove From from Layouts bool Erased = Layouts.erase(From); @@ -323,106 +325,73 @@ void LayoutTypeSystem::removeNode(LayoutTypeSystemNode *ToRemove) { NodeAllocator.Deallocate(ToRemove); } -static void moveEdgesWithoutSumming(LayoutTypeSystemNode *OldSrc, - LayoutTypeSystemNode *NewSrc, - LayoutTypeSystemNode *Tgt) { - // First, move successor edges from OldSrc to NewSrc - { - auto &OldSucc = OldSrc->Successors; - auto OldToTgtIt = OldSucc.lower_bound({ Tgt, nullptr }); - auto OldToTgtEnd = OldSucc.upper_bound({ std::next(Tgt), nullptr }); +using NeighborIterator = LayoutTypeSystemNode::NeighborsSet::iterator; - // Here we can move the edge descriptors directly to NewSucc, because we - // don't need to update the offset. - auto &NewSucc = NewSrc->Successors; - while (OldToTgtIt != OldToTgtEnd) { - auto Next = std::next(OldToTgtIt); - NewSucc.insert(OldSucc.extract(OldToTgtIt)); - OldToTgtIt = Next; - } - } +static void moveEdgeWithoutSumming(LayoutTypeSystemNode *OldSrc, + LayoutTypeSystemNode *NewSrc, + NeighborIterator EdgeIt) { - // Then, move predecessor edges from OldSrc to NewSrc - { - auto &TgtPred = Tgt->Predecessors; - auto TgtToOldIt = TgtPred.lower_bound({ OldSrc, nullptr }); - auto TgtToOldEnd = TgtPred.upper_bound({ std::next(OldSrc), nullptr }); + // First, move successor edge from OldSrc to NewSrc + auto SuccHandle = OldSrc->Successors.extract(EdgeIt); + revng_assert(not SuccHandle.empty()); + NewSrc->Successors.insert(std::move(SuccHandle)); - // Here we can extract the edge descriptors, update they key (representing - // the predecessor) and re-insert them, becasue we don't need to change the - // offset. - while (TgtToOldIt != TgtToOldEnd) { - auto Next = std::next(TgtToOldIt); - - auto OldPredEdge = TgtPred.extract(TgtToOldIt); - OldPredEdge.value().first = NewSrc; - TgtPred.insert(std::move(OldPredEdge)); - - TgtToOldIt = Next; - } - } + // Then, move predecessor edge from OldSrc to NewSrc + LayoutTypeSystemNode *Tgt = EdgeIt->first; + auto PredHandle = Tgt->Predecessors.extract({ OldSrc, EdgeIt->second }); + revng_assert(not PredHandle.empty()); + PredHandle.value().first = NewSrc; + Tgt->Predecessors.insert(std::move(PredHandle)); } -void LayoutTypeSystem::moveEdges(LayoutTypeSystemNode *OldSrc, - LayoutTypeSystemNode *NewSrc, - LayoutTypeSystemNode *Tgt, - int64_t OffsetToSum) { +void LayoutTypeSystem::moveEdge(LayoutTypeSystemNode *OldSrc, + LayoutTypeSystemNode *NewSrc, + NeighborIterator EdgeIt, + int64_t OffsetToSum) { - if (not OldSrc or not NewSrc or not Tgt) + if (not OldSrc or not NewSrc) return; if (not OffsetToSum) - return moveEdgesWithoutSumming(OldSrc, NewSrc, Tgt); + return moveEdgeWithoutSumming(OldSrc, NewSrc, EdgeIt); + + LayoutTypeSystemNode *Tgt = EdgeIt->first; // First, move successor edges from OldSrc to NewSrc - { - auto &OldSucc = OldSrc->Successors; - auto OldToTgtIt = OldSucc.lower_bound({ Tgt, nullptr }); - auto OldToTgtEnd = OldSucc.upper_bound({ std::next(Tgt), nullptr }); + auto OldSuccHandle = OldSrc->Successors.extract(EdgeIt); + revng_assert(not OldSuccHandle.empty()); - // Add new instance links with adjusted offsets from NewSrc to Tgt. - // Using the addInstanceLink methods already marks injects NewSrc among the - // predecessors of Tgt, so after this we only need to remove OldSrc from - // Tgt's predecessors and we're done. - while (OldToTgtIt != OldToTgtEnd) { - auto Next = std::next(OldToTgtIt); - auto OldSuccEdge = OldSucc.extract(OldToTgtIt); + // Add new instance links with adjusted offsets from NewSrc to Tgt. + // Using the addInstanceLink methods already marks injects NewSrc among the + // predecessors of Tgt, so after this we only need to remove OldSrc from + // Tgt's predecessors and we're done. - const TypeLinkTag *EdgeTag = OldSuccEdge.value().second; - switch (EdgeTag->getKind()) { + const TypeLinkTag *EdgeTag = OldSuccHandle.value().second; + switch (EdgeTag->getKind()) { - case TypeLinkTag::LK_Inheritance: { - if (OffsetToSum > 0LL) - addInstanceLink(NewSrc, Tgt, OffsetExpression(OffsetToSum)); - else - addInheritanceLink(NewSrc, Tgt); - } break; + case TypeLinkTag::LK_Inheritance: { + if (OffsetToSum > 0LL) + addInstanceLink(NewSrc, Tgt, OffsetExpression(OffsetToSum)); + else + addInheritanceLink(NewSrc, Tgt); + } break; - case TypeLinkTag::LK_Instance: { - OffsetExpression NewOE = EdgeTag->getOffsetExpr(); - NewOE.Offset += OffsetToSum; - revng_assert(NewOE.Offset >= 0LL); - addInstanceLink(NewSrc, Tgt, std::move(NewOE)); - } break; + case TypeLinkTag::LK_Instance: { + OffsetExpression NewOE = EdgeTag->getOffsetExpr(); + NewOE.Offset += OffsetToSum; + revng_assert(NewOE.Offset >= 0LL); + addInstanceLink(NewSrc, Tgt, std::move(NewOE)); + } break; - case TypeLinkTag::LK_Equality: - case TypeLinkTag::LK_Pointer: - default: - revng_unreachable("unexpected edge kind"); - } - - OldToTgtIt = Next; - } + case TypeLinkTag::LK_Equality: + case TypeLinkTag::LK_Pointer: + default: + revng_unreachable("unexpected edge kind"); } // Then, remove all the remaining info in Tgt that represent the fact that // OldSrc was a predecessor. - { - auto &TgtPred = Tgt->Predecessors; - auto TgtToOldIt = TgtPred.lower_bound({ OldSrc, nullptr }); - auto TgtToOldEnd = TgtPred.upper_bound({ std::next(OldSrc), nullptr }); - TgtPred.erase(TgtToOldIt, TgtToOldEnd); - } + auto PredHandle = Tgt->Predecessors.extract({ OldSrc, EdgeIt->second }); } static Logger<> VerifyDLALog("dla-verify-strict"); diff --git a/lib/DataLayoutAnalysis/Middleend/DLAComputeNonInterferingComponents.cpp b/lib/DataLayoutAnalysis/Middleend/DLAComputeNonInterferingComponents.cpp index fd12ac071..29887bf91 100644 --- a/lib/DataLayoutAnalysis/Middleend/DLAComputeNonInterferingComponents.cpp +++ b/lib/DataLayoutAnalysis/Middleend/DLAComputeNonInterferingComponents.cpp @@ -17,6 +17,7 @@ #include "revng-c/DataLayoutAnalysis/DLATypeSystem.h" #include "DLAStep.h" +#include "FieldSizeComputation.h" namespace dla { @@ -43,73 +44,57 @@ bool ComputeNonInterferingComponents::runOnTypeSystem(LayoutTypeSystem &TS) { revng_assert(N->Size); struct OrderedChild { - int64_t Offset; - decltype(N->Size) Size; - LTSN *Child; - // Make it sortable - std::strong_ordering operator<=>(const OrderedChild &) const = default; + dla::LayoutTypeSystemNode::NeighborsSet::iterator ChildIt; + size_t FieldSize; + + // Make it sortable with a different order + std::strong_ordering operator<=>(const OrderedChild &Other) const { + // Helper to treat Inheritance edges like InstanceAtOffset0 edges + const auto Off0Tag = TypeLinkTag::instanceTag(OffsetExpression{}); + + auto &ThisEdgeTag = isInheritanceEdge(*ChildIt) ? Off0Tag : + *ChildIt->second; + auto &OtherEdgeTag = isInheritanceEdge(*Other.ChildIt) ? + Off0Tag : + *Other.ChildIt->second; + + // Stuff that starts earlier goes first + if (auto Cmp = ThisEdgeTag <=> OtherEdgeTag; 0 != Cmp) + return Cmp; + + // Smaller stuff goes first + if (auto Cmp = FieldSize <=> Other.FieldSize; 0 != Cmp) + return Cmp; + + // Finally sort by address + return ChildIt->first <=> Other.ChildIt->first; + } + + auto getBeginEndByte() const { + auto ChildBeginByte = isInheritanceEdge(*ChildIt) ? + 0 : + ChildIt->second->getOffsetExpr().Offset; + auto ChildEndByte = ChildBeginByte + FieldSize; + return std::make_pair(ChildBeginByte, ChildEndByte); + } }; using ChildrenVec = llvm::SmallVector; using OrderedChildIt = ChildrenVec::iterator; // Collect the children in a vector. Here we use the OrderedChild struct, - // that embeds info on the size and offset of the children, so that we can - // later sort the vector according to it. + // that has a dedicated <=> operator so that we can later sort the vector + // according to it. ChildrenVec Children; - bool InheritsFromOther = false; - for (auto &[Child, EdgeTag] : - llvm::children_edges(N)) { + auto NChildIt = N->Successors.begin(); + auto NChildEnd = N->Successors.end(); + for (; NChildIt != NChildEnd; ++NChildIt) { + if (isPointerEdge(*NChildIt)) + continue; - auto OrdChild = OrderedChild{ - /* .Offset */ 0LL, - /* .Size */ Child->Size, - /* .Child */ Child, - }; - - switch (EdgeTag->getKind()) { - - case TypeLinkTag::LK_Instance: { - const OffsetExpression &OE = EdgeTag->getOffsetExpr(); - revng_assert(OE.Offset >= 0LL); - revng_assert(OE.Strides.size() == OE.TripCounts.size()); - - OrdChild.Offset = OE.Offset; - - for (const auto &[TripCount, Stride] : - llvm::reverse(llvm::zip(OE.TripCounts, OE.Strides))) { - - revng_assert(Stride > 0LL); - auto StrideSize = static_cast(Stride); - - // If we have a TripCount, we expect it to be strictly positive. - revng_assert(not TripCount.has_value() or TripCount.value() > 0LL); - - // Arrays with unknown numbers of elements are considered as if - // they had a single element - auto NumElems = TripCount.has_value() ? TripCount.value() : 1; - revng_assert(NumElems); - - // Here we are computing the larger size that is known to be - // accessed. So if we have an array, we consider it to be one - // element shorter than expected, and we add ChildSize only once - // at the end. - // This is equivalent to: - // ChildSize = (NumElems * StrideSize) - (StrideSize - ChildSize); - OrdChild.Size = ((NumElems - 1) * StrideSize) + OrdChild.Size; - } - } break; - - case TypeLinkTag::LK_Inheritance: { - revng_assert(not InheritsFromOther); - InheritsFromOther = true; - } break; - - default: - revng_unreachable("unexpected edge tag"); - } - - revng_assert(OrdChild.Offset >= 0LL and OrdChild.Size > 0ULL); - Children.push_back(std::move(OrdChild)); + Children.push_back(OrderedChild{ + .ChildIt = NChildIt, + .FieldSize = getFieldSize(NChildIt->first, NChildIt->second), + }); } // If there are no children, there's nothing to do. There might be some @@ -130,7 +115,7 @@ bool ComputeNonInterferingComponents::runOnTypeSystem(LayoutTypeSystem &TS) { continue; } - // Sort the children. Thanks to the ordering of std::tuple, children at + // Sort the children. Thanks to the ordering of OrderedChild, children at // lower offsets will be sorted before children with higher offsets, and // for children at the same offset, the smaller will be sorted before the // larger ones. @@ -158,8 +143,8 @@ bool ComputeNonInterferingComponents::runOnTypeSystem(LayoutTypeSystem &TS) { // Helper lambda to create a new component starting from the iterator to // a children that becomes the first element of the component. const auto MakeNewComponentFromChild = [](OrderedChildIt ChildIt) { - auto ChildBeginByte = ChildIt->Offset; - auto ChildEndByte = ChildBeginByte + ChildIt->Size; + const auto &[ChildBeginByte, + ChildEndByte] = ChildIt->getBeginEndByte(); return Component{ /* .StartChildIt */ ChildIt, /* .EndChildIt */ std::next(ChildIt), @@ -183,7 +168,9 @@ bool ComputeNonInterferingComponents::runOnTypeSystem(LayoutTypeSystem &TS) { auto CompStartByte = static_cast(CurrComp.StartByte); revng_assert(CompStartByte < CurrComp.EndByte); - const auto &[ChildStartByte, ChildSize, _] = *ChildIt; + const auto &[ChildStartByte, + ChildEndByte] = ChildIt->getBeginEndByte(); + auto ChildSize = ChildEndByte - ChildStartByte; revng_assert(ChildStartByte >= 0 and ChildSize > 0); auto ChildBeginByte = static_cast(ChildStartByte); @@ -208,7 +195,11 @@ bool ComputeNonInterferingComponents::runOnTypeSystem(LayoutTypeSystem &TS) { // If we have less than two components there's nothing to do. if (Components.size() < 2) { - N->InterferingInfo = AllChildrenAreInterfering; + revng_assert(not Components.empty()); + if (Components.back().NumChildren > 1) + N->InterferingInfo = AllChildrenAreInterfering; + else + N->InterferingInfo = AllChildrenAreNonInterfering; continue; } @@ -242,12 +233,10 @@ bool ComputeNonInterferingComponents::runOnTypeSystem(LayoutTypeSystem &TS) { using llvm::iterator_range; auto OrderedChildRange = iterator_range(C.StartChildIt, C.EndChildIt); for (auto &OrderedChild : OrderedChildRange) - TS.moveEdges(N, New, OrderedChild.Child, -C.StartByte); + TS.moveEdge(N, New, OrderedChild.ChildIt, -C.StartByte); // Add a link between N and the New node representing the component. // The component is at offset C.StartByte inside N. - // If this offset is zero we add an inheritance edge, otherwise an - // instance edge. TS.addInstanceLink(N, New, OffsetExpression(C.StartByte)); }