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.
Ghidra Version: latest master at the time of writing (fb844be1f61d71063b1e392f319d352cdd19b962) with PR #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.
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();
}
Describe the bug Sometimes, when
RuleIgnoreNan
tries to removeNaN
-branches from conditionals, it is too aggressive and also removes other branches, leading to incorrect decompiler output.To Reproduce Steps to reproduce the behavior:
decomp_dbg
decomp_dbg
restore /path/to/fcomp_error_constant.xml
and press enterload function FUN_0000000c
and press enterdecompile
and press enterprint C
and press enterif
statement looks likeFLOAT_00000008 == 1.5
instead ofFLOAT_00000008 <= 1.5
.Expected behavior I expected the guard of the
if
statement to look more likeFLOAT_00000008 <= 1.5
.Attachments fcomp_error_constant.xml
Environment
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 betweenZF
andr0x00000008(i) < #0x3fc00000
by detaching theu0x10000015:1
varnode fromZF
.After the change to
u0x10000015:1
, theNAN
flow has already been removed fromu0x10000015:1
, sou0x10000015:1
does not need to be removed fromZF
. In fact, removing it fromZF
is wrong, since it still contains the important flow fromr0x00000008(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
orBOOL_OR
) contains a comparison withNAN
. If so, it increments a counter and changes the binary operator to aCOPY
of the not-NAN flow. Finally, it returns the outputVarnode
of the operator.https://github.com/NationalSecurityAgency/ghidra/blob/fb844be1f61d71063b1e392f319d352cdd19b962/Ghidra/Features/Decompiler/src/decompile/cpp/ruleaction.cc#L9496-L9508
However, the calling code seems to interpret a non-null return value to mean "the
NAN
flow continues into thisVarnode
". If the operation was changed to aCOPY
however, theNAN
flow no longer flows into the returnedVarnode
. As such, I thinkRuleIgnoreNan::testForComparison
should return a null pointer if it changed the opcode to aCOPY
.Locally, I applied the following patch, which gives the correct output for the case above - the decompiled output now correctly uses
<=
.