Merge remote-tracking branch 'origin/GP-2359_UnaliasedStack'

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

@@ -2129,10 +2129,9 @@ int4 ActionRestructureVarnode::apply(Funcdata &data)
 {
   ScopeLocal *l1 = data.getScopeLocal();
 
-  bool aliasyes = data.isJumptableRecoveryOn() ? false : (numpass != 0);
+  bool aliasyes = (numpass != 0);	// Alias calculations are not reliable on the first pass
   l1->restructureVarnode(aliasyes);
-  // Note the alias calculation, may not be very good on the first pass
-  if (data.syncVarnodesWithSymbols(l1,false))
+  if (data.syncVarnodesWithSymbols(l1,false,aliasyes))
     count += 1;
 
   numpass += 1;
@@ -2157,7 +2156,7 @@ int4 ActionRestructureHigh::apply(Funcdata &data)
 #endif
 
   l1->restructureHigh();
-  if (data.syncVarnodesWithSymbols(l1,true))
+  if (data.syncVarnodesWithSymbols(l1,true,true))
     count += 1;
 
 #ifdef OPACTION_DEBUG

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

@@ -385,7 +385,7 @@ public:
   bool checkCallDoubleUse(const PcodeOp *opmatch,const PcodeOp *op,const Varnode *vn,uint4 fl,const ParamTrial &trial) const;
   bool onlyOpUse(const Varnode *invn,const PcodeOp *opmatch,const ParamTrial &trial,uint4 mainFlags) const;
   bool ancestorOpUse(int4 maxlevel,const Varnode *invn,const PcodeOp *op,ParamTrial &trial,int4 offset,uint4 mainFlags) const;
-  bool syncVarnodesWithSymbols(const ScopeLocal *lm,bool typesyes);
+  bool syncVarnodesWithSymbols(const ScopeLocal *lm,bool updateDatatypes,bool unmappedAliasCheck);
   Datatype *checkSymbolType(Varnode *vn);	///< Check for any delayed symbol data-type information on the given Varnode
   void transferVarnodeProperties(Varnode *vn,Varnode *newVn,int4 lsbOffset);
   bool fillinReadOnly(Varnode *vn);		///< Replace the given Varnode with its (constant) value in the load image

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

@@ -807,9 +807,9 @@ void Funcdata::calcNZMask(void)
 /// The caller can elect to update data-type information as well, where Varnodes
 /// and their associated HighVariables have their data-type finalized based symbols.
 /// \param lm is the Symbol scope within which to search for mapped Varnodes
-/// \param typesyes is \b true if the caller wants to update data-types
+/// \param updataDatatypes is \b true if the caller wants to update data-types
 /// \return \b true if any Varnode was updated
-bool Funcdata::syncVarnodesWithSymbols(const ScopeLocal *lm,bool typesyes)
+bool Funcdata::syncVarnodesWithSymbols(const ScopeLocal *lm,bool updateDatatypes,bool unmappedAliasCheck)
 
 {
   bool updateoccurred = false;
@@ -827,7 +827,7 @@ bool Funcdata::syncVarnodesWithSymbols(const ScopeLocal *lm,bool typesyes)
     if (entry != (SymbolEntry *)0) {
       fl = entry->getAllFlags();
       if (entry->getSize() >= vnexemplar->getSize()) {
-	if (typesyes) {
+	if (updateDatatypes) {
 	  ct = entry->getSizedType(vnexemplar->getAddr(), vnexemplar->getSize());
 	  if (ct != (Datatype *)0 && ct->getMetatype() == TYPE_UNKNOWN)
 	    ct = (Datatype *)0;
@@ -849,6 +849,10 @@ bool Funcdata::syncVarnodesWithSymbols(const ScopeLocal *lm,bool typesyes)
 	// kind of symbol, if we are in scope
 	fl = Varnode::mapped | Varnode::addrtied;
       }
+      else if (unmappedAliasCheck) {
+	// If the varnode is not in scope, check if we should treat as unaliased
+	fl = lm->isUnmappedUnaliased(vnexemplar) ? Varnode::nolocalalias : 0;
+      }
       else
 	fl = 0;
     }

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

@@ -2503,7 +2503,7 @@ void Heritage::heritage(void)
       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;
-	if ((!needwarning)&&(info->deadremoved>0)) {
+	if ((!needwarning)&&(info->deadremoved>0)&&!fd->isJumptableRecoveryOn()) {
 	  needwarning = true;
 	  bumpDeadcodeDelay(vn);
 	  warnvn = vn;
@@ -2512,7 +2512,7 @@ void Heritage::heritage(void)
       }
       else {	// Partially contained in old range, but may contain new stuff
 	disjoint.add((*liter).first,(*liter).second.size,pass,prev);
-	if ((!needwarning)&&(info->deadremoved>0)) {
+	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
 		  // In most cases, a truly new overlapping read will hit the bumpDeadcodeDelay either here or in prev==2

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

@@ -275,7 +275,8 @@ ScopeLocal::ScopeLocal(uint8 id,AddrSpace *spc,Funcdata *fd,Architecture *g) : S
 
 {
   space = spc;
-  deepestParamOffset = ~((uintb)0);
+  minParamOffset = ~((uintb)0);
+  maxParamOffset = 0;
   rangeLocked = false;
   stackGrowsNegative = true;
   restrictScope(fd);
@@ -344,7 +345,8 @@ void ScopeLocal::resetLocalWindow(void)
 
 {
   stackGrowsNegative = fd->getFuncProto().isStackGrowsNegative();
-  deepestParamOffset = stackGrowsNegative ? ~((uintb)0) : 0;
+  minParamOffset = ~(uintb)0;
+  maxParamOffset = 0;
 
   if (rangeLocked) return;
 
@@ -395,6 +397,22 @@ void ScopeLocal::decodeWrappingAttributes(Decoder &decoder)
   space = decoder.readSpace(ATTRIB_MAIN);
 }
 
+/// Currently we treat all unmapped Varnodes as not having an alias, unless the Varnode is on the stack
+/// and the location is also used to pass parameters.  This should not be called until the second pass, in
+/// order to give markNotMapped a chance to be called.
+/// Return \b true if the Varnode can be treated as having no aliases.
+/// \param vn is the given Varnode
+/// \return \b true if there are no aliases
+bool ScopeLocal::isUnmappedUnaliased(Varnode *vn) const
+
+{
+  if (vn->getSpace() != space) return false;	// Must be in mapped local (stack) space
+  if (maxParamOffset < minParamOffset) return false;	// If no min/max, then we have no know stack parameters
+  if (vn->getOffset() < minParamOffset || vn->getOffset() > maxParamOffset)
+    return true;
+  return false;
+}
+
 /// The given range can no longer hold a \e mapped local variable. This indicates the range
 /// is being used for temporary storage.
 /// \param spc is the address space holding the given range
@@ -412,14 +430,10 @@ void ScopeLocal::markNotMapped(AddrSpace *spc,uintb first,int4 sz,bool parameter
   else if (last > spc->getHighest())
     last = spc->getHighest();
   if (parameter) {		// Everything above parameter
-    if (stackGrowsNegative) {
-      if (first < deepestParamOffset)
-	deepestParamOffset = first;
-    }
-    else {
-      if (first > deepestParamOffset)
-	deepestParamOffset = first;
-    }
+    if (first < minParamOffset)
+      minParamOffset = first;
+    if (last > maxParamOffset)
+      maxParamOffset = last;
   }
   Address addr(space,first);
 				// Remove any symbols under range
@@ -463,7 +477,8 @@ string ScopeLocal::buildVariableName(const Address &addr,
 	start = -start;
       }
       else {
-	if (deepestParamOffset + 1 > 1 && stackGrowsNegative == (addr.getOffset() < deepestParamOffset)) {
+	if ((minParamOffset < maxParamOffset) &&
+	    (stackGrowsNegative ? (addr.getOffset() < minParamOffset) : (addr.getOffset() > maxParamOffset))) {
 	  s << 'Y';		// Indicate unusual region of stack
 	}
       }

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

@@ -203,7 +203,8 @@ class ScopeLocal : public ScopeInternal {
   list<NameRecommend> nameRecommend;	///< Symbol name recommendations for specific addresses
   list<DynamicRecommend> dynRecommend;		///< Symbol name recommendations for dynamic locations
   list<TypeRecommend> typeRecommend;	///< Data-types for specific storage locations
-  uintb deepestParamOffset;		///< Deepest position of a parameter passed (to a called function) on the stack
+  uintb minParamOffset;		///< Minimum offset of parameter passed (to a called function) on the stack
+  uintb maxParamOffset;		///< Maximum offset of parameter passed (to a called function) on the stack
   bool stackGrowsNegative;	///< Marked \b true if the stack is considered to \e grow towards smaller offsets
   bool rangeLocked;		///< True if the subset of addresses \e mapped to \b this scope has been locked
   bool adjustFit(RangeHint &a) const;	///< Make the given RangeHint fit in the current Symbol map
@@ -226,6 +227,8 @@ public:
   /// \return \b true is the Varnode can be used as unaffected storage
   bool isUnaffectedStorage(Varnode *vn) const { return (vn->getSpace() == space); }
 
+  bool isUnmappedUnaliased(Varnode *vn) const;	///< Check if a given unmapped Varnode should be treated as unaliased.
+
   void markNotMapped(AddrSpace *spc,uintb first,int4 sz,bool param);	///< Mark a specific address range is not mapped
 
 				// Routines that are specific to one address space