Changed handling of reheritaged memory locations

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

@@ -1782,6 +1782,7 @@ void ActionReturnRecovery::buildReturnOutput(ParamActive *active,PcodeOp *retop,
     PcodeOp *newop = data.newOp(2,retop->getAddr());
     data.opSetOpcode(newop,CPUI_PIECE);
     Varnode *newwhole = data.newVarnodeOut(trialhi.getSize()+triallo.getSize(),joinaddr,newop);
+    newwhole->setWriteMask();		// Don't let new Varnode cause additional heritage
     data.opInsertBefore(newop,retop);
     newparam.pop_back();
     newparam.back() = newwhole;
@@ -1813,7 +1814,8 @@ void ActionReturnRecovery::buildReturnOutput(ParamActive *active,PcodeOp *retop,
 	if (vn->getAddr() < addr)
 	  addr = vn->getAddr();
 	Varnode *newout = data.newVarnodeOut(preexist->getSize()+vn->getSize(),addr,newop);
-	data.opSetInput(newop,vn,0); // Most sig part
+	newout->setWriteMask();		// Don't let new Varnode cause additional heritage
+	data.opSetInput(newop,vn,0);	// Most sig part
 	data.opSetInput(newop,preexist,1);
 	data.opInsertBefore(newop,retop);
 	preexist = newout;

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

@@ -494,6 +494,7 @@ void SplitVarnode::createJoinedWhole(Funcdata &data)
 									    lo->getAddr(),lo->getSize());
   }
   whole = data.newVarnode(wholesize,newaddr);
+  whole->setWriteMask();
 }
 
 void SplitVarnode::buildLoFromWhole(Funcdata &data)
@@ -519,10 +520,12 @@ void SplitVarnode::buildLoFromWhole(Funcdata &data)
   else if (loop->code() == CPUI_INDIRECT) {
     // When converting an INDIRECT to a SUBPIECE, we need to reinsert the op AFTER the affector
     PcodeOp *affector = PcodeOp::getOpFromConst(loop->getIn(1)->getAddr());
-    data.opUninsert(loop);
+    if (!affector->isDead())
+      data.opUninsert(loop);
     data.opSetOpcode(loop,CPUI_SUBPIECE);
     data.opSetAllInput(loop,inlist);
-    data.opInsertAfter(loop,affector);
+    if (!affector->isDead())
+      data.opInsertAfter(loop,affector);
   }
   else {
     data.opSetOpcode(loop,CPUI_SUBPIECE);
@@ -553,10 +556,12 @@ void SplitVarnode::buildHiFromWhole(Funcdata &data)
   else if (hiop->code() == CPUI_INDIRECT) {
     // When converting the INDIRECT to a SUBPIECE, we need to reinsert AFTER the affector
     PcodeOp *affector = PcodeOp::getOpFromConst(hiop->getIn(1)->getAddr());
-    data.opUninsert(hiop);
+    if (!affector->isDead())
+      data.opUninsert(hiop);
     data.opSetOpcode(hiop,CPUI_SUBPIECE);
     data.opSetAllInput(hiop,inlist);
-    data.opInsertAfter(hiop,affector);
+    if (!affector->isDead())
+      data.opInsertAfter(hiop,affector);
   }
   else {
     data.opSetOpcode(hiop,CPUI_SUBPIECE);
@@ -1960,6 +1965,7 @@ bool LessThreeWay::normalizeMid(void)
   }
   midconstform = false;
   if (vnhie2->isConstant()) {
+    if (!hiconstform) return false;	// If mid is constant, both mid and hi must be constant
     midconstform = true;
     midval = vnhie2->getOffset();
     if (vnhie2->getSize() == in.getSize()) {
@@ -2820,6 +2826,7 @@ bool IndirectForm::verify(Varnode *h,Varnode *l,PcodeOp *ind)
   indhi = ind;
   if (indhi->getIn(1)->getSpace()->getType()!=IPTR_IOP) return false;
   affector = PcodeOp::getOpFromConst(indhi->getIn(1)->getAddr());
+  if (affector->isDead()) return false;
   reshi = indhi->getOut();
   if (reshi->getSpace()->getType() == IPTR_INTERNAL) return false;		// Indirect must not be through a temporary
 

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

@@ -482,9 +482,9 @@ bool FlowInfo::setFallthruBound(Address &bound)
   if (iter!=visited.begin()) {
     --iter;			// Last range less than or equal to us
     if (addr == (*iter).first) { // If we have already visited this address
-      addrlist.pop_back();	// Throw it away
       PcodeOp *op = target(addr); // But make sure the address
       data.opMarkStartBasic(op); // starts a basic block
+      addrlist.pop_back();	// Throw it away
       return false;
     }
     if (addr < (*iter).first + (*iter).second.size)

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

@@ -349,8 +349,11 @@ void Funcdata::opInsertAfter(PcodeOp *op,PcodeOp *prev)
   if (prev->isMarker()) {
     if (prev->code() == CPUI_INDIRECT) {
       Varnode *invn = prev->getIn(1);
-      if (invn->getSpace()->getType()==IPTR_IOP)
+      if (invn->getSpace()->getType()==IPTR_IOP) {
-	prev = PcodeOp::getOpFromConst(invn->getAddr()); // Store or call
+	PcodeOp *targOp = PcodeOp::getOpFromConst(invn->getAddr()); // Store or call
+	if (!targOp->isDead())
+	  prev = targOp;
+      }
     }
   }
   list<PcodeOp *>::iterator iter = prev->getBasicIter();

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

@@ -148,6 +148,54 @@ void Heritage::clearInfoList(void)
     (*iter).reset();
 }
 
+/// \brief Remove deprecated CPUI_MULTIEQUAL or CPUI_INDIRECT 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.
+/// \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
+void Heritage::removeRevisitedMarkers(const vector<Varnode *> &remove,const Address &addr,int4 size)
+
+{
+  vector<Varnode *> newInputs;
+  list<PcodeOp *>::iterator pos;
+  for(int4 i=0;i<remove.size();++i) {
+    Varnode *vn = remove[i];
+    PcodeOp *op = vn->getDef();
+    BlockBasic *bl = op->getParent();
+    if (op->code() == CPUI_INDIRECT) {
+      Varnode *iopVn = op->getIn(1);
+      PcodeOp *targetOp =  PcodeOp::getOpFromConst(iopVn->getAddr());
+      if (targetOp->isDead())
+	pos = op->getBasicIter();
+      else
+	pos = targetOp->getBasicIter();
+      ++pos;		// Insert SUBPIECE after target of INDIRECT
+    }
+    else {
+      pos = op->getBasicIter();	// Insert SUBPIECE after all MULTIEQUALs in block
+      ++pos;
+      while(pos != bl->endOp() && (*pos)->code() == CPUI_MULTIEQUAL)
+	++pos;
+    }
+    int4 offset = vn->overlap(addr,size);
+    fd->opUninsert(op);
+    newInputs.clear();
+    Varnode *big = fd->newVarnode(size,addr);
+    big->setActiveHeritage();
+    newInputs.push_back(big);
+    newInputs.push_back(fd->newConstant(4, offset));
+    fd->opSetOpcode(op, CPUI_SUBPIECE);
+    fd->opSetAllInput(op, newInputs);
+    fd->opInsert(op, bl, pos);
+    vn->setWriteMask();
+  }
+}
+
 /// \brief Collect free reads, writes, and inputs in the given address range
 ///
 /// \param addr is the starting address of the range
@@ -155,10 +203,11 @@ void Heritage::clearInfoList(void)
 /// \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,
 		      vector<Varnode *> &read,vector<Varnode *> &write,
-		      vector<Varnode *> &input) const
+		      vector<Varnode *> &input,vector<Varnode *> &remove) const
 
 {
   Varnode *vn;
@@ -177,9 +226,13 @@ int4 Heritage::collect(Address addr,int4 size,
     vn = *viter;
     if (!vn->isWriteMask()) {
       if (vn->isWritten()) {
-	if (vn->getSize() > maxsize) // Look for maximum write size
+	if (vn->getSize() < size && vn->getDef()->isMarker())
-	  maxsize = vn->getSize();
+	  remove.push_back(vn);
-	write.push_back(vn);
+	else {
+	  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);
@@ -2178,6 +2231,7 @@ void Heritage::placeMultiequals(void)
   vector<Varnode *> readvars;
   vector<Varnode *> writevars;
   vector<Varnode *> inputvars;
+  vector<Varnode *> removevars;
   PcodeOp *multiop;
   Varnode *vnin;
   BlockBasic *bl;
@@ -2189,7 +2243,8 @@ void Heritage::placeMultiequals(void)
     readvars.clear();
     writevars.clear();
     inputvars.clear();
-    max = collect(addr,size,readvars,writevars,inputvars); // Collect reads/writes
+    removevars.clear();
+    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);
@@ -2197,11 +2252,14 @@ void Heritage::placeMultiequals(void)
 	readvars.clear();
 	writevars.clear();
 	inputvars.clear();
-	collect(addr,size,readvars,writevars,inputvars);
+	removevars.clear();
+	collect(addr,size,readvars,writevars,inputvars,removevars);
       }
     }
     if (readvars.empty() && (addr.getSpace()->getType() == IPTR_INTERNAL))
       continue;
+    if (!removevars.empty())
+      removeRevisitedMarkers(removevars, addr, size);
     guardInput(addr,size,inputvars);
     guard(addr,size,readvars,writevars,inputvars);
     if (readvars.empty()&&writevars.empty()) continue;

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

@@ -229,7 +229,8 @@ class Heritage {
   void floatExtensionWrite(Varnode *vn,JoinRecord *joinrec);
   void processJoins(void);
   void buildADT(void);		///< Build the augmented dominator tree
-  int4 collect(Address addr,int4 size,vector<Varnode *> &read,vector<Varnode *> &write,vector<Varnode *> &input) const;
+  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;
   bool callOpIndirectEffect(const Address &addr,int4 size,PcodeOp *op) const;
   Varnode *normalizeReadSize(Varnode *vn,const Address &addr,int4 size);
   Varnode *normalizeWriteSize(Varnode *vn,const Address &addr,int4 size);

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

@@ -289,7 +289,6 @@ void ScopeLocal::collectNameRecs(void)
   while(iter!=nametree.end()) {
     Symbol *sym = *iter++;
     if (sym->isNameLocked()&&(!sym->isTypeLocked())) {
-      addRecommendName(sym);
       if (sym->isThisPointer()) {		// If there is a "this" pointer
 	Datatype *dt = sym->getType();
 	if (dt->getMetatype() == TYPE_PTR) {
@@ -301,6 +300,7 @@ void ScopeLocal::collectNameRecs(void)
 	  }
 	}
       }
+      addRecommendName(sym);	// This deletes the symbol
     }
   }
 }
@@ -1209,8 +1209,12 @@ SymbolEntry *ScopeLocal::remapSymbol(Symbol *sym,const Address &addr,const Addre
   SymbolEntry *entry = sym->getFirstWholeMap();
   int4 size = entry->getSize();
   if (!entry->isDynamic()) {
-    if (entry->getAddr() == addr && entry->getFirstUseAddress() == usepoint)
+    if (entry->getAddr() == addr) {
-      return entry;
+      if (usepoint.isInvalid() && entry->getFirstUseAddress().isInvalid())
+	return entry;
+      if (entry->getFirstUseAddress() == usepoint)
+	return entry;
+    }
   }
   removeSymbolMappings(sym);
   RangeList rnglist;