Merge remote-tracking branch 'origin/GP-4715_RemoveRevisitedFix' into

patch (Closes #6488)

Ghidra/Features/Decompiler/certification.manifest

@@ -66,6 +66,7 @@ src/decompile/datatests/ptrtoarray.xml||GHIDRA||||END|
 src/decompile/datatests/readvolatile.xml||GHIDRA||||END|
 src/decompile/datatests/retspecial.xml||GHIDRA||||END|
 src/decompile/datatests/retstruct.xml||GHIDRA||||END|
+src/decompile/datatests/revisit.xml||GHIDRA||||END|
 src/decompile/datatests/sbyte.xml||GHIDRA||||END|
 src/decompile/datatests/skipnext2.xml||GHIDRA||||END|
 src/decompile/datatests/stackreturn.xml||GHIDRA||||END|

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

@@ -1403,7 +1403,7 @@ void SplitVarnode::replaceCopyForce(Funcdata &data,const Address &addr,SplitVarn
 
 {
   Varnode *inVn = in.getWhole();
-  bool returnForm = copyhi->stopsCopyPropagation();
+  bool returnForm = copyhi->isReturnCopy();
   if (returnForm && inVn->getAddr() != addr) {
     // Placeholder for global propagation past a RETURN needs an additional COPY
     PcodeOp *otherPoint1 = copyhi->getIn(0)->getDef();
@@ -1423,7 +1423,7 @@ void SplitVarnode::replaceCopyForce(Funcdata &data,const Address &addr,SplitVarn
   Varnode *outVn = data.newVarnodeOut(in.getSize(), addr, wholeCopy);
   outVn->setAddrForce();
   if (returnForm)
-    wholeCopy->setStopCopyPropagation();
+    data.markReturnCopy(wholeCopy);
   data.opSetInput(wholeCopy,inVn,0);
   data.opInsertBefore(wholeCopy, copyhi);
   data.opDestroy(copyhi);	// Destroy the original COPYs.  Outputs have no descendants.
@@ -3156,7 +3156,7 @@ bool CopyForceForm::verify(Varnode *h,Varnode *l,Varnode *w,PcodeOp *cpy)
       continue;
     if (!SplitVarnode::isAddrTiedContiguous(reslo, reshi, addrOut))	// Output MUST be contiguous addresses
       continue;
-    if (copyhi->stopsCopyPropagation()) {	// Special form has additional requirements
+    if (copyhi->isReturnCopy()) {	// Special form has additional requirements
       if (h->loneDescend() == (PcodeOp *)0)
 	continue;
       if (l->loneDescend() == (PcodeOp *)0)

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

@@ -445,6 +445,7 @@ public:
   PcodeOp *newIndirectOp(PcodeOp *indeffect,const Address &addr,int4 sz,uint4 extraFlags);
   PcodeOp *newIndirectCreation(PcodeOp *indeffect,const Address &addr,int4 sz,bool possibleout);
   void markIndirectCreation(PcodeOp *indop,bool possibleOutput);	///< Convert CPUI_INDIRECT into an \e indirect \e creation
+  void markReturnCopy(PcodeOp *op) { op->flags |= PcodeOp::return_copy; }	///< Mark COPY as returning a global value
   PcodeOp *findOp(const SeqNum &sq) { return obank.findOp(sq); }	///< Find PcodeOp with given sequence number
   void opInsertBefore(PcodeOp *op,PcodeOp *follow);		///< Insert given PcodeOp before a specific op
   void opInsertAfter(PcodeOp *op,PcodeOp *prev);		///< Insert given PcodeOp after a specific op

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

@@ -397,36 +397,45 @@ void Funcdata::combineInputVarnodes(Varnode *vnHi,Varnode *vnLo)
   if (!isContiguous)
     throw LowlevelError("Input varnodes being combined are not contiguous");
   vector<PcodeOp *> pieceList;
-  bool otherOps = false;
+  bool otherOpsHi = false;
+  bool otherOpsLo = false;
   list<PcodeOp *>::const_iterator iter;
   for(iter=vnHi->beginDescend();iter!=vnHi->endDescend();++iter) {
     PcodeOp *op = *iter;
     if (op->code() == CPUI_PIECE && op->getIn(0) == vnHi && op->getIn(1) == vnLo)
       pieceList.push_back(op);
     else
-      otherOps = true;
+      otherOpsHi = true;
+  }
+  for(iter=vnLo->beginDescend();iter!=vnLo->endDescend();++iter) {
+    PcodeOp *op = *iter;
+    if (op->code() != CPUI_PIECE || op->getIn(0) != vnHi || op->getIn(1) != vnLo)
+      otherOpsLo = true;
   }
   for(int4 i=0;i<pieceList.size();++i) {
     opRemoveInput(pieceList[i], 1);
     opUnsetInput(pieceList[i], 0);
   }
+  // If there are other PcodeOps besides PIECEs that are directly combining vnHi and vnLo
+  // create replacement Varnodes constructed as SUBPIECEs of the new combined Varnode
   PcodeOp *subHi = (PcodeOp *)0;
   PcodeOp *subLo = (PcodeOp *)0;
-  if (otherOps) {
+  if (otherOpsHi) {
-    // If there are other PcodeOps besides PIECEs that are directly combining vnHi and vnLo
-    // create replacement Varnodes constructed as SUBPIECEs of the new combined Varnode
     BlockBasic *bb = (BlockBasic *)bblocks.getBlock(0);
     subHi = newOp(2,bb->getStart());
     opSetOpcode(subHi, CPUI_SUBPIECE);
     opSetInput(subHi,newConstant(4, vnLo->getSize()),1);
     Varnode *newHi = newVarnodeOut(vnHi->getSize(),vnHi->getAddr(),subHi);
     opInsertBegin(subHi, bb);
+    totalReplace(vnHi, newHi);
+  }
+  if (otherOpsLo) {
+    BlockBasic *bb = (BlockBasic *)bblocks.getBlock(0);
     subLo = newOp(2,bb->getStart());
     opSetOpcode(subLo, CPUI_SUBPIECE);
     opSetInput(subLo,newConstant(4, 0),1);
     Varnode *newLo = newVarnodeOut(vnLo->getSize(),vnLo->getAddr(),subLo);
     opInsertBegin(subLo, bb);
-    totalReplace(vnHi, newHi);
     totalReplace(vnLo, newLo);
   }
   int4 outSize = vnHi->getSize() + vnLo->getSize();
@@ -438,10 +447,10 @@ void Funcdata::combineInputVarnodes(Varnode *vnHi,Varnode *vnLo)
     opSetInput(pieceList[i],inVn,0);
     opSetOpcode(pieceList[i], CPUI_COPY);
   }
-  if (otherOps) {
+  if (otherOpsHi)
     opSetInput(subHi,inVn,0);
+  if (otherOpsLo)
     opSetInput(subLo,inVn,0);
-  }
 }
 
 /// Construct a constant Varnode up to 128 bits,  using INT_ZEXT and PIECE if necessary.

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

@@ -4,9 +4,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -98,6 +98,43 @@ int4 LocationMap::findPass(const Address &addr) const
   return -1;
 }
 
+/// Addresses fed to this method must already be sorted.  If the given range intersects the last range in
+/// the list, the last range is extended to cover it.  Otherwise the range is added as a new element to the
+/// end of the list.  The given boolean properties are associated with any new element.  If an old element is
+/// extended, any new boolean properties are ORed into the old element's properties.
+/// \param addr is the starting address of the given range
+/// \param size is the number of bytes in the given range
+/// \param fl are the given boolean properties to associate with the range
+void TaskList::add(Address addr,int4 size,uint4 fl)
+
+{
+  if (!tasklist.empty()) {
+    MemRange &entry(tasklist.back());
+    int4 over = addr.overlap(0,entry.addr,entry.size);
+    if (over >= 0) {
+      int4 relsize = size + over;
+      if (relsize > entry.size)
+	entry.size = relsize;
+      entry.flags |= fl;
+      return;
+    }
+  }
+  tasklist.emplace_back(addr,size,fl);
+}
+
+/// This can be used to add a range anywhere in the list, but the new range must already be disjoint
+/// from ranges in the list.
+/// \param pos is the position in the list before which the new range will be inserted
+/// \param addr is the starting address of the new range
+/// \param size is the number of bytes in the new range
+/// \param fl is the boolean properties to be associated with the new range
+/// \return an iterator to the new range
+TaskList::iterator TaskList::insert(iterator pos,Address addr,int4 size,uint4 fl)
+
+{
+  return tasklist.emplace(pos,addr,size,fl);
+}
+
 /// Any basic blocks currently in \b this queue are removed. Space is
 /// reserved for a new set of prioritized stacks.
 /// \param maxdepth is the number of stacks to allocate
@@ -194,13 +231,13 @@ void Heritage::clearInfoList(void)
     (*iter).reset();
 }
 
-/// \brief Remove deprecated CPUI_MULTIEQUAL or CPUI_INDIRECT ops, preparing to re-heritage
+/// \brief Remove deprecated CPUI_MULTIEQUAL, CPUI_INDIRECT, or CPUI_COPY ops, preparing to re-heritage
 ///
 /// If a previous Varnode was heritaged through a MULTIEQUAL or INDIRECT op, but now
 /// a larger range containing the Varnode is being heritaged, we throw away the op,
 /// letting the data-flow for the new larger range determine the data-flow for the
 /// old Varnode.  The original Varnode is redefined as the output of a SUBPIECE
-/// of a larger free Varnode.
+/// of a larger free Varnode.  Return form COPYs are simply removed, in preparation for a larger COPY.
 /// \param remove is the list of Varnodes written by MULTIEQUAL or INDIRECT
 /// \param addr is the start of the larger range
 /// \param size is the size of the range
@@ -233,13 +270,18 @@ void Heritage::removeRevisitedMarkers(const vector<Varnode *> &remove,const Addr
       else
 	pos = targetOp->getBasicIter();
       ++pos;		// Insert SUBPIECE after target of INDIRECT
+      vn->clearAddrForce();	// Replacement INDIRECT will hold the address
     }
-    else {
+    else if (op->code() == CPUI_MULTIEQUAL) {
       pos = op->getBasicIter();	// Insert SUBPIECE after all MULTIEQUALs in block
       ++pos;
       while(pos != bl->endOp() && (*pos)->code() == CPUI_MULTIEQUAL)
 	++pos;
     }
+    else {			// Remove return form COPY
+      fd->opUnlink(op);
+      continue;
+    }
     int4 offset = vn->overlap(addr,size);
     fd->opUninsert(op);
     newInputs.clear();
@@ -256,48 +298,50 @@ void Heritage::removeRevisitedMarkers(const vector<Varnode *> &remove,const Addr
 
 /// \brief Collect free reads, writes, and inputs in the given address range
 ///
-/// \param addr is the starting address of the range
+/// \param memrange is the given address range
-/// \param size is the number of bytes in the range
 /// \param read will hold any read Varnodes in the range
 /// \param write will hold any written Varnodes
 /// \param input will hold any input Varnodes
 /// \param remove will hold any PcodeOps that need to be removed
 /// \return the maximum size of a write
-int4 Heritage::collect(Address addr,int4 size,
+int4 Heritage::collect(MemRange &memrange,vector<Varnode *> &read,vector<Varnode *> &write,
-		      vector<Varnode *> &read,vector<Varnode *> &write,
+		       vector<Varnode *> &input,vector<Varnode *> &remove) const
-		      vector<Varnode *> &input,vector<Varnode *> &remove) const
-
 {
-  Varnode *vn;
+  read.clear();
-  VarnodeLocSet::const_iterator viter = fd->beginLoc(addr);
+  write.clear();
+  input.clear();
+  remove.clear();
   VarnodeLocSet::const_iterator enditer;
-  uintb start = addr.getOffset();
+  uintb start = memrange.addr.getOffset();
-  addr = addr + size;
+  Address endaddr = memrange.addr + memrange.size;
-  if (addr.getOffset() < start) {	// Wraparound
+  if (endaddr.getOffset() < start) {	// Wraparound
-    Address tmp(addr.getSpace(),addr.getSpace()->getHighest());
+    Address tmp(endaddr.getSpace(),endaddr.getSpace()->getHighest());
     enditer = fd->endLoc(tmp);
   }
   else
-    enditer = fd->beginLoc(addr);
+    enditer = fd->beginLoc(endaddr);
   int4 maxsize = 0;
-  while( viter != enditer ) {
+  for(VarnodeLocSet::const_iterator viter = fd->beginLoc(memrange.addr); viter != enditer; ++viter) {
-    vn = *viter;
+    Varnode *vn = *viter;
     if (!vn->isWriteMask()) {
       if (vn->isWritten()) {
-	if (vn->getSize() < size && vn->getDef()->isMarker())
+	PcodeOp *op = vn->getDef();
-	  remove.push_back(vn);
+	if (op->isMarker() || op->isReturnCopy()) {	// Evidence of previous heritage in this range
-	else {
+	  if (vn->getSize() < memrange.size) {
-	  if (vn->getSize() > maxsize) // Look for maximum write size
+	    remove.push_back(vn);
-	    maxsize = vn->getSize();
+	    continue;
-	  write.push_back(vn);
+	  }
+	  memrange.clearProperty(MemRange::new_addresses);	// Previous pass covered everything
 	}
+	if (vn->getSize() > maxsize) // Look for maximum write size
+	  maxsize = vn->getSize();
+	write.push_back(vn);
       }
       else if ((!vn->isHeritageKnown())&&(!vn->hasNoDescend()))
 	read.push_back(vn);
       else if (vn->isInput())
 	input.push_back(vn);
     }
-    ++viter;
   }
   return maxsize;
 }
@@ -1101,15 +1145,15 @@ void Heritage::reprocessFreeStores(AddrSpace *spc,vector<PcodeOp *> &freeStores)
 /// The traditional phi-node placement and renaming algorithms don't expect
 /// variable pairs where there is partial overlap. For the given address range,
 /// we make all the free Varnode sizes look uniform by adding PIECE and SUBPIECE
-/// ops. We also add INDIRECT ops, so that we can ignore indirect effects
+/// ops. If enabled, we also add INDIRECT ops, so that renaming takes into account
-/// of LOAD/STORE/CALL ops.
+/// indirect effects of LOAD/STORE/CALL ops.
 /// \param addr is the starting address of the given range
 /// \param size is the number of bytes in the given range
-/// \param guardPerformed is true if a guard has been previously performed on the range
+/// \param addIndirects is true if a guard has been previously performed on the range
 /// \param read is the set of Varnode values reading from the range
 /// \param write is the set of written Varnodes in the range
 /// \param inputvars is the set of Varnodes in the range already marked as input
-void Heritage::guard(const Address &addr,int4 size,bool guardPerformed,
+void Heritage::guard(const Address &addr,int4 size,bool addIndirects,
 		     vector<Varnode *> &read,vector<Varnode *> &write,vector<Varnode *> &inputvars)
 
 {
@@ -1141,7 +1185,7 @@ void Heritage::guard(const Address &addr,int4 size,bool guardPerformed,
   // free for an address that has already been guarded before.
   // Because INDIRECTs for a single CALL or STORE really issue simultaneously, having multiple INDIRECT guards
   // for the same address confuses the renaming algorithm, so we don't add guards if we've added them before.
-  if (!guardPerformed) {
+  if (addIndirects) {
     fl = 0;
     // Query for generic properties of address (use empty usepoint)
     fd->getScopeLocal()->queryProperties(addr,size,Address(),fl);
@@ -1639,7 +1683,7 @@ void Heritage::guardReturns(uint4 fl,const Address &addr,int4 size,vector<Varnod
     vn->setAddrForce();
     vn->setActiveHeritage();
     fd->opSetOpcode(copyop,CPUI_COPY);
-    copyop->setStopCopyPropagation();
+    fd->markReturnCopy(copyop);
     Varnode *invn = fd->newVarnode(size,addr);
     invn->setActiveHeritage();
     fd->opSetInput(copyop,invn,0);
@@ -1656,9 +1700,8 @@ void Heritage::guardReturns(uint4 fl,const Address &addr,int4 size,vector<Varnod
 /// Varnode.
 /// \param refine is the refinement array
 /// \param addr is the starting address of the given range
-/// \param size is the number of bytes in the range
 /// \param vnlist is the list of Varnodes to add to the array
-void Heritage::buildRefinement(vector<int4> &refine,const Address &addr,int4 size,const vector<Varnode *> &vnlist)
+void Heritage::buildRefinement(vector<int4> &refine,const Address &addr,const vector<Varnode *> &vnlist)
 
 {
   for(uint4 i=0;i<vnlist.size();++i) {
@@ -1838,20 +1881,21 @@ void Heritage::remove13Refinement(vector<int4> &refine)
 /// \brief Find the common refinement of all reads and writes in the address range
 ///
 /// Split the reads and writes so they match the refinement.
-/// \param addr is the first address in the range
+/// \param memiter points to the address range to be refined
-/// \param size is the number of bytes in the range
 /// \param readvars is all \e free Varnodes overlapping the address range
 /// \param writevars is all written Varnodes overlapping the address range
 /// \param inputvars is all known input Varnodes overlapping the address range
 /// \return \b true if there is a non-trivial refinement
-bool Heritage::refinement(const Address &addr,int4 size,const vector<Varnode *> &readvars,const vector<Varnode *> &writevars,const vector<Varnode *> &inputvars)
+TaskList::iterator Heritage::refinement(TaskList::iterator memiter,const vector<Varnode *> &readvars,
-
+					const vector<Varnode *> &writevars,const vector<Varnode *> &inputvars)
 {
-  if (size > 1024) return false;
+  int4 size = (*memiter).size;
+  if (size > 1024) return disjoint.end();
+  Address addr = (*memiter).addr;
   vector<int4> refine(size+1,0);
-  buildRefinement(refine,addr,size,readvars);
+  buildRefinement(refine,addr,readvars);
-  buildRefinement(refine,addr,size,writevars);
+  buildRefinement(refine,addr,writevars);
-  buildRefinement(refine,addr,size,inputvars);
+  buildRefinement(refine,addr,inputvars);
   int4 lastpos = 0;
   for(int4 curpos=1;curpos < size;++curpos) { // Convert boundary points to partition sizes
     if (refine[curpos] != 0) {
@@ -1859,7 +1903,7 @@ bool Heritage::refinement(const Address &addr,int4 size,const vector<Varnode *>
       lastpos = curpos;
     }
   }
-  if (lastpos == 0) return false; // No non-trivial refinements
+  if (lastpos == 0) return disjoint.end(); // No non-trivial refinements
   refine[lastpos] = size-lastpos;
   remove13Refinement(refine);
   vector<Varnode *> newvn;
@@ -1871,22 +1915,26 @@ bool Heritage::refinement(const Address &addr,int4 size,const vector<Varnode *>
     refineInput(inputvars[i],addr,refine,newvn);
 
   // Alter the disjoint cover (both locally and globally) to reflect our refinement
-  LocationMap::iterator iter = disjoint.find(addr);
+  uint4 flags = (*memiter).flags;
-  int4 addrPass = (*iter).second.pass;
+  memiter = disjoint.erase(memiter);
-  disjoint.erase(iter);
+  LocationMap::iterator iter = globaldisjoint.find(addr);
-  iter = globaldisjoint.find(addr);
+  int4 curPass = (*iter).second.pass;
   globaldisjoint.erase(iter);
-  Address curaddr = addr;
   int4 cut = 0;
+  int4 sz = refine[cut];
   int4 intersect;
+  TaskList::iterator resiter = disjoint.insert(memiter,addr,sz,flags);
+  globaldisjoint.add(addr,sz,curPass,intersect);
+  cut += sz;
+  addr = addr + sz;
   while(cut < size) {
-    int4 sz = refine[cut];
+    sz = refine[cut];
-    disjoint.add(curaddr,sz,addrPass,intersect);
+    disjoint.insert(memiter,addr,sz,flags);
-    globaldisjoint.add(curaddr,sz,addrPass,intersect);
+    globaldisjoint.add(addr,sz,curPass,intersect);
     cut += sz;
-    curaddr = curaddr + sz;
+    addr = addr + sz;
   }
-  return true;
+  return resiter;
 }
 
 /// \brief Make sure existing inputs for the given range fill it entirely
@@ -2549,52 +2597,43 @@ void Heritage::rename(void)
 void Heritage::placeMultiequals(void)
 
 {
-  LocationMap::iterator iter;
+  TaskList::iterator iter;
   vector<Varnode *> readvars;
   vector<Varnode *> writevars;
   vector<Varnode *> inputvars;
   vector<Varnode *> removevars;
 
   for(iter=disjoint.begin();iter!=disjoint.end();++iter) { 
-    Address addr = (*iter).first;
+    int4 max = collect(*iter,readvars,writevars,inputvars,removevars); // Collect reads/writes
-    int4 size = (*iter).second.size;
+    if ((*iter).size > 4 && max < (*iter).size) {
-    bool guardPerformed = (*iter).second.pass < pass;
+      TaskList::iterator refiter = refinement(iter,readvars,writevars,inputvars);
-    readvars.clear();
+      if (refiter != disjoint.end()) {
-    writevars.clear();
+	iter = refiter;
-    inputvars.clear();
+	collect(*iter,readvars,writevars,inputvars,removevars);
-    removevars.clear();
-    int4 max = collect(addr,size,readvars,writevars,inputvars,removevars); // Collect reads/writes
-    if ((size > 4)&&(max < size)) {
-      if (refinement(addr,size,readvars,writevars,inputvars)) {
-	iter = disjoint.find(addr);
-	size =(*iter).second.size;
-	readvars.clear();
-	writevars.clear();
-	inputvars.clear();
-	removevars.clear();
-	collect(addr,size,readvars,writevars,inputvars,removevars);
       }
     }
+    const MemRange &memrange(*iter);
+    int4 size = memrange.size;
     if (readvars.empty()) {
       if (writevars.empty() && inputvars.empty()) {
 	continue;
       }
-      if (addr.getSpace()->getType() == IPTR_INTERNAL || guardPerformed)
+      if (memrange.addr.getSpace()->getType() == IPTR_INTERNAL || memrange.oldAddresses())
 	continue;
     }
     if (!removevars.empty())
-      removeRevisitedMarkers(removevars, addr, size);
+      removeRevisitedMarkers(removevars, memrange.addr, size);
-    guardInput(addr,size,inputvars);
+    guardInput(memrange.addr,size,inputvars);
-    guard(addr,size,guardPerformed,readvars,writevars,inputvars);
+    guard(memrange.addr,size,memrange.newAddresses(),readvars,writevars,inputvars);
     calcMultiequals(writevars); // Calculate where MULTIEQUALs go
     for(int4 i=0;i<merge.size();++i) {
       BlockBasic *bl = (BlockBasic *) merge[i];
       PcodeOp *multiop = fd->newOp(bl->sizeIn(),bl->getStart());
-      Varnode *vnout = fd->newVarnodeOut(size,addr,multiop);
+      Varnode *vnout = fd->newVarnodeOut(size,memrange.addr,multiop);
       vnout->setActiveHeritage();
       fd->opSetOpcode(multiop,CPUI_MULTIEQUAL); // Create each MULTIEQUAL
       for(int4 j=0;j<bl->sizeIn();++j) {
-	Varnode *vnin = fd->newVarnode(size,addr);
+	Varnode *vnin = fd->newVarnode(size,memrange.addr);
 	fd->opSetInput(multiop,vnin,j);
       }
       fd->opInsertBegin(multiop,bl);	// Insert at beginning of block
@@ -2666,7 +2705,7 @@ void Heritage::heritage(void)
       int4 prev = 0;
       LocationMap::iterator liter = globaldisjoint.add(vn->getAddr(),vn->getSize(),pass,prev);
       if (prev == 0)		// All new location being heritaged, or intersecting with something new
-	disjoint.add((*liter).first,(*liter).second.size,pass,prev);
+	disjoint.add((*liter).first,(*liter).second.size,MemRange::new_addresses);
       else if (prev==2) { // If completely contained in range from previous pass
 	if (vn->isHeritageKnown()) continue; // Don't heritage if we don't have to 
 	if (vn->hasNoDescend()) continue;
@@ -2675,10 +2714,10 @@ void Heritage::heritage(void)
 	  bumpDeadcodeDelay(vn->getSpace());
 	  warnvn = vn;
 	}
-	disjoint.add((*liter).first,(*liter).second.size,(*liter).second.pass,prev);
+	disjoint.add((*liter).first,(*liter).second.size,MemRange::old_addresses);
       }
       else {	// Partially contained in old range, but may contain new stuff
-	disjoint.add((*liter).first,(*liter).second.size,(*liter).second.pass,prev);
+	disjoint.add((*liter).first,(*liter).second.size,MemRange::old_addresses | MemRange::new_addresses);
 	if ((!needwarning)&&(info->deadremoved>0)&&!fd->isJumptableRecoveryOn()) {
 	  // TODO: We should check if this varnode is tiled by previously heritaged ranges
 	  if (vn->isHeritageKnown()) continue;		// Assume that it is tiled and produced by merging

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

@@ -5,9 +5,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -28,12 +28,6 @@ namespace ghidra {
 /// range (indexed by its initial address) maps to its own Varnode stack.
 typedef map<Address,vector<Varnode *> > VariableStack;
 
-/// \brief Label for describing extent of address range that has been heritaged
-struct SizePass {
-  int4 size;			///< Size of the range (in bytes)
-  int4 pass;			///< Pass when the range was heritaged
-};
-
 /// \brief Map object for keeping track of which address ranges have been heritaged
 ///
 /// We keep track of a fairly fine grained description of when each address range
@@ -43,6 +37,11 @@ struct SizePass {
 /// that informs the caller whether the address has been heritaged and if so in which pass.
 class LocationMap {
 public:
+  /// \brief Label for describing extent of address range that has been heritaged
+  struct SizePass {
+    int4 size;			///< Size of the range (in bytes)
+    int4 pass;			///< Pass when the range was heritaged
+  };
   /// Iterator into the main map
   typedef map<Address,SizePass>::iterator iterator;
 private:
@@ -51,12 +50,48 @@ public:
   iterator add(Address addr,int4 size,int4 pass,int4 &intersect); ///< Mark new address as \b heritaged
   iterator find(const Address &addr);			///< Look up if/how given address was heritaged
   int4 findPass(const Address &addr) const;		///< Look up if/how given address was heritaged
-  void erase(iterator iter) { themap.erase(iter); }	///< Remove a particular entry from the map
+  iterator erase(iterator iter) { return themap.erase(iter); }	///< Remove a particular entry from the map
   iterator begin(void) { return themap.begin(); }	///< Get starting iterator over heritaged ranges
   iterator end(void) { return themap.end(); }		///< Get ending iterator over heritaged ranges
   void clear(void) { themap.clear(); }			///< Clear the map of heritaged ranges
 };
 
+/// \brief An address range to be processed
+struct MemRange {
+  /// Properties of a single address range
+  enum {
+    new_addresses = 1,	///< The range covers addresses that have not been seen in previous passes
+    old_addresses = 2	///< The range covers addresses that were seen in previous passes
+  };
+  Address addr;		///< Starting address of the range
+  int4 size;		///< Number of bytes in the range
+  uint4 flags;		///< Property flags
+  MemRange(const Address &ad,int4 sz,uint4 fl) : addr(ad) { size = sz; flags = fl; }	///< Constructor
+  bool newAddresses(void) const { return ((flags & new_addresses)!=0); }	///< Does \b this range cover new addresses?
+  bool oldAddresses(void) const { return ((flags & old_addresses)!=0); }	///< Does \b this range cover old addresses?
+  void clearProperty(uint4 val) { flags &= ~val; }	///< Clear specific properties from the memory range
+};
+
+/// \brief A list of address ranges that need to be converted to SSA form
+///
+/// The disjoint list of ranges are built up and processed in a single pass.  The container is fed a list
+/// of ranges that may be overlapping but are already in address order. It constructs a disjoint list by taking
+/// the union of overlapping ranges.
+class TaskList {
+public:
+  /// Iterator in the list
+  typedef list<MemRange>::iterator iterator;
+private:
+  list<MemRange> tasklist;		///< List of address ranges that needs to be processed
+public:
+  void add(Address addr,int4 size,uint4 fl);				///< Add a range to the list
+  iterator insert(iterator pos,Address addr,int4 size,uint4 fl);	///< Insert a disjoint range in the list
+  iterator erase(iterator iter) { return tasklist.erase(iter); }	///< Remove a particular range
+  iterator begin(void) { return tasklist.begin(); }			///< Get iterator to beginning of \b this list
+  iterator end(void) { return tasklist.end(); }				///< Get iterator to end of \b this list
+  void clear(void) { tasklist.clear(); }				///< Clear all ranges in the list
+};
+
 /// \brief Priority queue for the phi-node (MULTIEQUAL) placement algorithm
 ///
 /// A \e work-list for basic blocks used during phi-node placement.  Implemented as
@@ -202,7 +237,7 @@ class Heritage {
 
   Funcdata *fd;		        ///< The function \b this is controlling SSA construction 
   LocationMap globaldisjoint;	///< Disjoint cover of every heritaged memory location
-  LocationMap disjoint;		///< Disjoint cover of memory locations currently being heritaged
+  TaskList disjoint;	///< Disjoint cover of memory locations currently being heritaged
   vector<vector<FlowBlock *> > domchild; ///< Parent->child edges in dominator tree
   vector<vector<FlowBlock *> > augment; ///< Augmented edges
   vector<uint4> flags;		///< Block properties for phi-node placement algorithm
@@ -233,7 +268,7 @@ class Heritage {
   void processJoins(void);
   void buildADT(void);		///< Build the augmented dominator tree
   void removeRevisitedMarkers(const vector<Varnode *> &remove,const Address &addr,int4 size);
-  int4 collect(Address addr,int4 size,vector<Varnode *> &read,vector<Varnode *> &write,vector<Varnode *> &input,vector<Varnode *> &remove) const;
+  int4 collect(MemRange &memrange,vector<Varnode *> &read,vector<Varnode *> &write,vector<Varnode *> &input,vector<Varnode *> &remove) const;
   bool callOpIndirectEffect(const Address &addr,int4 size,PcodeOp *op) const;
   Varnode *normalizeReadSize(Varnode *vn,PcodeOp *op,const Address &addr,int4 size);
   Varnode *normalizeWriteSize(Varnode *vn,const Address &addr,int4 size);
@@ -264,13 +299,14 @@ class Heritage {
   void guardLoads(uint4 fl,const Address &addr,int4 size,vector<Varnode *> &write);
   void guardReturnsOverlapping(const Address &addr,int4 size);
   void guardReturns(uint4 fl,const Address &addr,int4 size,vector<Varnode *> &write);
-  static void buildRefinement(vector<int4> &refine,const Address &addr,int4 size,const vector<Varnode *> &vnlist);
+  static void buildRefinement(vector<int4> &refine,const Address &addr,const vector<Varnode *> &vnlist);
   void splitByRefinement(Varnode *vn,const Address &addr,const vector<int4> &refine,vector<Varnode *> &split);
   void refineRead(Varnode *vn,const Address &addr,const vector<int4> &refine,vector<Varnode *> &newvn);
   void refineWrite(Varnode *vn,const Address &addr,const vector<int4> &refine,vector<Varnode *> &newvn);
   void refineInput(Varnode *vn,const Address &addr,const vector<int4> &refine,vector<Varnode *> &newvn);
   void remove13Refinement(vector<int4> &refine);
-  bool refinement(const Address &addr,int4 size,const vector<Varnode *> &readvars,const vector<Varnode *> &writevars,const vector<Varnode *> &inputvars);
+  TaskList::iterator refinement(TaskList::iterator memiter,const vector<Varnode *> &readvars,
+				const vector<Varnode *> &writevars,const vector<Varnode *> &inputvars);
   void visitIncr(FlowBlock *qnode,FlowBlock *vnode);
   void calcMultiequals(const vector<Varnode *> &write);
   void renameRecurse(BlockBasic *bl,VariableStack &varstack);

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

@@ -279,7 +279,7 @@ void PcodeOp::setOpcode(TypeOp *t_op)
   flags &= ~(PcodeOp::branch | PcodeOp::call | PcodeOp::coderef | PcodeOp::commutative |
 	     PcodeOp::returns | PcodeOp::nocollapse | PcodeOp::marker | PcodeOp::booloutput |
 	     PcodeOp::unary | PcodeOp::binary | PcodeOp::ternary | PcodeOp::special |
-	     PcodeOp::has_callspec | PcodeOp::no_copy_propagation);
+	     PcodeOp::has_callspec | PcodeOp::return_copy);
   opcode = t_op;
   flags |= t_op->getFlags();
 }

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

@@ -4,9 +4,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -90,7 +90,7 @@ public:
     binary = 0x10000,		///< Evaluate as binary expression
     special = 0x20000,		///< Cannot be evaluated (without special processing)
     ternary = 0x40000,		///< Evaluate as ternary operator (or higher)
-    no_copy_propagation = 0x80000,	///< Op does not allow COPY propagation through its inputs
+    return_copy = 0x80000,	///< Special form of COPY op for holding global values to (past) the end of the function
     nonprinting = 0x100000,	///< Op should not be directly printed as source
     halt = 0x200000,		///< instruction causes processor or process to halt
     badinstruction = 0x400000,	///< placeholder for bad instruction data
@@ -218,8 +218,7 @@ public:
   void setHoldOutput(void) { addlflags |= hold_output; }	///< Prevent output from being removed as dead code
   bool isPartialRoot(void) const { return ((addlflags&concat_root)!=0); }	///< Output is root of CONCAT tree
   void setPartialRoot(void) { addlflags |= concat_root; }	///< Mark \b this as root of CONCAT tree
-  bool stopsCopyPropagation(void) const { return ((flags&no_copy_propagation)!=0); }	///< Does \b this allow COPY propagation
+  bool isReturnCopy(void) const { return ((flags&return_copy)!=0); }	///< Is \b this a \e return form COPY
-  void setStopCopyPropagation(void) { flags |= no_copy_propagation; }	///< Stop COPY propagation through inputs
   bool noIndirectCollapse(void) const { return ((addlflags & no_indirect_collapse)!=0); }	///< Check if INDIRECT collapse is possible
   void setNoIndirectCollapse(void) { addlflags |= no_indirect_collapse; }	///< Prevent collapse of INDIRECT
   bool isStoreUnmapped(void) const { return ((addlflags & store_unmapped)!=0); }	///< Is STORE location supposed to be unmapped

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

@@ -3606,7 +3606,7 @@ int4 RulePropagateCopy::applyOp(PcodeOp *op,Funcdata &data)
   PcodeOp *copyop;
   Varnode *vn,*invn;
 
-  if (op->stopsCopyPropagation()) return 0;
+  if (op->isReturnCopy()) return 0;
   for(i=0;i<op->numInput();++i) {
     vn = op->getIn(i);
     if (!vn->isWritten()) continue; // Varnode must be written to

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

@@ -816,7 +816,7 @@ Datatype *TypeOpCallother::getOutputLocal(const PcodeOp *op) const
 TypeOpReturn::TypeOpReturn(TypeFactory *t) : TypeOp(t,CPUI_RETURN,"return")
 
 {
-  opflags = PcodeOp::special|PcodeOp::returns|PcodeOp::nocollapse|PcodeOp::no_copy_propagation;
+  opflags = PcodeOp::special|PcodeOp::returns|PcodeOp::nocollapse|PcodeOp::return_copy;
   behave = new OpBehavior(CPUI_RETURN,false,true); // Dummy behavior
 }
 

Ghidra/Features/Decompiler/src/decompile/datatests/revisit.xml

@@ -0,0 +1,31 @@
+<decompilertest>
+<binaryimage arch="x86:LE:64:default:gcc">
+<!--
+	Function makes two changes to the same global address but with different sized accesses.
+	One access is through an alias whose recovery is delayed, forcing the SSA form for the
+	address to be revisited.
+-->
+<bytechunk space="ram" offset="0x100000" readonly="true">
+488d1d6d00000048891d52000000488b
+0d4b0000008b0183c00a890166e84800
+668b054d000000660564006689054200
+0000c3
+</bytechunk>
+<symbol space="ram" offset="0x100000" name="revisit"/>
+</binaryimage>
+<script>
+  <com>lo fu revisit</com>
+  <com>decompile</com>
+  <com>print raw</com>
+  <com>quit</com>
+</script>
+<stringmatch name="Revisit SSA #1" min="1" max="1">EAX\(0x00100017:.*\) = r0x00100074:4\(i\) \+ #0xa</stringmatch>
+<stringmatch name="Revisit SSA #2" min="1" max="1">r0x00100074:4\(0x0010001c:.*\) = EAX\(0x00100017:.*\) \[\] i0x0010001c:</stringmatch>
+<stringmatch name="Revisit SSA #3" min="1" max="1">r0x00100074:2\(0x0010001c:.*\) = SUB42\(r0x00100074:4\(0x0010001c:.*\),#0x0:4\)</stringmatch>
+<stringmatch name="Revisit SSA #4" min="1" max="1">AX\(0x00100027:.*\) = r0x00100074:2\(0x0010001c:.*\) \+ #0x64:2</stringmatch>
+<stringmatch name="Revisit SSA #5" min="1" max="1">r0x00100076:2\(0x0010002b:.*\) = SUB42\(r0x00100074:4\(0x0010001c:.*\),#0x2\)</stringmatch>
+<stringmatch name="Revisit SSA #6" min="1" max="1">r0x00100074:4\(0x0010002b:.*\) = CONCAT22\(r0x00100076:2\(0x0010002b:.*\),AX\(0x00100027:.*\)\)</stringmatch>
+<stringmatch name="Revisit SSA #7" min="1" max="1">r0x00100074:4\(0x00100032:.*\) = r0x00100074:4\(0x0010002b:.*\)</stringmatch>
+<stringmatch name="Revisit SSA #8" min="0" max="0">r0x00100074:2\(.*\) = .* \[\] i0x</stringmatch>
+<stringmatch name="Revisit SSA #9" min="0" max="0">r0x00100074:2\(0x00100032:.*\) = r0x00100074:2.*</stringmatch>
+</decompilertest>