Decompiler: RuleIgnoreNan is too aggressive when removing NaN branches

[LukeSerne]

Describe the bug
Sometimes, when RuleIgnoreNan tries to remove NaN-branches from conditionals, it is too aggressive and also removes other branches, leading to incorrect decompiler output.

To Reproduce
Steps to reproduce the behavior:

1. Apply PR Decompiler: Simplify comparisons between INT_OR and zero. #6578
2. Compile decomp_dbg
3. Download fcomp_error_constant.xml, originally from issue Decompiler erroneously swapping operators without changing the operand in floating-point comparisons where data is marked constant (x86) #6528.
4. Run decomp_dbg
5. Type restore /path/to/fcomp_error_constant.xml and press enter
6. Type load function FUN_0000000c and press enter
7. Type decompile and press enter
8. Type print C and press enter
9. Observe that the guard of the if statement looks like FLOAT_00000008 == 1.5 instead of FLOAT_00000008 <= 1.5.

Expected behavior
I expected the guard of the if statement to look more like FLOAT_00000008 <= 1.5.

Attachments
fcomp_error_constant.xml

Environment

• OS: Ubuntu 22.04
• Ghidra Version: latest master at the time of writing (fb844be) with PR Decompiler: Simplify comparisons between INT_OR and zero. #6578
• Ghidra Origin: locally built

Additional context
Looking into the problem using DecompVis, I found that the cause of this issue is an application of the rule RuleIgnoreNan, as shown by this screenshot. The change outlined in red is incorrect. It also removes the connection between ZF and r0x00000008(i) < #0x3fc00000 by detaching the u0x10000015:1 varnode from ZF.

After the change to u0x10000015:1, the NAN flow has already been removed from u0x10000015:1, so u0x10000015:1 does not need to be removed from ZF. In fact, removing it from ZF is wrong, since it still contains the important flow from r0x00000008(i) < #0x3fc00000.

Looking through the relevant code, it seems that the error is in RuleIgnoreNan::testForComparison. This function tests if the given binary operator (BOOL_AND or BOOL_OR) contains a comparison with NAN. If so, it increments a counter and changes the binary operator to a COPY of the not-NAN flow. Finally, it returns the output Varnode of the operator.

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

Lines 9496 to 9508 in fb844be

	Varnode *RuleIgnoreNan::testForComparison(Varnode *floatVar,PcodeOp *op,int4 slot,OpCode matchCode,int4 &count,Funcdata &data)
	{
	if (op->code() == matchCode) {
	Varnode *vn = op->getIn(1-slot);
	if (checkBackForCompare(floatVar,vn)) {
	data.opSetOpcode(op, CPUI_COPY);
	data.opRemoveInput(op, 1);
	data.opSetInput(op, vn, 0);
	count += 1;
	}
	return op->getOut();
	}

However, the calling code seems to interpret a non-null return value to mean "the NAN flow continues into this Varnode". If the operation was changed to a COPY however, the NAN flow no longer flows into the returned Varnode. As such, I think RuleIgnoreNan::testForComparison should return a null pointer if it changed the opcode to a COPY.

Locally, I applied the following patch, which gives the correct output for the case above - the decompiled output now correctly uses <=.

diff --git a/Ghidra/Features/Decompiler/src/decompile/cpp/ruleaction.cc b/Ghidra/Features/Decompiler/src/decompile/cpp/ruleaction.cc
index 51e3313ec9..f04ca39000 100644
--- a/Ghidra/Features/Decompiler/src/decompile/cpp/ruleaction.cc
+++ b/Ghidra/Features/Decompiler/src/decompile/cpp/ruleaction.cc
@@ -9503,6 +9503,8 @@ Varnode *RuleIgnoreNan::testForComparison(Varnode *floatVar,PcodeOp *op,int4 slo
       data.opRemoveInput(op, 1);
       data.opSetInput(op, vn, 0);
       count += 1;
+      // NaN flow has been removed from this comparison - don't descend further
+      return (Varnode *)0;
     }
     return op->getOut();
   }