From 2b599d0ebcbcfbc72136bbe5c5e6be3897cd8978 Mon Sep 17 00:00:00 2001 From: Pietro Fezzardi Date: Fri, 30 Oct 2020 01:12:03 +0100 Subject: [PATCH] RestructureCFG: fix NDuplicates --- .../revng-c/RestructureCFGPass/GenerateAst.h | 71 +++++++------- lib/Decompiler/MarkForSerialization.cpp | 6 +- lib/RestructureCFGPass/RestructureCFG.cpp | 98 ++++++++++++++----- 3 files changed, 113 insertions(+), 62 deletions(-) diff --git a/include/revng-c/RestructureCFGPass/GenerateAst.h b/include/revng-c/RestructureCFGPass/GenerateAst.h index 22b9265ae..65811d96a 100644 --- a/include/revng-c/RestructureCFGPass/GenerateAst.h +++ b/include/revng-c/RestructureCFGPass/GenerateAst.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -340,62 +341,57 @@ createTile(RegionCFG &Graph, TileToNodeMap[Tile] = Node; } -template -inline void generateAst(RegionCFG &Region, - ASTTree &AST, - typename RegionCFG::DuplicationMap &NDuplicates, - std::map *, ASTTree> &CollapsedMap) { +// This was a function template, but it's never instantiated anywhere else with +// a template paramenter different from llvm::BasicBlock *, so we dropped the +// template for now. +// It will probably come back when we unit-test this. +// +// template +inline void +generateAst(RegionCFG &Region, + ASTTree &AST, + std::map *, ASTTree> &CollapsedMap) { // Define some using used in all the function body. + using NodeT = llvm::BasicBlock *; using BasicBlockNodeT = typename RegionCFG::BasicBlockNodeT; - using BasicBlockNodeTVect = typename RegionCFG::BasicBlockNodeTVect; // Get some fields of `RegionCFG`. std::string RegionName = Region.getRegionName(); std::string FunctionName = Region.getFunctionName(); - RegionCFG &Graph = Region; - - Graph.markUnexpectedAndAnyPCAsInlined(); + Region.markUnexpectedAndAnyPCAsInlined(); if (CombLogger.isEnabled()) { CombLogger << "Weaveing region " + RegionName + "\n"; - Graph.dumpDotOnFile("weaves", FunctionName, "PREWEAVE"); + Region.dumpDotOnFile("weaves", FunctionName, "PREWEAVE"); } // Invoke the weave function. - Graph.weave(); + Region.weave(); if (CombLogger.isEnabled()) { - Graph.dumpDotOnFile("weaves", FunctionName, "POSTWEAVE"); + Region.dumpDotOnFile("weaves", FunctionName, "POSTWEAVE"); CombLogger << "Inflating region " + RegionName + "\n"; - Graph.dumpDotOnFile("dots", FunctionName, "PRECOMB"); + Region.dumpDotOnFile("dots", FunctionName, "PRECOMB"); } - Graph.inflate(); + Region.inflate(); if (CombLogger.isEnabled()) { - Graph.dumpDotOnFile("dots", FunctionName, "POSTCOMB"); - } - - // Compute the NDuplicates, which will be used later. - // TODO: this now doesn't run multiple - for (BasicBlockNodeBB *BBNode : Graph.nodes()) { - if (BBNode->isCode()) { - llvm::BasicBlock *BB = BBNode->getOriginalNode(); - NDuplicates[BB] += 1; - } + Region.dumpDotOnFile("dots", FunctionName, "POSTCOMB"); } // TODO: factorize out the AST generation phase. llvm::DominatorTreeBase, false> ASTDT; - ASTDT.recalculate(Graph); + ASTDT.recalculate(Region); CombLogger << DoLog; std::map *, BasicBlockNode *> TileToNodeMap; + using BasicBlockNodeTVect = typename RegionCFG::BasicBlockNodeTVect; BasicBlockNodeTVect PONodes; - for (auto *N : post_order(&Graph)) + for (auto *N : post_order(&Region)) PONodes.push_back(N); unsigned Counter = 0; @@ -403,9 +399,9 @@ inline void generateAst(RegionCFG &Region, if (CombLogger.isEnabled()) { Counter++; - Graph.dumpDotOnFile("dots", - FunctionName, - "AST-" + std::to_string(Counter)); + Region.dumpDotOnFile("dots", + FunctionName, + "AST-" + std::to_string(Counter)); } // Collect the children nodes in the dominator tree. @@ -435,7 +431,8 @@ inline void generateAst(RegionCFG &Region, const auto &[It, New] = CollapsedMap.insert({ BodyGraph, ASTTree() }); ASTTree &CollapsedAST = It->second; if (New) - generateAst(*BodyGraph, CollapsedAST, NDuplicates, CollapsedMap); + generateAst(*BodyGraph, CollapsedAST, CollapsedMap); + ASTNode *Body = AST.copyASTNodesFrom(CollapsedAST); switch (Successors.size()) { @@ -449,7 +446,7 @@ inline void generateAst(RegionCFG &Region, ASTNode *ASTChild = nullptr; if (ASTDT.dominates(Node, Succ)) { ASTChild = findASTNode(AST, TileToNodeMap, Succ); - createTile(Graph, ASTDT, TileToNodeMap, Node, Succ); + createTile(Region, ASTDT, TileToNodeMap, Node, Succ); } ASTObject.reset(new ScsNode(Node, Body, ASTChild)); } break; @@ -464,7 +461,7 @@ inline void generateAst(RegionCFG &Region, llvm::Value *SwitchCondition = nullptr; if (not Node->isDispatcher()) { - llvm::BasicBlock *OriginalNode = Node->getOriginalNode(); + NodeT OriginalNode = Node->getOriginalNode(); llvm::Instruction *Terminator = OriginalNode->getTerminator(); llvm::SwitchInst *Switch = llvm::cast(Terminator); SwitchCondition = Switch->getCondition(); @@ -541,7 +538,7 @@ inline void generateAst(RegionCFG &Region, } } - createTile(Graph, ASTDT, TileToNodeMap, Node, PostDomBB); + createTile(Region, ASTDT, TileToNodeMap, Node, PostDomBB); ASTObject.reset(new SwitchNode(Node, SwitchCondition, @@ -614,7 +611,7 @@ inline void generateAst(RegionCFG &Region, if (DominatedSucc == Successor2) Else = findASTNode(AST, TileToNodeMap, Successor2); } - createTile(Graph, ASTDT, TileToNodeMap, Node, NotDominatedSucc); + createTile(Region, ASTDT, TileToNodeMap, Node, NotDominatedSucc); // Build the `IfNode`. using UniqueExpr = ASTTree::expr_unique_ptr; @@ -695,7 +692,7 @@ inline void generateAst(RegionCFG &Region, ASTObject.reset(new IfNode(Node, Condition, Then, Else, PostDom)); - createTile(Graph, ASTDT, TileToNodeMap, Node, PostDomBB); + createTile(Region, ASTDT, TileToNodeMap, Node, PostDomBB); } break; case 3: { @@ -737,7 +734,7 @@ inline void generateAst(RegionCFG &Region, ExprNode *Condition = AST.addCondExpr(std::move(CondExpr)); ASTObject.reset(new IfNode(Node, Condition, Then, Else, PostDom)); - createTile(Graph, ASTDT, TileToNodeMap, Node, PostDomBB); + createTile(Region, ASTDT, TileToNodeMap, Node, PostDomBB); } break; default: { @@ -778,7 +775,7 @@ inline void generateAst(RegionCFG &Region, } else { ASTObject.reset(new CodeNode(Node, Succ)); } - createTile(Graph, ASTDT, TileToNodeMap, Node, Children[0]); + createTile(Region, ASTDT, TileToNodeMap, Node, Children[0]); } break; default: { diff --git a/lib/Decompiler/MarkForSerialization.cpp b/lib/Decompiler/MarkForSerialization.cpp index 5518ed4a6..f4a645b51 100644 --- a/lib/Decompiler/MarkForSerialization.cpp +++ b/lib/Decompiler/MarkForSerialization.cpp @@ -102,8 +102,10 @@ Analysis::InterruptType Analysis::transfer(BasicBlock *BB) { continue; } Instruction *UserI = cast(U); - if (NBBDuplicates != NDuplicates.at(UserI->getParent())) { - // revng_assert(NBBDuplicates < NDuplicates.at(UserI->getParent())); + auto *UserBB = UserI->getParent(); + auto NUserDuplicates = NDuplicates.at(UserBB); + revng_assert(isa(UserI) or NBBDuplicates <= NUserDuplicates); + if (NBBDuplicates < NUserDuplicates) { markValueToSerialize(&I); } else { Pending.insert(&I); diff --git a/lib/RestructureCFGPass/RestructureCFG.cpp b/lib/RestructureCFGPass/RestructureCFG.cpp index 7c8c8f8a3..8ad1cee37 100644 --- a/lib/RestructureCFGPass/RestructureCFG.cpp +++ b/lib/RestructureCFGPass/RestructureCFG.cpp @@ -11,6 +11,7 @@ // LLVM includes #include +#include #include #include #include @@ -23,6 +24,7 @@ #include // revng-c includes +#include "revng-c/RestructureCFGPass/BasicBlockNodeImpl.h" #include "revng-c/TargetFunctionOption/TargetFunctionOption.h" // Local libraries includes @@ -393,10 +395,25 @@ static RegisterPass X("restructure-cfg", true, true); -static cl::opt OutputPath("restructure-metrics-output-dir", - desc("Restructure metrics dir"), - value_desc("restructure-dir"), - cat(MainCategory)); +static cl::opt MetricsOutputPath("restructure-metrics-output-dir", + desc("Restructure metrics dir"), + value_desc("restructure-dir"), + cat(MainCategory)); + +inline void +accumulateDuplicates(RegionCFG &Region, + std::map &NDuplicates) { + // Compute the NDuplicates, which will be used later. + for (BasicBlockNodeBB *BBNode : Region.nodes()) { + if (BBNode->isCode()) { + auto *BB = BBNode->getOriginalNode(); + ++NDuplicates[BB]; + } else if (BBNode->isCollapsed()) { + auto *BodyGraph = BBNode->getCollapsedCFG(); + accumulateDuplicates(*BodyGraph, NDuplicates); + } + } +} bool RestructureCFG::runOnFunction(Function &F) { NDuplicates.clear(); @@ -1145,15 +1162,21 @@ bool RestructureCFG::runOnFunction(Function &F) { // Check that the root region is acyclic at this point. revng_assert(RootCFG.isDAG()); - // Compute the initial weight of the CFG. + // Collect statistics unsigned InitialWeight = 0; - for (BasicBlockNodeBB *BBNode : RootCFG.nodes()) { - InitialWeight += BBNode->getWeight(); + if (MetricsOutputPath.getNumOccurrences()) { + revng_assert(MetricsOutputPath.getNumOccurrences() == 1); + // Compute the initial weight of the CFG. + for (BasicBlockNodeBB *BBNode : RootCFG.nodes()) { + InitialWeight += BBNode->getWeight(); + } } // Invoke the AST generation for the root region. - std::map *, ASTTree> CollapsedMap; - generateAst(RootCFG, AST, NDuplicates, CollapsedMap); + std::map *, ASTTree> CollapsedMap; + generateAst(RootCFG, AST, CollapsedMap); + + accumulateDuplicates(RootCFG, NDuplicates); // Scorporated this part which was previously inside the `generateAst` to // avoid having it run twice or more (it was run inside the recursive step @@ -1162,25 +1185,54 @@ bool RestructureCFG::runOnFunction(Function &F) { normalize(AST, F.getName()); // Serialize final AST on file - if (CombLogger.isEnabled()) { + if (CombLogger.isEnabled()) AST.dumpOnFile("ast", F.getName(), "Final"); - } - - // Collect the number of cloned nodes introduced by the comb for a single - // `llvm::BasicBlock`, information which is needed later in the - // `MarkForSerialization` pass. - // - // Collect also the final weight of the CFG. - unsigned FinalWeight = 0; - - // Compute the increase in weight. - float Increase = float(FinalWeight) / float(InitialWeight); // Serialize the collected metrics in the outputfile. - if (OutputPath.getNumOccurrences() == 1) { + if (MetricsOutputPath.getNumOccurrences()) { + // Compute the increase in weight, on the AST + unsigned FinalWeight = 0; + for (ASTNode *N : AST.nodes()) { + switch (N->getKind()) { + case ASTNode::NK_Scs: + case ASTNode::NK_If: + case ASTNode::NK_Switch: { + // Control-flow nodes emit single constructs, so we just increase the + // weight by one. + // Control-flow nodes would also have nested scopes (then-else for if, + // cases for switch, loop body for scs). However, those nodes are visted + // separately, and will be accounted for later. + ++FinalWeight; + } break; + case ASTNode::NK_Set: + case ASTNode::NK_Break: + case ASTNode::NK_SwitchBreak: + case ASTNode::NK_Continue: { + // These AST Nodes are emitted as single instructions. + // Just increase the weight by one. + ++FinalWeight; + } break; + case ASTNode::NK_List: { + // Sequence nodes are just scopes, they don't have a real weight. + // Their weight is just sum of the weights of the nodes they contain, + // that will be visited nevertheless. + } break; + case ASTNode::NK_Code: { + auto *BB = cast(N)->getOriginalBB(); + revng_assert(BB); + FinalWeight += WeightTraits::getWeight(BB); + } break; + default: + revng_abort("unxpected AST node"); + } + } + + float Increase = float(FinalWeight) / float(InitialWeight); + std::ofstream Output; const char *FunctionName = F.getName().data(); - std::ostream &OutputStream = pathToStream(OutputPath + "/" + FunctionName, + std::ostream &OutputStream = pathToStream(MetricsOutputPath + "/" + + FunctionName, Output); OutputStream << "function," "duplications,percentage,tuntangle,puntangle,iweight\n";