Ghidra: Decompiler output partly disappears after local var renaming

[GovanifY]

Describe the bug
When renaming certain local variables, not changing their type or whatnot, it is possible for the decompiler to remove other local variables and omit to decompile parts of the code.

To Reproduce
Analyze the file in the attachments(rename it to 102h.exe, github does not seem to allow exes attachments), go to 0x43f0ce, rename local_10 to whatever and see part of the program disappear.

Expected behavior
Renaming the variable not removing decompiled output.

Environment (please complete the following information):

• OS: 5.1.15-gentoo
• Java Version: openjdk 11.0.3-gentoo 2019-04-16
• Ghidra Version: 9.1_DEV (master at https://github.com/NationalSecurityAgency/ghidra/commit/15ee8040096d41f7e3f3044de950996737edd834 )

Attachments
102h.txt

[mumbel]

just adding more info, not familiar enough with those instructions.

Here is the diff in the debug function compilation, looks like the undefined4->float is the root issue. Its float local_10 in Decompile and undefined4 local_10 in listing, guessing the rename triggers that type change which makes the decompiler act differently on the pcode.

< <symbol name="local_10" typelock="false" namelock="true" readonly="false" cat="-1">
< <typeref name="undefined4"/></symbol>
---
> <symbol name="whatever" typelock="true" namelock="true" readonly="false" cat="-1">
> <typeref name="float"/></symbol>

[emteere]

If the local variable type can come from a parameter or return type, it is usually better to define it there. Not having looked at the example, that would be my first suggestion. Entering the type on a local is usually the last thing done to improve a funciton, as setting local types can get in the way of the decompiler data flow analysis.

[GovanifY]

@emteere the thing with this issue is that there is a discrepancy between the listing and decompiler view, and simply changing the name of the variable, not its type, forces it to assume one type. As such the issue triggers even when trying to leave a type alone, because of its nature.

[mumbel]

@GovanifY I wonder if the missing instructions might have been removed from dead code options. I'd guess local_10 is actually float[3]. As float, writes to local_c and local_8 might look like dead code.

[GovanifY]

Yup, it looks accurate; from what I've gathered this is a vector(3) normalizing function, and in_vec = &x (param_2 = &local_10) wouldn't make a lot of sense if the variable was not a float array.

[emteere]

Thanks for posting the example. I've duplicated the issue.

If you define local_10 as a float[3], then the code does decompile correctly for me in the main branch. If it doesn't for you, make sure you re-compiled the decompiler.

This is still a bug, we'll look into the root cause.

[GovanifY]

We figured that out later after having posted this bug report, thanks for looking into the bug!

[emteere]

Extracted function sample.

102h.bytes.txt
102h.xml.txt

[astrelsky]

I have noticed recently, at least with #874 that in the decompiler's function signature, "custom storage" is checked for some reason. It is only checked in the decompiler's function signature and not in the listings. Not sure if it is related to what is causing the bug or not.

[caheckman]

This should no longer be a problem in the master branch, and fixes will be in the 9.2 release. This is essentially the same problem as #524 . The renaming action forces the decompiler to incorrectly interpret a location as a distinct variable, when it is really a structure field or array element, which then results in faulty alias analysis.