Merge remote-tracking branch 'origin/GT-3150_James_add_override_warnings' into Ghidra_9.1

Author: ryanmkurtz

Ghidra/Features/Base/src/main/help/help/topics/ReferencesPlugin/References_from.htm

@@ -742,8 +742,10 @@ <H2><A name="refTypes"></A>Ref-Types<BR>
             <TD valign="top">Used to change CALLOTHER pcode operations to CALL operations. The new call target is the "to" address of the 
             reference.  The override only takes effect when the reference is primary, and only when there is exactly one primary 
             CALLOTHER_OVERRIDE_CALL reference at the "from" address of the reference. Only the first CALLOTHER operation at the "from"
-            address of the reference is changed.  Note that this reference override takes precedence over those of CALLOTHER_OVERRIDE_JUMP
-            references. <BR>
+            address of the reference is changed. <EM> Applying this override to instances of a CALLOTHER op that have output is not recommended 
+            and can adversely affect decompilation. </EM>  You can see whether a particular instance has an output by enabling the "PCode" field 
+            of the Listing. Note that this reference override takes precedence over those of CALLOTHER_OVERRIDE_JUMP
+            references. <BR> 
             </TD>
           </TR>
 
@@ -760,7 +762,8 @@ <H2><A name="refTypes"></A>Ref-Types<BR>
             <TD valign="top">Used to change CALLOTHER pcode operations to BRANCH operations. The new jump target is the "to" address of the 
             reference.  The override only takes effect when the reference is primary, and only when there is exactly one primary 
             CALLOTHER_OVERRIDE_JUMP reference at the "from" address of the reference. Only the first CALLOTHER operation at the "from"
-            address of the reference is changed. <BR>
+            address of the reference is changed. <EM> Applying this override to an instance of a CALLOTHER op with output is not recommended </EM> (see
+            the discussion above about CALLOTHER_OVERRIDE_CALL references).<BR>
             </TD>
           </TR>
 

Ghidra/Features/Base/src/main/java/ghidra/app/util/viewer/field/PostCommentFieldFactory.java

@@ -223,7 +223,13 @@ private String[] getAutoPostComment(CodeUnit cu) {
 				if (overrideData.hasMultipleCallOthers()) {
 					comments.addFirst("-- WARNING: additional CALLOTHER ops present");
 				}
-				comments.addFirst(callOtherCallOverrideComment);
+				String outputWarningString = overrideData.getOutputWarningString();
+				if (outputWarningString != null) {
+					comments.addFirst(outputWarningString);
+				}
+				else {
+					comments.addFirst(callOtherCallOverrideComment);
+				}
 			}
 			else {
 				overrideData =
@@ -237,7 +243,13 @@ private String[] getAutoPostComment(CodeUnit cu) {
 					if (overrideData.hasMultipleCallOthers()) {
 						comments.addFirst("-- WARNING: additional CALLOTHER ops present");
 					}
-					comments.addFirst(callOtherJumpOverrideComment);
+					String outputWarningString = overrideData.getOutputWarningString();
+					if (outputWarningString != null) {
+						comments.addFirst(outputWarningString);
+					}
+					else {
+						comments.addFirst(callOtherJumpOverrideComment);
+					}
 				}
 			}
 		}
@@ -596,13 +608,19 @@ else if (type.equals(RefType.CALLOTHER_OVERRIDE_CALL) ||
 		boolean hasMultipleCallOthers = false;
 		//used to report the name of the CALLOTHER op that is overridden
 		String callOtherName = null;
+		String outputWarningString = null;
 		for (PcodeOp op : pcodeOps) {
 			if (ops.contains(op.getOpcode())) {
 				hasAppropriatePcodeOp = true;
 				if (op.getOpcode() == PcodeOp.CALLOTHER) {
 					if (callOtherName == null) {
 						callOtherName = inst.getProgram().getLanguage().getUserDefinedOpName(
 							(int) op.getInput(0).getOffset());
+						if (op.getOutput() != null) {
+							outputWarningString =
+								"WARNING: Output of " + callOtherName +
+									" destroyed by override!";
+						}
 					}
 					else {
 						hasMultipleCallOthers = true;
@@ -618,21 +636,22 @@ else if (type.equals(RefType.CALLOTHER_OVERRIDE_CALL) ||
 		if (type.equals(RefType.CALL_OVERRIDE_UNCONDITIONAL)) {
 			Address ref = pcodeOverride.getOverridingReference(type);
 			if (ref != null) {
-				return new OverrideCommentData(ref, null, false);
+				return new OverrideCommentData(ref, null, false, outputWarningString);
 			}
 			return null;
 		}
 		if (type.equals(RefType.JUMP_OVERRIDE_UNCONDITIONAL)) {
 			Address ref = pcodeOverride.getOverridingReference(type);
 			if (ref != null) {
-				return new OverrideCommentData(ref, null, false);
+				return new OverrideCommentData(ref, null, false, outputWarningString);
 			}
 			return null;
 		}
 		if (type.equals(RefType.CALLOTHER_OVERRIDE_CALL)) {
 			Address ref = pcodeOverride.getOverridingReference(type);
 			if (ref != null) {
-				return new OverrideCommentData(ref, callOtherName, hasMultipleCallOthers);
+				return new OverrideCommentData(ref, callOtherName, hasMultipleCallOthers,
+					outputWarningString);
 			}
 			return null;
 		}
@@ -645,20 +664,23 @@ else if (type.equals(RefType.CALLOTHER_OVERRIDE_CALL) ||
 		if (ref == null) {
 			return null;
 		}
-		return new OverrideCommentData(ref, callOtherName, hasMultipleCallOthers);
+		return new OverrideCommentData(ref, callOtherName, hasMultipleCallOthers,
+			outputWarningString);
 
 	}
 
 	private class OverrideCommentData {
 		private Address overridingRef;
 		private String overriddenCallOther;
 		private boolean hasMultipleCallOthers;
+		private String outputWarningString = null;
 
 		OverrideCommentData(Address overridingRef, String overriddenCallOther,
-				boolean multipleCallOthers) {
+				boolean multipleCallOthers, String outputWarningString) {
 			this.overridingRef = overridingRef;
 			this.overriddenCallOther = overriddenCallOther;
 			this.hasMultipleCallOthers = multipleCallOthers;
+			this.outputWarningString = outputWarningString;
 		}
 
 		Address getOverridingRef() {
@@ -673,6 +695,9 @@ boolean hasMultipleCallOthers() {
 			return hasMultipleCallOthers;
 		}
 
+		String getOutputWarningString() {
+			return outputWarningString;
+		}
 	}
 
 }

Ghidra/Features/Base/src/test.slow/java/ghidra/app/util/viewer/field/PostCommentFieldFactoryTest.java

@@ -161,6 +161,12 @@ private ProgramDB buildProgram() throws Exception {
 		builder.disassemble("100d000", 2);
 		builder.createReturnInstruction("100d002");
 
+		builder.createEmptyFunction("override_warning", "0x100e000", 10, null);
+		builder.setBytes("100e000", "a6 00");
+		builder.disassemble("100e000", 2);
+		builder.createReturnInstruction("100e002");
+		builder.createEmptyFunction("call_dest_12", "0x100e020", 10, null);
+
 
 		return builder.getProgram();
 	}
@@ -1007,6 +1013,57 @@ public void testMultipleOps2() {
 			tf.getText());
 	}
 
+	@Test
+	public void testOverrideWarnings() {
+		assertFalse(cb.goToField(addr("100e000"), PostCommentFieldFactory.FIELD_NAME, 0, 1));
+		//verify CALLOTHER_OVERRIDE_CALL warning
+		int transactionID = program.startTransaction("call_warning");
+		ReferenceManager refManager = program.getReferenceManager();
+		Reference ref = null;
+		try {
+			ref = refManager.addMemoryReference(addr("100e000"), addr("100e020"),
+				RefType.CALLOTHER_OVERRIDE_CALL, SourceType.ANALYSIS, 0);
+			refManager.setPrimary(ref, true);
+		}
+		finally {
+			program.endTransaction(transactionID, true);
+		}
+		assertTrue(cb.goToField(addr("100e000"), PostCommentFieldFactory.FIELD_NAME, 0, 1));
+		ListingField tf = cb.getCurrentField();
+		assertEquals("WARNING: Output of pcodeop_one destroyed by override!", tf.getText());
+		//set ref non-primary, verify that warning goes away
+		transactionID = program.startTransaction("turn_off_call_warning");
+		try {
+			refManager.setPrimary(ref, false);
+		}
+		finally {
+			program.endTransaction(transactionID, true);
+		}
+		assertFalse(cb.goToField(addr("100e000"), PostCommentFieldFactory.FIELD_NAME, 0, 1));
+		//verify CALLOTHER_OVERRIDE_JUMP warning
+		transactionID = program.startTransaction("jump_warning");
+		try {
+			ref = refManager.addMemoryReference(addr("100e000"), addr("100e020"),
+				RefType.CALLOTHER_OVERRIDE_JUMP, SourceType.ANALYSIS, 0);
+			refManager.setPrimary(ref, true);
+		}
+		finally {
+			program.endTransaction(transactionID, true);
+		}
+		assertTrue(cb.goToField(addr("100e000"), PostCommentFieldFactory.FIELD_NAME, 0, 1));
+		assertEquals("WARNING: Output of pcodeop_one destroyed by override!", tf.getText());
+		//set ref non-primary, verify that warning goes away
+		transactionID = program.startTransaction("turn_off_jump_warning");
+		try {
+			refManager.setPrimary(ref, false);
+		}
+		finally {
+			program.endTransaction(transactionID, true);
+		}
+		assertFalse(cb.goToField(addr("100e000"), PostCommentFieldFactory.FIELD_NAME, 0, 1));
+
+	}
+
 	private void setCommentInFunction(Function function, String comment) {
 		CodeUnit cu = program.getListing().getCodeUnitAt(function.getEntryPoint());
 		int transactionID = program.startTransaction("test");

Ghidra/Processors/Toy/data/languages/toyInstructions.sinc

@@ -55,10 +55,12 @@
 #  1111 1nnn nnnn nnnn  # call n             call n
 #
 #### user-defined
-#  1010 0010 ssss 0000  # user_one rs        user_one rs
-#  1010 0010 ssss 0000  # user_two rs        user_two rs
-#  1010 0011 0000 0000  # user_three         user_three 
-#  1010 0100 ssss tttt  # user_four rs rt    user_four rs rt              
+#  1010 0010 ssss 0000  # user_one rs       user_one rs
+#  1010 0010 ssss 0000  # user_two rs       user_two rs
+#  1010 0011 0000 0000  # user_three        user_three 
+#  1010 0100 ssss tttt  # user_four rs rt   user_four rs rt       
+#  1010 0101 nnnn nnnn  # user_five n       user_five n  
+#  1010 0110 ssss 0000  # user_six rs       user_six rs
 #
 #### RESERVED
 #  1101 1001 xxxx xxxx  # RESERVED BANK
@@ -218,3 +220,5 @@ define pcodeop pcodeop_three;
 :user_three is $(INSTR_PHASE) op1215=0xa & op0811=0x03 & op0007=0x0                { pcodeop_three();}
 :user_four rs rt is $(INSTR_PHASE) op1215=0xa & op0811=0x04 & rs & rt              { pcodeop_one(rs); call [rt]; pcodeop_three();}  
 :user_five Rel8 is $(INSTR_PHASE) op1215=0xa & op0811=0x05 & Rel8                  { lr = inst_next; call Rel8; pcodeop_three();} 
+:user_six rs is $(INSTR_PHASE) op1215=0xa & op0811=0x06 & rs & op0003=0x0          { r1 = pcodeop_one(rs); call [r1];}
+