First pass at removing redundant COPYs

Ghidra/Features/Decompiler/src/decompile/cpp/block.cc

@@ -354,6 +354,21 @@ int4 FlowBlock::calcDepth(const FlowBlock *leaf) const
   return depth;
 }
 
+/// Return \b true if \b this block \e dominates the given block (or is equal to it).
+/// This assumes that block indices have been set with a reverse post order so that having a
+/// smaller index is a necessary condition for dominance.
+/// \param subBlock is the given block to test against \b this for dominance
+/// \return \b true if \b this dominates
+bool FlowBlock::dominates(const FlowBlock *subBlock) const
+
+{
+  while(subBlock != (const FlowBlock *)0 && index <= subBlock->index) {
+    if (subBlock == this) return true;
+    subBlock = subBlock->getImmedDom();
+  }
+  return false;
+}
+
 /// \return \b true if \b this is the top of a loop
 bool FlowBlock::hasLoopIn(void) const
 

Ghidra/Features/Decompiler/src/decompile/cpp/block.hh

@@ -214,6 +214,7 @@ public:
   const FlowBlock *getFrontLeaf(void) const;				///< Get the first leaf FlowBlock
   FlowBlock *getFrontLeaf(void);					///< Get the first leaf FlowBlock
   int4 calcDepth(const FlowBlock *leaf) const;		///< Get the depth of the given component FlowBlock
+  bool dominates(const FlowBlock *subBlock) const;	///< Does \b this block dominate the given block
   int4 sizeOut(void) const { return outofthis.size(); }	///< Get the number of out edges
   int4 sizeIn(void) const { return intothis.size(); }	///< Get the number of in edges
   bool hasLoopIn(void) const;				///< Is there a looping edge coming into \b this block

Ghidra/Features/Decompiler/src/decompile/cpp/coreaction.cc

@@ -3589,14 +3589,24 @@ int4 ActionCopyMarker::apply(Funcdata &data)
     op = *iter;
     switch(op->code()) {
     case CPUI_COPY:
-      if (op->getOut()->getHigh() == op->getIn(0)->getHigh()) {
+      v1 = op->getOut();
-	data.opSetFlag(op,PcodeOp::nonprinting);
+      h1 = v1->getHigh();
+      if (h1 == op->getIn(0)->getHigh()) {
+	data.opSetFlag(op, PcodeOp::nonprinting);
 	count += 1;
       }
-      else if (op->getOut()->hasNoDescend()) {	// Don't print shadow assignments
+      else {	// COPY between different HighVariables
-	if (shadowedVarnode(op->getOut())) {
+	if (h1->hasCopyIn()) {		// If we've seen other COPYs into this high
-	  data.opSetFlag(op,PcodeOp::nonprinting);
+	  if (!h1->isCopyProcessed())	// and we haven't searched before,
-	  count += 1;
+	    data.getMerge().markRedundantCopies(h1);	// search for redundant COPYs
+	}
+	else
+	  h1->setCopyIn();
+	if (v1->hasNoDescend()) {	// Don't print shadow assignments
+	  if (shadowedVarnode(v1)) {
+	    data.opSetFlag(op, PcodeOp::nonprinting);
+	    count += 1;
+	  }
 	}
       }
       break;

Ghidra/Features/Decompiler/src/decompile/cpp/merge.cc

@@ -857,6 +857,27 @@ void Merge::findSingleCopy(HighVariable *high,vector<Varnode *> &singlelist)
   }
 }
 
+/// \brief Compare COPY ops first by Varnode input, then by block containing the op
+///
+/// A sort designed to group COPY ops from the same Varnode together. Then within a group,
+/// COPYs are sorted by their containing basic block (so that dominating ops come first).
+/// \param op1 is the first PcodeOp being compared
+/// \param op2 is the second PcodeOp being compared
+/// \return \b true if the first PcodeOp should be ordered before the second
+bool Merge::compareCopyByInVarnode(PcodeOp *op1,PcodeOp *op2)
+
+{
+  Varnode *inVn1 = op1->getIn(0);
+  Varnode *inVn2 = op2->getIn(0);
+  if (inVn1 != inVn2)		// First compare by Varnode inputs
+    return (inVn1->getCreateIndex() < inVn2->getCreateIndex());
+  int4 index1 = op1->getParent()->getIndex();
+  int4 index2 = op2->getParent()->getIndex();
+  if (index1 != index2)
+    return (index1 < index2);
+  return (op1->getSeqNum().getOrder() < op2->getSeqNum().getOrder());
+}
+
 /// \brief Hide \e shadow Varnodes related to the given HighVariable by consolidating COPY chains
 ///
 /// If two Varnodes are copied from the same common ancestor then they will always contain the
@@ -900,6 +921,108 @@ bool Merge::hideShadows(HighVariable *high)
   return res;
 }
 
+/// \brief Check if the given PcodeOp COPYs are redundant
+///
+/// Both the given COPYs assign to the same HighVariable. One is redundant if there is no other
+/// assignment to the HighVariable between the first COPY and the second COPY.
+/// The first COPY must come from a block with a smaller or equal index to the second COPY.
+/// If the indices are equal, the first COPY must come before the second within the block.
+/// \param high is the HighVariable being assigned to
+/// \param domOp is the first COPY
+/// \param subOp is the second COPY
+/// \return \b true if the second COPY is redundant
+bool Merge::checkCopyPair(HighVariable *high,PcodeOp *domOp,PcodeOp *subOp)
+
+{
+  FlowBlock *domBlock = domOp->getParent();
+  FlowBlock *subBlock = subOp->getParent();
+  if (!domBlock->dominates(subBlock))
+    return false;
+  int4 domId = domBlock->getIndex();
+  int4 subId = subBlock->getIndex();
+  for(int4 i=0;i<high->numInstances();++i) {
+    Varnode *vn = high->getInstance(i);
+    if (!vn->isWritten()) continue;
+    PcodeOp *op = vn->getDef();
+    if (op == domOp) continue;
+    if (op == subOp) continue;
+    int4 index = op->getParent()->getIndex();
+    if (index == domId) {	// Assignment same block as domOp
+      if (op->getSeqNum().getOrder() > domOp->getSeqNum().getOrder())
+	return false;
+    }
+    if (index == subId) {	// Assignment same block as subOp
+      if (op->getSeqNum().getOrder() < subOp->getSeqNum().getOrder())
+	return false;
+    }
+  }
+  // All cover blocks in between domOp and subOp must be empty
+  if (subBlock != domBlock) {
+    high->updateCover();
+    subBlock = subBlock->getImmedDom();	// Don't need to check first block
+    while(subBlock != domBlock) {	// Don't need to check last block
+      if (!high->wholecover.getCoverBlock(subBlock->getIndex()).empty())
+	return false;
+      subBlock = subBlock->getImmedDom();
+    }
+  }
+  return true;
+}
+
+/// \brief Search for and mark redundant COPY ops into the given high as \e non-printing
+///
+/// Trimming during the merge process can insert multiple COPYs from the same source. In some
+/// cases, one or more COPYs may be redundant and shouldn't be printed. This method searches
+/// for redundancy among COPY ops that assign to the given HighVariable.
+/// \param high is the given HighVariable
+/// \return \b true if any redundant COPYs were discovered
+bool Merge::markRedundantCopies(HighVariable *high)
+
+{
+  bool hasRedundant = false;
+  vector<PcodeOp *> copyIns;
+
+  high->setCopyProcessed();
+  // Find all the COPY ops into this HighVariable from a different HighVariable
+  for(int4 i=0;i<high->numInstances();++i) {
+    Varnode *vn = high->getInstance(i);
+    if (!vn->isWritten()) continue;
+    PcodeOp *op = vn->getDef();
+    if (op->code() != CPUI_COPY) continue;
+    if (op->getIn(0)->getHigh() != high)
+      copyIns.push_back(op);
+  }
+
+  // Group COPYs based on the incoming Varnode
+  sort(copyIns.begin(),copyIns.end(),compareCopyByInVarnode);
+  int4 pos = 0;
+  while(pos < copyIns.size()) {
+    // Find a group of COPYs coming from the same Varnode
+    Varnode *inVn = copyIns[pos]->getIn(0);
+    int4 sz = 1;
+    while(pos + sz < copyIns.size()) {
+      Varnode *nextVn = copyIns[pos+sz]->getIn(0);
+      if (nextVn != inVn) break;
+      sz += 1;
+    }
+    if (sz > 1) {	// If there is more than one COPY in a group
+      for(int4 i=sz-1;i>0;--i) {
+	PcodeOp *subOp = copyIns[pos+i];
+	for(int4 j=i-1;j>=0;--j) {
+	  // Make sure earlier index provides dominant op
+	  if (checkCopyPair(high,copyIns[pos+j],subOp)) {
+	    data.opSetFlag(subOp, PcodeOp::nonprinting);
+	    hasRedundant = true;
+	    break;
+	  }
+	}
+      }
+    }
+    pos += sz;
+  }
+  return hasRedundant;
+}
+
 /// \brief Perform low-level details of merging two HighVariables if possible
 ///
 /// This routine only fails (returning \b false) if there is a Cover intersection between

Ghidra/Features/Decompiler/src/decompile/cpp/merge.hh

@@ -89,6 +89,7 @@ class Merge {
   static bool mergeTestBasic(Varnode *vn);
   static void findSingleCopy(HighVariable *high,vector<Varnode *> &singlelist);
   static bool compareHighByBlock(const HighVariable *a,const HighVariable *b);
+  static bool compareCopyByInVarnode(PcodeOp *op1,PcodeOp *op2);
   void collectCovering(vector<Varnode *> &vlist,HighVariable *high,PcodeOp *op);
   bool collectCorrectable(const vector<Varnode *> &vlist,list<PcodeOp *> &oplist,vector<int4> &slotlist,
 			   PcodeOp *op);
@@ -103,6 +104,7 @@ class Merge {
   void mergeIndirect(PcodeOp *indop);
   void mergeLinear(vector<HighVariable *> &highvec);
   bool merge(HighVariable *high1,HighVariable *high2,bool isspeculative);
+  bool checkCopyPair(HighVariable *high,PcodeOp *domOp,PcodeOp *subOp);
 public:
   Merge(Funcdata &fd) : data(fd) {} ///< Construct given a specific function
   bool intersection(HighVariable *a,HighVariable *b);
@@ -116,6 +118,7 @@ public:
   void mergeMarker(void);
   void mergeAdjacent(void);
   bool hideShadows(HighVariable *high);
+  bool markRedundantCopies(HighVariable *high);
 };
 
 /// \brief Compare HighVariables by the blocks they cover

Ghidra/Features/Decompiler/src/decompile/cpp/variable.hh

@@ -45,7 +45,9 @@ public:
   enum {
     flagsdirty = 1,		///< Boolean properties for the HighVariable are dirty
     typedirty = 2,		///< The data-type for the HighVariable is dirty
-    coverdirty = 4		///< The cover for the HighVariable is dirty
+    coverdirty = 4,		///< The cover for the HighVariable is dirty
+    copy_in = 8,		///< There exist COPY ops into \b this HighVariable from other HighVariables
+    copy_processed = 16		///< COPY ops into \b this HighVariable have been analyzed
   };
 private:
   friend class Merge;
@@ -61,6 +63,7 @@ private:
   void updateFlags(void) const;		///< (Re)derive boolean properties of \b this from the member Varnodes
   void updateCover(void) const;		///< (Re)derive the cover of \b this from the member Varnodes
   void updateType(void) const;		///< (Re)derive the data-type for \b this from the member Varnodes
+  void setCopyProcessed(void) const { highflags |= copy_processed; }	///< Mark that \b this has had its COPY ins processed
 public:
   HighVariable(Varnode *vn);		///< Construct a HighVariable with a single member Varnode
   Datatype *getType(void) const { updateType(); return type; }	///< Get the data-type
@@ -108,6 +111,9 @@ public:
   void setMark(void) const { flags |= Varnode::mark; }		///< Set the mark on this variable
   void clearMark(void) const { flags &= ~Varnode::mark; }	///< Clear the mark on this variable
   bool isMark(void) const { return ((flags&Varnode::mark)!=0); }	///< Return \b true if \b this is marked
+  void setCopyIn(void) const { highflags |= copy_in; }	///< Mark the existence of COPY ops into \b this
+  bool hasCopyIn(void) const { return ((highflags&copy_in)!=0); }	///< Are there COPY ops into \b this
+  bool isCopyProcessed(void) const { return ((highflags&copy_processed)!=0); }	///< Have COPY ops into \b this been processed
 
   /// \brief Determine if \b this HighVariable has an associated cover.
   ///