Merge remote-tracking branch 'origin/GP-2391_ARmThumb_returndetect--SQUASHED'

Ghidra/Features/Base/src/main/java/ghidra/app/cmd/function/CreateThunkFunctionCmd.java

@@ -612,7 +612,7 @@ public class CreateThunkFunctionCmd extends BackgroundCommand {
 
 				// Storing to a location is not allowed for a thunk
 				//   as a side-effect of the thunk.
-				if (pcodeOp.getOpcode() == PcodeOp.STORE) {
+				if (checkForSideEffects && pcodeOp.getOpcode() == PcodeOp.STORE) {
 					return null;
 				}
 
@@ -702,14 +702,35 @@ public class CreateThunkFunctionCmd extends BackgroundCommand {
 		return null;
 	}
 
+	// Check for a local branch, which is an unconditional branch that jumps at most
+	// 8 bytes ahead to allow for embedded addresses or mini thunks to another thunk.
+	//
+	// An example of a mini thunk is an Arm Thumb function that converts to ARM code
+	// to for calling another function that is Arm code.
+	//
 	private static boolean isLocalBranch(Listing listing, Instruction instr, FlowType flowType) {
-		if ((flowType.isJump() && !flowType.isConditional())) {
-			Address[] flows = instr.getFlows();
-			// allow a jump of 4 instructions forward.
-			if (flows.length == 1 && Math.abs(flows[0].subtract(instr.getMinAddress())) <= 4) {
-				return true;
-			}
+		// if not a jump instruction, or is a conditional jump, not local branch
+		if (!flowType.isJump() || flowType.isConditional()) {
+			return false;
 		}
+
+		Address[] flows = instr.getFlows();
+		if (flows.length != 1) {
+			// no branch, or more than one branch is not local
+			return false;
+		}
+
+		// if not in same address space, can't be a local branch
+		Address minAddress = instr.getMinAddress();
+		if (!minAddress.hasSameAddressSpace(flows[0])) {
+			return false;
+		}
+
+		// allow a jump of 8 bytes forward to allow for an embedded address
+		if (Math.abs(flows[0].subtract(instr.getMinAddress())) <= 8) {
+			return true;
+		}
+	
 		return false;
 	}
 

Ghidra/Features/Base/src/main/java/ghidra/program/util/ContextEvaluator.java

@@ -92,6 +92,17 @@ public interface ContextEvaluator {
 	 */
 	boolean evaluateDestination(VarnodeContext context, Instruction instruction);
 
+	/**
+	 * Evaluate the the target of a return
+	 * 
+	 * @param retVN varnode that is the target of a RETURN pcodeop
+	 * @param context current register context
+	 * @param instruction instruction that has an unknown destination
+	 * 
+	 * @return true if the evaluation should stop, false to continue evaluation
+	 */
+	public boolean evaluateReturn(Varnode retVN, VarnodeContext context, Instruction instruction);
+	
 	/**
 	 * Called when a value is needed for a register that is unknown
 	 * 

Ghidra/Features/Base/src/main/java/ghidra/program/util/ContextEvaluatorAdapter.java

@@ -54,6 +54,11 @@ public class ContextEvaluatorAdapter implements ContextEvaluator {
 	public boolean evaluateDestination(VarnodeContext context, Instruction instruction) {
 		return false;
 	}
+	
+	@Override
+	public boolean evaluateReturn(Varnode retVN, VarnodeContext context, Instruction instruction) {
+		return false;
+	}
 
 	@Override
 	public Long unknownValue(VarnodeContext context, Instruction instruction, Varnode node) {

Ghidra/Features/Base/src/main/java/ghidra/program/util/SymbolicPropogator.java

@@ -838,9 +838,7 @@ public class SymbolicPropogator {
 					case PcodeOp.BRANCHIND:
 						try {
 							val1 = vContext.getValue(in[0], evaluator);
-							lval1 = vContext.getConstant(val1, evaluator);
-							vt = vContext.getVarnode(minInstrAddress.getAddressSpace().getSpaceID(),
-								lval1, 0);
+							vt = getConstantOrExternal(vContext, minInstrAddress, val1);
 							makeReference(vContext, instruction, ptype, -1, vt,
 								instruction.getFlowType(), monitor);
 						}
@@ -887,8 +885,11 @@ public class SymbolicPropogator {
 									// if not, we must rely on reference to function.
 									target = resolveFunctionReference(val1.getAddress());
 								}
+								else if (val1.getSpace() == AddressSpace.EXTERNAL_SPACE.getSpaceID()) {
+									// target = val1.getAddress();
+								}
 								// if the value didn't get changed, then the real value isn't in here, don't make a reference
-								if (target != null && (val1.isAddress() || val1.isConstant())) {
+								if (target != null) {
 									Reference[] refs = instruction.getReferencesFrom();
 									// make sure we aren't replacing a read ref with a call to the same place
 									if (refs.length <= 0 ||
@@ -1083,6 +1084,18 @@ public class SymbolicPropogator {
 						break;
 
 					case PcodeOp.RETURN:
+
+						// if return value is a location, give evaluator a chance to check the value
+						try {
+							val1 = vContext.getValue(in[0], evaluator);
+							if (evaluator != null && evaluator.evaluateReturn(val1, vContext, instruction)) {
+								canceled = true;
+								return null;
+							}
+						}
+						catch (NotFoundException e) {
+							// constant not found, ignore
+						}
 						// put references on any return value that is a pointer and could be returned
 
 						addReturnReferences(instruction, vContext, monitor);
@@ -1428,6 +1441,19 @@ public class SymbolicPropogator {
 		return nextAddr;
 	}
 
+	private Varnode getConstantOrExternal(VarnodeContext vContext, Address minInstrAddress,
+			Varnode val1) throws NotFoundException {
+		Varnode vt;
+		if (!context.isExternalSpace(val1.getSpace())) {
+			long lval = vContext.getConstant(val1, evaluator);
+			vt = vContext.getVarnode(minInstrAddress.getAddressSpace().getSpaceID(),
+				lval, 0);
+		} else {
+			vt = val1;
+		}
+		return vt;
+	}
+
 	private Varnode getStoredLocation(VarnodeContext vContext, Varnode[] in) {
 		Varnode out = null;
 		Varnode val;
@@ -1755,7 +1781,7 @@ public class SymbolicPropogator {
 	 * @param prog program
 	 * @param addr addr of instruction that could have an override of the stack depth
 	 * @param purge current purge depth.
-	 * @return
+	 * @return new purge, which includes the extrapop value
 	 */
 	private int addStackOverride(Program prog, Address addr, int purge) {
 		Integer stackDepthChange = CallDepthChangeInfo.getStackDepthChange(prog, addr);
@@ -1904,8 +1930,8 @@ public class SymbolicPropogator {
 	/**
 	 * Find the operand that is assigning to the varnode with contains the load or store reference offset
 	 * 
-	 * @param instruction
-	 * @param assigningVarnode
+	 * @param instruction the instruction with operands
+	 * @param assigningVarnode varnode representing the load/store assignment
 	 * @return operand index if found or -1 if not
 	 */
 	private int findOperandWithVarnodeAssignment(Instruction instruction,
@@ -2079,7 +2105,8 @@ public class SymbolicPropogator {
 	 * 
 	 * @param instruction - reference is to be placed on (used for address)
 	 * @param offset - offset into the address space. (word addressing based)
-	 * @return
+	 * 
+	 * @return spaceID of address to use for the reference
 	 */
 	private int getReferenceSpaceID(Instruction instruction, long offset) {
 		// TODO: this should be passed to the client callback to make the decision
@@ -2172,11 +2199,11 @@ public class SymbolicPropogator {
 	 * @param opIndex - operand it should be placed on, or -1 if unknown
 	 * @param vt - place to reference, could be a full address, or just a constant
 	 * @param refType - type of reference
-	 * @param monitor
+	 * @param monitor to cancel
 	 */
 	public void makeReference(VarnodeContext varnodeContext, Instruction instruction, int pcodeop,
 			int opIndex, Varnode vt, RefType refType, TaskMonitor monitor) {
-		if (!vt.isAddress()) {
+		if (!vt.isAddress() && !varnodeContext.isExternalSpace(vt.getSpace())) {
 			if (evaluator != null) {
 				evaluator.evaluateSymbolicReference(varnodeContext, instruction, vt.getAddress());
 			}
@@ -2196,13 +2223,15 @@ public class SymbolicPropogator {
 	 *  The target could be an external Address carried along and then finally used.
 	 *  External addresses are OK as long as nothing is done to the offset.
 	 *  
-	 * @param vContext - context to use for any other infomation needed
+	 * @param vContext - context to use for any other information needed
 	 * @param instruction - instruction to place the reference on.
 	 * @param opIndex - operand it should be placed on, or -1 if unknown
 	 * @param knownSpaceID target space ID or -1 if only offset is known
 	 * @param wordOffset - target offset that is word addressing based
+	 * @param size - size of the access to the location
 	 * @param refType - type of reference
-	 * @param pcodeop - pcode op that caused the reference
+	 * @param pcodeop - op that caused the reference
+	 * @param knownReference - true if reference is known to be a real reference, not speculative
 	 * @param monitor - the task monitor
 	 */
 	public void makeReference(VarnodeContext vContext, Instruction instruction, int opIndex,
@@ -2437,25 +2466,23 @@ public class SymbolicPropogator {
 		}
 		DataType dt = Undefined.getUndefinedDataType(size);
 
-		Data data = null;
 		try {
 			// create data at the location so that we record the access size
 			//   the data is undefined, and SHOULD be overwritten if something
 			//   else knows better about the location.
 			// This should only be done on references that are know good read/write, not data
-			data = program.getListing().createData(address, dt);
+			program.getListing().createData(address, dt);
 		}
 		catch (CodeUnitInsertionException e) {
-			data = program.getListing().getDefinedDataAt(address);
+			program.getListing().getDefinedDataAt(address);
 		}
 		int addrByteSize = dt.getLength();
 
 		return addrByteSize;
 	}
 
-	private int findOpIndexForRef(VarnodeContext context, Instruction instruction, int opIndex,
+	private int findOpIndexForRef(VarnodeContext vcontext, Instruction instruction, int opIndex,
 			long wordOffset, RefType refType) {
-		boolean foundExactValue = false;
 
 		int numOperands = instruction.getNumOperands();
 
@@ -2466,7 +2493,6 @@ public class SymbolicPropogator {
 				Address opAddr = instruction.getAddress(i);
 				if (opAddr != null && opAddr.getAddressableWordOffset() == wordOffset) {
 					opIndex = i;
-					foundExactValue = true;
 					break;
 				}
 			}
@@ -2481,14 +2507,12 @@ public class SymbolicPropogator {
 					// value for pointer can differ by 1 bit, which is sometimes ignored for flow
 					if (checkOffByOne(reg, wordOffset)) {
 						opIndex = i;
-						foundExactValue = true;
 						if (refType.isFlow()) {
 							break;
 						}
 					}
 					if (checkOffByOne(reg.getParentRegister(), wordOffset)) {
 						opIndex = i;
-						foundExactValue = true;
 						if (refType.isFlow()) {
 							break;
 						}
@@ -2501,7 +2525,6 @@ public class SymbolicPropogator {
 				// sort of a hack, for memory that is not byte addressable
 				if (val == wordOffset || val == (wordOffset >> 1)) {
 					opIndex = i;
-					foundExactValue = true;
 					break;
 				}
 			}
@@ -2520,7 +2543,6 @@ public class SymbolicPropogator {
 							if (val == wordOffset || val == (wordOffset >> 1) ||
 								(val + baseRegVal) == wordOffset) {
 								opIndex = i;
-								foundExactValue = true;
 								break;
 							}
 							val = ((Scalar) obj).getSignedValue();
@@ -2529,7 +2551,7 @@ public class SymbolicPropogator {
 						}
 						if (obj instanceof Register) {
 							Register reg = (Register) obj;
-							BigInteger val = context.getValue(reg, false);
+							BigInteger val = vcontext.getValue(reg, false);
 							if (val != null) {
 								baseRegVal = val.longValue();
 								if ((baseRegVal & pointerMask) == wordOffset) {
@@ -2542,7 +2564,6 @@ public class SymbolicPropogator {
 					}
 					if (offset_residue_neg == 0 || offset_residue_pos == 0) {
 						opIndex = i;
-						foundExactValue = true;
 						break;
 					}
 					if (opIndex == Reference.MNEMONIC && i == (numOperands - 1)) {
@@ -2599,7 +2620,7 @@ public class SymbolicPropogator {
 	/**
 	 * enable/disable checking return for constant references
 	 * 
-	 * @param checkReturnRefsOption
+	 * @param checkReturnRefsOption true if enable check return for constant references
 	 */
 	public void setReturnRefCheck(boolean checkReturnRefsOption) {
 		checkForReturnRefs = checkReturnRefsOption;
@@ -2608,7 +2629,7 @@ public class SymbolicPropogator {
 	/**
 	 * enable/disable checking stored values for constant references
 	 * 
-	 * @param checkStoredRefsOption
+	 * @param checkStoredRefsOption true if enable check for stored values for constant references
 	 */
 	public void setStoredRefCheck(boolean checkStoredRefsOption) {
 		checkForStoredRefs = checkStoredRefsOption;

Ghidra/Features/Base/src/main/java/ghidra/program/util/VarnodeContext.java

@@ -50,7 +50,7 @@ public class VarnodeContext implements ProcessorContext {
 
 	// holds temp values for computation
 	private HashMap<Varnode, Varnode> tempVals = new HashMap<>();
-	protected HashMap<Varnode, Varnode> tempUniqueVals = new HashMap<>();
+	protected HashMap<Long, Varnode> tempUniqueVals = new HashMap<>(); // unique's stored only by offset
 	protected boolean keepTempUniqueValues = false;
 
 	protected HashSet<Varnode> clearVals = new HashSet<>();
@@ -327,30 +327,6 @@ public class VarnodeContext implements ProcessorContext {
 		}
 	}
 
-	/**
-	 * Check if varnode is in the stack space
-	 * 
-	 * @param varnode varnode to check
-	 * 
-	 * @return true if this varnode is stored in the symbolic stack space
-	 */
-	public boolean isStackSymbolicSpace(Varnode varnode) {
-		// symbolic spaces are off of a register, find the space
-		AddressSpace regSpace = addrFactory.getAddressSpace(varnode.getSpace());
-
-		return isStackSpaceName(regSpace.getName());
-	}
-
-	/**
-	 * Check if spaceName is associated with the stack
-	 * 
-	 * @param spaceName of address space to check
-	 * @return true if spaceName is associated with the stack space
-	 */
-	public boolean isStackSpaceName(String spaceName) {
-		return validSymbolicStackNames.contains(spaceName);
-	}
-
 	/**
 	 * 
 	 * @return Register that represents the stack register
@@ -381,7 +357,7 @@ public class VarnodeContext implements ProcessorContext {
 		}
 		Varnode rvnode = null;
 		if (varnode.isUnique()) {
-			rvnode = tempUniqueVals.get(varnode);
+			rvnode = tempUniqueVals.get(varnode.getOffset());
 		}
 		else {
 			rvnode = tempVals.get(varnode);
@@ -493,10 +469,8 @@ public class VarnodeContext implements ProcessorContext {
 			// don't trust any place that has an external reference off of it
 			Reference[] refsFrom = program.getReferenceManager().getReferencesFrom(addr);
 			if (refsFrom.length > 0 && refsFrom[0].isExternalReference()) {
-				return varnode;
-				// TODO: External address space is not a space yet!
-				//Address external = refsFrom[0].getToAddress();
-				//return createVarnode(external.getOffset(), external.getAddressSpace().getBaseSpaceID(), 0);
+				Address external = refsFrom[0].getToAddress();
+				return createVarnode(external.getOffset(), external.getAddressSpace().getSpaceID(), 0);
 			}
 
 			// If the memory is Writeable, then maybe don't trust it
@@ -733,7 +707,7 @@ public class VarnodeContext implements ProcessorContext {
 			if (mustClear) {
 				result = null;
 			}
-			tempUniqueVals.put(out, result);
+			tempUniqueVals.put(out.getOffset(), result);
 		}
 		else {
 			tempVals.put(out, result);
@@ -1062,15 +1036,21 @@ public class VarnodeContext implements ProcessorContext {
 	 * 
 	 * @param out varnode to put it in
 	 * @param in varnode to copy from.
-	 * @param evaluator 
-	 * @throws NotFoundException 
+	 * @param mustClearAll true if must clear if value is not unique
+	 * @param evaluator user provided evaluator if needed
+	 * @throws NotFoundException if there is no known value for in
 	 */
 	public void copy(Varnode out, Varnode in, boolean mustClearAll, ContextEvaluator evaluator)
 			throws NotFoundException {
 		Varnode val1 = null;
+		val1 = getValue(in, evaluator);
+		// if truncating a constant get a new constant of the proper size
+		if (val1 != null && val1.isConstant() && in.getSize() > out.getSize()) {
+			val1 = createConstantVarnode(val1.getOffset(), out.getSize());
+		}
+		
 		if (!in.isRegister() || !out.isRegister()) {
 			// normal case easy get value, put value
-			val1 = getValue(in, evaluator);
 			putValue(out, val1, mustClearAll);
 			return;
 		}
@@ -1078,7 +1058,6 @@ public class VarnodeContext implements ProcessorContext {
 			clearVals.add(out);
 		}
 
-		val1 = getValue(in, evaluator);
 		putValue(out, val1, mustClearAll);
 	}
 
@@ -1189,10 +1168,6 @@ public class VarnodeContext implements ProcessorContext {
 		return createVarnode(result, spaceID, val1.getSize());
 	}
 
-	protected boolean isRegister(Varnode varnode) {
-		return varnode.isRegister() || trans.getRegister(varnode) != null;
-	}
-
 	public Varnode and(Varnode val1, Varnode val2, ContextEvaluator evaluator)
 			throws NotFoundException {
 		if (val1.equals(val2)) {
@@ -1226,6 +1201,9 @@ public class VarnodeContext implements ProcessorContext {
 				}
 			}
 		}
+		else if (isExternalSpace(spaceID)) {
+			return (val1); // can't mess with an external address
+		}
 		else {
 			throw notFoundExc;
 		}
@@ -1475,24 +1453,103 @@ public class VarnodeContext implements ProcessorContext {
 		putValue(regVnode, createConstantVarnode(value.longValue(), regVnode.getSize()), false);
 		propogateResults(false);
 	}
+	
+	/**
+	 * Check if the varnode is associated with a Symbolic location
+	 * 
+	 * @param varnode to check
+	 * @return true if  the varnode is a symbolic location
+	 */
+	public boolean isSymbol(Varnode varnode) {
+		return isSymbolicSpace(varnode.getAddress().getAddressSpace());
+	}
+	
+	/**
+	 * Check if the varnode is associated with a register.
+	 * 
+	 * @param varnode to check
+	 * @return true if the varnode is associated with a register
+	 */
+	public boolean isRegister(Varnode varnode) {
+		return varnode.isRegister() || trans.getRegister(varnode) != null;
+	}
+	
+	/**
+	 * Check if varnode is in the stack space
+	 * 
+	 * @param varnode varnode to check
+	 * 
+	 * @return true if this varnode is stored in the symbolic stack space
+	 */
+	public boolean isStackSymbolicSpace(Varnode varnode) {
+		// symbolic spaces are off of a register, find the space
+		AddressSpace regSpace = addrFactory.getAddressSpace(varnode.getSpace());
 
-	public boolean isSymbol(Varnode node) {
-		return isSymbolicSpace(node.getAddress().getAddressSpace());
+		return isStackSpaceName(regSpace.getName());
 	}
 
+	/**
+	 * Check if spaceName is associated with the stack
+	 * 
+	 * @param spaceName of address space to check
+	 * @return true if spaceName is associated with the stack space
+	 */
+	public boolean isStackSpaceName(String spaceName) {
+		return validSymbolicStackNames.contains(spaceName);
+	}
+
+	/**
+	 * Check if the space name is a symbolic space.
+	 * A symbolic space is a space named after a register/unknown value and
+	 * an offset into that symbolic space.
+	 * 
+	 * Symbolic spaces come from the OffsetAddressFactory
+	 * 
+	 * @param space the address space
+	 * @return true if is a symbolic space
+	 */
 	public boolean isSymbolicSpace(AddressSpace space) {
 		int spaceID = space.getSpaceID();
 		return OffsetAddressFactory.isSymbolSpace(spaceID);
 	}
 
+	/**
+	 * Check if the space ID is a symbolic space.
+	 * A symbolic space is a space named after a register/unknown value and
+	 * an offset into that symbolic space.
+	 * 
+	 * Symbolic spaces come from the OffsetAddressFactory
+	 * 
+	 * @param spaceID the ID of the space
+	 * @return true if is a symbolic space
+	 */
 	public boolean isSymbolicSpace(int spaceID) {
 		return OffsetAddressFactory.isSymbolSpace(spaceID);
 	}
 
+	/**
+	 * Check if the space ID is an external space.
+	 * 
+	 * External spaces are single locations that have no size
+	 * normally associated with a location in another program.
+	 * 
+	 * @param spaceID the ID of the space
+	 * @return true if is a symbolic space
+	 */
+	public boolean isExternalSpace(int spaceID) {
+		return spaceID == AddressSpace.EXTERNAL_SPACE.getSpaceID();
+	}
+
+	/**
+	 * Save the current memory state
+	 */
 	public void pushMemState() {
 		memoryVals.push(new HashMap<Varnode, Varnode>());
 	}
 
+	/**
+	 * restore a previously saved memory state
+	 */
 	public void popMemState() {
 		memoryVals.pop();
 	}
@@ -1518,6 +1575,12 @@ class OffsetAddressFactory extends DefaultAddressFactory {
 				}
 			}
 		}
+		try {
+			addAddressSpace(AddressSpace.EXTERNAL_SPACE);
+		}
+		catch (DuplicateNameException e) {
+			throw new AssertException("Duplicate name should not occur.");
+		}
 	}
 
 	private int getNextUniqueID() {

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/app/util/PseudoDisassembler.java

@@ -575,34 +575,86 @@ public class PseudoDisassembler {
 	 * If a bad instruction is hit or it does not flow well, then return
 	 * false.
 	 * 
-	 * @param target - taraget address to disassemble
+	 * @param entryPoint address to check
 	 * 
-	 * @return true if this is a probable subroutine.
+	 * @return true if entryPoint is the probable subroutine start
 	 */
 	private boolean checkValidSubroutine(Address entryPoint, boolean allowExistingInstructions) {
 		return checkValidSubroutine(entryPoint, allowExistingInstructions, true);
 	}
 
+	/**
+	 * Check if there is a valid subroutine at the target address
+	 * 
+	 * @param entryPoint address to check
+	 * @param allowExistingInstructions true to allow running into existing instructions
+	 * @param mustTerminate true if the subroutine must hit a terminator (return) instruction
+	 * 
+	 * @return true if entryPoint is the probable subroutine start
+	 */
 	private boolean checkValidSubroutine(Address entryPoint, boolean allowExistingInstructions,
 			boolean mustTerminate) {
+
+		return checkValidSubroutine(entryPoint, allowExistingInstructions,
+			mustTerminate, false);
+	}
+
+	/**
+	 * Check if there is a valid subroutine at the target address
+	 * 
+	 * @param entryPoint address to check
+	 * @param allowExistingInstructions true to allow running into existing instructions
+	 * @param mustTerminate true if the subroutine must hit a terminator (return) instruction
+	 * @param requireContiguous true if the caller will require some number of contiguous instructions
+	 *        call getLastCheckValidInstructionCount() to get the initial number of contiguous instructions
+	 *        if this is true
+	 * 
+	 * @return true if entryPoint is the probable subroutine start
+	 */
+	public boolean checkValidSubroutine(Address entryPoint,
+			boolean allowExistingInstructions, boolean mustTerminate, boolean requireContiguous) {
+		
 		PseudoDisassemblerContext procContext = new PseudoDisassemblerContext(programContext);
 
 		return checkValidSubroutine(entryPoint, procContext, allowExistingInstructions,
-			mustTerminate);
+			mustTerminate, requireContiguous);
 	}
-	
+
+	/**
+	 * Check if there is a valid subroutine at the target address
+	 * 
+	 * @param entryPoint address to check
+	 * @param procContext processor context to use when pseudo disassembling instructions
+	 * @param allowExistingInstructions true to allow running into existing instructions
+	 * @param mustTerminate true if the subroutine must hit a terminator (return) instruction
+	 * 
+	 * @return true if entryPoint is the probable subroutine start
+	 */
 	public boolean checkValidSubroutine(Address entryPoint, PseudoDisassemblerContext procContext,
 			boolean allowExistingInstructions, boolean mustTerminate) {
 		return checkValidSubroutine(entryPoint, procContext, allowExistingInstructions, mustTerminate, false);
 	}
 
+	/**
+	 * Check if there is a valid subroutine at the target address
+	 * 
+	 * @param entryPoint address to check
+	 * @param procContext processor context to use when pseudo disassembling instructions
+	 * @param allowExistingInstructions true to allow running into existing instructions
+	 * @param mustTerminate true if the subroutine must hit a terminator (return) instruction
+	 * @param requireContiguous true if the caller will require some number of contiguous instructions
+	 *        call getLastCheckValidInstructionCount() to get the initial number of contiguous instructions
+	 *        if this is true
+	 * 
+	 * @return true if entryPoint is the probable subroutine start
+	 */
 	public boolean checkValidSubroutine(Address entryPoint, PseudoDisassemblerContext procContext,
 			boolean allowExistingInstructions, boolean mustTerminate, boolean requireContiguous) {
 		
 		AddressSet contiguousSet = new AddressSet();
 
 		lastCheckValidDisassemblyCount = 0;
-		
+	
 		if (!entryPoint.isMemoryAddress()) {
 			return false;
 		}

Ghidra/Processors/68000/src/main/java/ghidra/app/plugin/core/analysis/Motorola68KAnalyzer.java

@@ -304,6 +304,11 @@ public class Motorola68KAnalyzer extends ConstantPropagationAnalyzer {
 				return instruction.getMinAddress().equals(targetSwitchAddr);
 			}
 
+			@Override
+			public boolean evaluateReturn(Varnode retVN, VarnodeContext context, Instruction instruction) {
+				return false;
+			}
+			
 			@Override
 			public Long unknownValue(VarnodeContext context, Instruction instruction,
 					Varnode node) {

Ghidra/Processors/ARM/data/patterns/ARM_LE_patterns.xml

@@ -248,7 +248,19 @@
              0x01 0xbd </data> <!-- push {r0,r1} ; ldr r0,[dest] ; str r0, [sp, stack[-4]] ; pop {r0,pc} -->
       <align mark="0" bits="1"/>
       <setcontext name="TMode" value="1"/>
-      <funcstart validcode="function" thunk="true" /> <!-- must be something defined right before this -->
+      <funcstart thunk="true" />
   </pattern>
-  
+
+  <pattern> <!-- Thumb - thunk -->
+      <data> 0x10 0xb5   <!-- push     {r4,lr}  -->
+             0x02 0x4c   <!-- ldr      r4,[PTR_+0xc]  -->
+             0x24 0x68   <!-- ldr      r4,[r4,#0x0]  -->
+             0x01 0x94   <!-- str      r4,[sp,#local_4]  -->
+             0x10 0xbd   <!-- pop      {r4,pc}  -->
+      </data> 
+      <align mark="0" bits="1"/>
+      <setcontext name="TMode" value="1"/>
+      <funcstart thunk="true" />
+  </pattern>
+    
 </patternlist>

Ghidra/Processors/ARM/src/main/java/ghidra/app/plugin/core/analysis/ArmAnalyzer.java

@@ -132,6 +132,7 @@ public class ArmAnalyzer extends ConstantPropagationAnalyzer {
 						}
 
 					}
+
 					return false;
 				}
 
@@ -189,7 +190,6 @@ public class ArmAnalyzer extends ConstantPropagationAnalyzer {
 						// must disassemble right now, because TB flag could get set back at end of blx
 						doArmThumbDisassembly(program, instr, context, address, instr.getFlowType(),
 							true, monitor);
-						return false;
 					}
 
 					return super.evaluateReference(context, instr, pcodeop, address, size, refType);
@@ -211,6 +211,21 @@ public class ArmAnalyzer extends ConstantPropagationAnalyzer {
 					}
 					return false;
 				}
+				
+				@Override
+				public boolean evaluateReturn(Varnode retVN, VarnodeContext context, Instruction instruction) {
+					// check if a return is actually returning, or is branching with a constant PC
+
+					if (retVN != null && retVN.isConstant()) {
+						long offset = retVN.getOffset();
+						if (offset > 3 && offset != -1) {
+							// need to override the return to a branch
+							instruction.setFlowOverride(FlowOverride.BRANCH);
+						}
+					}
+					
+					return false;
+				}
 			};
 
 		AddressSet resultSet = symEval.flowConstants(flowStart, flowSet, eval, true, monitor);
@@ -399,7 +414,12 @@ public class ArmAnalyzer extends ConstantPropagationAnalyzer {
 			public boolean evaluateDestination(VarnodeContext context, Instruction instruction) {
 				return instruction.getMinAddress().equals(targetSwitchAddr);
 			}
-
+			
+			@Override
+			public boolean evaluateReturn(Varnode retVN, VarnodeContext context, Instruction instruction) {
+				return false;
+			}
+			
 			@Override
 			public Long unknownValue(VarnodeContext context, Instruction instruction,
 					Varnode node) {

Ghidra/Processors/ARM/src/test.slow/java/ghidra/app/plugin/core/analysis/ArmBranchReturnDetectionTest.java

@@ -0,0 +1,110 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * 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.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package ghidra.app.plugin.core.analysis;
+
+import static org.junit.Assert.*;
+
+import org.junit.*;
+
+import ghidra.app.plugin.core.analysis.AnalysisBackgroundCommand;
+import ghidra.app.plugin.core.analysis.AutoAnalysisManager;
+import ghidra.framework.cmd.Command;
+import ghidra.framework.options.Options;
+import ghidra.framework.plugintool.PluginTool;
+import ghidra.program.database.ProgramBuilder;
+import ghidra.program.model.address.AddressSet;
+import ghidra.program.model.data.DWordDataType;
+import ghidra.program.model.data.DataType;
+import ghidra.program.model.listing.*;
+import ghidra.program.model.symbol.SourceType;
+import ghidra.test.AbstractGhidraHeadedIntegrationTest;
+import ghidra.test.TestEnv;
+
+public class ArmBranchReturnDetectionTest extends AbstractGhidraHeadedIntegrationTest {
+
+	private TestEnv env;
+	private PluginTool tool;
+
+	private Program program;
+
+	private ProgramBuilder builder;
+
+	@Before
+	public void setUp() throws Exception {
+		env = new TestEnv();
+		tool = env.getTool();
+	}
+
+	@After
+	public void tearDown() {
+		if (program != null) {
+			env.release(program);
+		}
+		program = null;
+		env.dispose();
+	}
+
+	private void analyze() {
+		// turn off some analyzers
+		setAnalysisOptions("Stack");
+		setAnalysisOptions("Embedded Media");
+		setAnalysisOptions("DWARF");
+		setAnalysisOptions("Create Address Tables");
+
+		AutoAnalysisManager analysisMgr = AutoAnalysisManager.getAnalysisManager(program);
+		analysisMgr.reAnalyzeAll(null);
+
+		Command cmd = new AnalysisBackgroundCommand(analysisMgr, false);
+		tool.execute(cmd, program);
+		waitForBusyTool(tool);
+	}
+	
+
+	protected void setAnalysisOptions(String optionName) {
+		int txId = program.startTransaction("Analyze");
+		Options analysisOptions = program.getOptions(Program.ANALYSIS_PROPERTIES);
+		analysisOptions.setBoolean(optionName, false);
+		program.endTransaction(txId, true);
+	}
+	
+	/**
+	 * This tests that a pop to the pc with the lr register is changed to a return
+	 * 
+	 * That the thunking function can be found with the constant reference analyzer
+	 * 
+	 */
+	@Test
+	public void testDelayArmPopReturn1() throws Exception {
+
+		builder = new ProgramBuilder("thunk", ProgramBuilder._ARM);
+
+		builder.setBytes("0x00015d9c", "10 b5 03 48 10 bc 01 bc 00 47");
+		builder.setRegisterValue("TMode", "0x00015d9c", "0x00015d9c", 1);
+		builder.disassemble("0x00015d9c", 27, true);
+		
+		builder.createFunction("0x00015d9c");
+		builder.createLabel("0x00015d9c", "func1");;
+
+		program = builder.getProgram();
+
+		analyze();
+		
+		Instruction instruction = program.getListing().getInstructionAt(builder.addr(0x00015da4));
+		assertNotNull(instruction);
+		
+		assertTrue("pop turned into return", instruction.getFlowType().isTerminal());
+	}
+}

Ghidra/Processors/PowerPC/src/main/java/ghidra/app/plugin/core/analysis/PowerPCAddressAnalyzer.java

@@ -481,6 +481,11 @@ public class PowerPCAddressAnalyzer extends ConstantPropagationAnalyzer {
 				return instruction.getMinAddress().equals(targetSwitchAddr);
 			}
 
+			@Override
+			public boolean evaluateReturn(Varnode retVN, VarnodeContext context, Instruction instruction) {
+				return false;
+			}
+			
 			@Override
 			public Long unknownValue(VarnodeContext context, Instruction instruction,
 					Varnode node) {

Ghidra/Test/IntegrationTest/src/test.slow/java/ghidra/app/cmd/function/CreateFunctionThunkTest.java

@@ -25,9 +25,11 @@ import ghidra.framework.cmd.Command;
 import ghidra.framework.options.Options;
 import ghidra.framework.plugintool.PluginTool;
 import ghidra.program.database.ProgramBuilder;
+import ghidra.program.model.address.AddressSet;
+import ghidra.program.model.data.DWordDataType;
 import ghidra.program.model.data.DataType;
-import ghidra.program.model.listing.Function;
-import ghidra.program.model.listing.Program;
+import ghidra.program.model.listing.*;
+import ghidra.program.model.symbol.SourceType;
 import ghidra.test.AbstractGhidraHeadedIntegrationTest;
 import ghidra.test.TestEnv;
 
@@ -70,6 +72,7 @@ public class CreateFunctionThunkTest extends AbstractGhidraHeadedIntegrationTest
 		tool.execute(cmd, program);
 		waitForBusyTool(tool);
 	}
+	
 
 	protected void setAnalysisOptions(String optionName) {
 		int txId = program.startTransaction("Analyze");
@@ -128,4 +131,81 @@ public class CreateFunctionThunkTest extends AbstractGhidraHeadedIntegrationTest
 		assertEquals(true, isThunk.isThunk());
 		assertEquals("chdir", isThunk.getName());
 	}
+	
+	/**
+	 * This tests the forcing of a function to be a thunk with CreateThunkFunctionCmd
+	 * Tests that the Function start analyzer will create a thunk given the thunk tag on a matching function
+	 * That the ARM Thumb language has a thunking pattern.
+	 * 
+	 * That the thunking function can be found with the constant reference analyzer
+	 * 
+	 */
+	@Test
+	public void testArmThumbThunk() throws Exception {
+
+		builder = new ProgramBuilder("thunk", ProgramBuilder._ARM);
+
+		builder.setBytes("0x00015d9c", "00 00 00 00 03 b4 01 48 01 90 01 bd ad 5d 01 00 10 bd");
+		builder.setRegisterValue("TMode", "0x00015da0", "0x00015da0", 1);
+		builder.disassemble("0x00015da0", 27, true);
+
+		
+		builder.createFunction("0x00015da0");
+		builder.createLabel("0x15dac", "chdir");
+		builder.createFunction("0x15dac");
+
+		program = builder.getProgram();
+		
+		builder.applyDataType("0x00015d9c", DWordDataType.dataType);
+
+		analyze();
+		
+		
+		Instruction instruction = program.getListing().getInstructionAt(builder.addr(0x15dac));
+		assertNotNull(instruction);
+		
+
+		Function isThunk = program.getFunctionManager().getFunctionAt(builder.addr(0x00015da0));
+		assertEquals(true, isThunk.isThunk());
+		assertEquals("chdir", isThunk.getName());
+	}
+	
+	/**
+	 * This tests the forcing of a function to be a thunk with CreateThunkFunctionCmd
+	 * Tests that the Function start analyzer will create a thunk given the thunk tag on a matching function
+	 * That the ARM Thumb language has a thunking pattern.
+	 * 
+	 * That the thunking function can be found with the constant reference analyzer
+	 * 
+	 */
+	@Test
+	public void testArmThumbThunk2() throws Exception {
+
+		builder = new ProgramBuilder("thunk", ProgramBuilder._ARM);
+
+		builder.setBytes("0x10000", "10 b5 02 4c 24 68 01 94 10 bd 00 00 14 00 01 00 01 20 70 47 11 00 01 00");
+		builder.setRegisterValue("TMode", "0x10000", "0x10000", 1);
+		builder.disassemble("0x10000", 27, true);
+
+		
+		builder.createLabel("00010000", "thunker");
+		builder.createFunction("0x10000");
+		builder.createLabel("00010010", "thunkee");
+		builder.createFunction("00010010");
+
+		program = builder.getProgram();
+		
+		//builder.applyDataType("0x00015d9c", DWordDataType.dataType);
+
+		analyze();
+		
+		
+		Instruction instruction = program.getListing().getInstructionAt(builder.addr(0x10000));
+		assertNotNull(instruction);
+		
+
+		Function isThunk = program.getFunctionManager().getFunctionAt(builder.addr(0x10000));
+		assertEquals(true, isThunk.isThunk());
+		assertEquals("thunker", isThunk.getName());
+	}
 }