some crucial fixes for dominant COPY model

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

@@ -728,25 +728,28 @@ FlowBlock *FlowBlock::findCommonBlock(const vector<FlowBlock *> &blockSet)
 
 {
   vector<FlowBlock *> markedSet;
+  FlowBlock *bl;
   FlowBlock *res = blockSet[0];
   int4 bestIndex = res->getIndex();
-  res->setMark();
+  bl = res;
-  markedSet.push_back(res);
+  do {
-  for(int4 i=0;i<blockSet.size();++i) {
+    bl->setMark();
+    markedSet.push_back(bl);
+    bl = bl->getImmedDom();
+  } while (bl != (FlowBlock *)0);
+  for(int4 i=1;i<blockSet.size();++i) {
     if (bestIndex == 0)
       break;
-    FlowBlock *bl = blockSet[i];
+    bl = blockSet[i];
-    while(bl->getIndex() > bestIndex) {
+    while(!bl->isMark()) {
-      if (bl->isMark()) break;
       bl->setMark();
       markedSet.push_back(bl);
       bl = bl->getImmedDom();
     }
-    if (bl->isMark()) continue;
+    if (bl->getIndex() < bestIndex) {	// If first meeting with old paths is higher than ever before
-    res = bl;
+      res = bl;				// we have a new best
-    bestIndex = res->getIndex();
+      bestIndex = res->getIndex();
-    res->setMark();
+    }
-    markedSet.push_back(res);
   }
   for(int4 i=0;i<markedSet.size();++i)
     markedSet[i]->clearMark();

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

@@ -3579,7 +3579,6 @@ bool ActionCopyMarker::shadowedVarnode(const Varnode *vn)
 int4 ActionCopyMarker::apply(Funcdata &data)
 
 {
-  vector<HighVariable *> multiCopy;
   list<PcodeOp *>::const_iterator iter;
   PcodeOp *op;
   HighVariable *h1,*h2,*h3;
@@ -3597,14 +3596,6 @@ int4 ActionCopyMarker::apply(Funcdata &data)
 	count += 1;
       }
       else {	// COPY between different HighVariables
-	if (h1->hasCopyIn()) {		// If we've seen other COPYs into this high
-	  if (!h1->isCopyProcessed()) {	// and we haven't searched before,
-	    multiCopy.push_back(h1);
-	    h1->setCopyProcessed();
-	  }
-	}
-	else
-	  h1->setCopyIn();
 	if (v1->hasNoDescend()) {	// Don't print shadow assignments
 	  if (shadowedVarnode(v1)) {
 	    data.opSetFlag(op, PcodeOp::nonprinting);
@@ -3644,8 +3635,6 @@ int4 ActionCopyMarker::apply(Funcdata &data)
       break;
     }
   }
-  for(int4 i=0;i<multiCopy.size();++i)
-    data.getMerge().processCopyTrims(multiCopy[i]);
   return 0;
 }
 

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

@@ -272,6 +272,7 @@ void Merge::mergeOpcode(OpCode opc)
       }
     }
   }
+  processCopyTrims();	// TODO: move this into its own action
 }
 
 /// \brief Try to merge all HighVariables in the given range that have the same data-type
@@ -325,6 +326,26 @@ void Merge::mergeByDatatype(VarnodeLocSet::const_iterator startiter,VarnodeLocSe
   }
 }
 
+/// \brief Allocate COPY PcodeOp designed to trim an overextended Cover
+///
+/// A COPY is allocated with the given input and data-type.  A \e unique space
+/// output is created.
+/// \param inVn is the given input Varnode for the new COPY
+/// \param ct is the data-type to assign to the new unique output
+/// \param addr is the address associated with the new COPY
+/// \return the newly allocated COPY
+PcodeOp *Merge::allocateCopyTrim(Varnode *inVn,Datatype *ct,const Address &addr)
+
+{
+  PcodeOp *copyOp = data.newOp(1,addr);
+  data.opSetOpcode(copyOp,CPUI_COPY);
+  Varnode *outVn = data.newUnique(inVn->getSize(),ct);
+  data.opSetOutput(copyOp,outVn);
+  data.opSetInput(copyOp,inVn,0);
+  copyTrims.push_back(copyOp);
+  return copyOp;
+}
+
 /// \brief Snip off set of \e read p-code ops for a given Varnode
 ///
 /// The data-flow for the given Varnode is truncated by creating a COPY p-code from the Varnode
@@ -337,7 +358,6 @@ void Merge::snipReads(Varnode *vn,list<PcodeOp *> &markedop)
 {
   if (markedop.empty()) return;
 
-  Varnode *uniq;
   PcodeOp *copyop,*op;
   BlockBasic *bl;
   Address pc;
@@ -359,11 +379,7 @@ void Merge::snipReads(Varnode *vn,list<PcodeOp *> &markedop)
     else
       afterop = vn->getDef();
   }
-  copyop = data.newOp(1,pc);
+  copyop = allocateCopyTrim(vn, vn->getType(), pc);
-  data.opSetOpcode(copyop,CPUI_COPY);
-  uniq = data.newUnique(vn->getSize(),vn->getType());
-  data.opSetOutput(copyop,uniq);
-  data.opSetInput(copyop,vn,0);
   if (afterop == (PcodeOp *)0)
     data.opInsertBegin(copyop,bl);
   else
@@ -374,7 +390,7 @@ void Merge::snipReads(Varnode *vn,list<PcodeOp *> &markedop)
     op = *iter;
     for(slot=0;slot<op->numInput();++slot)
       if (op->getIn(slot)==vn) break; // Find the correct slot
-    data.opSetInput(op,uniq,slot);
+    data.opSetInput(op,copyop->getOut(),slot);
   }
 }
 
@@ -553,7 +569,7 @@ void Merge::trimOpInput(PcodeOp *op,int4 slot)
 
 {
   PcodeOp *copyop;
-  Varnode *uniq,*vn;
+  Varnode *vn;
   Address pc;
   
   if (op->code() == CPUI_MULTIEQUAL) {
@@ -563,12 +579,8 @@ void Merge::trimOpInput(PcodeOp *op,int4 slot)
   else
     pc = op->getAddr();
   vn = op->getIn(slot);
-  copyop = data.newOp(1,pc);
+  copyop = allocateCopyTrim(vn, vn->getType(), pc);
-  data.opSetOpcode(copyop,CPUI_COPY);
+  data.opSetInput(op,copyop->getOut(),slot);
-  uniq = data.newUnique(vn->getSize(),vn->getType());
-  data.opSetOutput(copyop,uniq);
-  data.opSetInput(copyop,vn,0);
-  data.opSetInput(op,uniq,slot);
   if (op->code() == CPUI_MULTIEQUAL)
     data.opInsertEnd(copyop,(BlockBasic *)op->getParent()->getIn(slot));
   else
@@ -717,25 +729,20 @@ void Merge::snipIndirect(PcodeOp *indop)
 
   if (correctable.empty()) return;
   Varnode *refvn = correctable.front()->getIn(correctslot[0]);
-  Varnode *snipvn;
   PcodeOp *snipop,*insertop;
 
 				// NOTE: the covers for any input to op which is
 				// an instance of the output high must
 				// all intersect so the varnodes must all be
 				// traceable via COPY to the same root
-  snipop = data.newOp(1,op->getAddr());
+  snipop = allocateCopyTrim(refvn, refvn->getType(), op->getAddr());
-  data.opSetOpcode(snipop,CPUI_COPY);
-  snipvn = data.newUnique(refvn->getSize(),refvn->getType());
-  data.opSetOutput(snipop,snipvn);
-  data.opSetInput(snipop,refvn,0);
   data.opInsertBefore(snipop,op);
   list<PcodeOp *>::iterator oiter;
   int4 i,slot;
   for(oiter=correctable.begin(),i=0;i<correctslot.size();++oiter,++i) {
     insertop = *oiter;
     slot = correctslot[i];
-    data.opSetInput(insertop,snipvn,slot);
+    data.opSetInput(insertop,snipop->getOut(),slot);
   }
 }
 
@@ -762,14 +769,9 @@ void Merge::mergeIndirect(PcodeOp *indop)
 				// the indirect effect. So fix this
 
   PcodeOp *newop;
-  Varnode *trimvn;
 
-  newop = data.newOp(1,indop->getAddr());
+  newop = allocateCopyTrim(invn0, outvn->getType(), indop->getAddr());
-  trimvn = data.newUnique(outvn->getSize(),outvn->getType());
+  data.opSetInput(indop,newop->getOut(),0);
-  data.opSetOutput(newop,trimvn);
-  data.opSetOpcode(newop,CPUI_COPY);
-  data.opSetInput(newop,indop->getIn(0),0);
-  data.opSetInput(indop,trimvn,0);
   data.opInsertBefore(newop,indop);
   if (!mergeTestRequired(outvn->getHigh(),indop->getIn(0)->getHigh()) ||
       (!merge(indop->getIn(0)->getHigh(),outvn->getHigh(),false))) // Try merge again
@@ -1065,7 +1067,7 @@ void Merge::markRedundantCopies(HighVariable *high,vector<PcodeOp *> &copy,int4
   }
 }
 
-void Merge::processCopyTrims(HighVariable *high)
+void Merge::processCopyTrimsForHigh(HighVariable *high)
 
 {
   vector<PcodeOp *> copyIns;
@@ -1076,8 +1078,9 @@ void Merge::processCopyTrims(HighVariable *high)
     if (!vn->isWritten()) continue;
     PcodeOp *op = vn->getDef();
     if (op->code() != CPUI_COPY) continue;
-    if (op->getIn(0)->getHigh() != high)
+    if (op->getIn(0)->getHigh() == high) continue;
-      copyIns.push_back(op);
+    if (op->getOut()->getSpace()->getType() != IPTR_INTERNAL) continue;
+    copyIns.push_back(op);
   }
 
   // Group COPYs based on the incoming Varnode
@@ -1100,6 +1103,27 @@ void Merge::processCopyTrims(HighVariable *high)
   }
 }
 
+void Merge::processCopyTrims(void)
+
+{
+  vector<HighVariable *> multiCopy;
+
+  for(int4 i=0;i<copyTrims.size();++i) {
+    HighVariable *high = copyTrims[i]->getOut()->getHigh();
+    if (high->hasCopyIn()) {	// If we've seen COPYs into this high before
+      if (!high->isCopyProcessed()) {	// and we haven't processed it before,
+	multiCopy.push_back(high);	// slate the high for copy trim processing
+	high->setCopyProcessed();
+      }
+    }
+    else
+      high->setCopyIn();
+  }
+  copyTrims.clear();
+  for(int4 i=0;i<multiCopy.size();++i)
+    processCopyTrimsForHigh(multiCopy[i]);
+}
+
 /// \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

@@ -80,6 +80,7 @@ class Funcdata;
 class Merge {
   Funcdata &data;		///< The function containing the Varnodes to be merged
   map<HighEdge,bool> highedgemap; ///< A cache of intersection tests, sorted by HighVariable pair
+  vector<PcodeOp *> copyTrims;	///< COPY ops inserted to facilitate merges
   bool updateHigh(HighVariable *a); ///< Make sure given HighVariable's Cover is up-to-date
   void purgeHigh(HighVariable *high); ///< Remove cached intersection tests for a given HighVariable
   bool blockIntersection(HighVariable *a,HighVariable *b,int4 blk);
@@ -93,6 +94,7 @@ class Merge {
   void collectCovering(vector<Varnode *> &vlist,HighVariable *high,PcodeOp *op);
   bool collectCorrectable(const vector<Varnode *> &vlist,list<PcodeOp *> &oplist,vector<int4> &slotlist,
 			   PcodeOp *op);
+  PcodeOp *allocateCopyTrim(Varnode *inVn,Datatype *ct,const Address &addr);
   void snipReads(Varnode *vn,list<PcodeOp *> &markedop);
   void snipIndirect(PcodeOp *indop);
   void eliminateIntersect(Varnode *vn,const vector<BlockVarnode> &blocksort);
@@ -107,6 +109,7 @@ class Merge {
   bool checkCopyPair(HighVariable *high,PcodeOp *domOp,PcodeOp *subOp);
   void buildDominantCopy(HighVariable *high,vector<PcodeOp *> &copy,int4 pos,int4 size);
   void markRedundantCopies(HighVariable *high,vector<PcodeOp *> &copy,int4 pos,int4 size);
+  void processCopyTrimsForHigh(HighVariable *high);
 public:
   Merge(Funcdata &fd) : data(fd) {} ///< Construct given a specific function
   bool intersection(HighVariable *a,HighVariable *b);
@@ -120,7 +123,7 @@ public:
   void mergeMarker(void);
   void mergeAdjacent(void);
   bool hideShadows(HighVariable *high);
-  void processCopyTrims(HighVariable *high);
+  void processCopyTrims(void);
 };
 
 /// \brief Compare HighVariables by the blocks they cover

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

@@ -63,6 +63,10 @@ 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 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
+  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
@@ -110,10 +114,6 @@ 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
-  void setCopyProcessed(void) const { highflags |= copy_processed; }	///< Mark that \b this has had its COPY ins processed
 
   /// \brief Determine if \b this HighVariable has an associated cover.
   ///