ghidra
ghidra copied to clipboard
NationalSecurityAgency
Decompiler produces incorrect code for 80C51 variant
Describe the bug The decompiler ignores a lot of pin assignments for the following firmware. firmware.zip
To Reproduce Steps to reproduce the behavior:
- load firmware; select 8051/default/default for language
- go to address 0x0000 and hit 'd' to start disassembly
- jump to address 0x6907
Expected behavior A lot of the assembly instructions should be included in the decompilation. The repeated setting and clearing of P1.5, for example, toggles a pin on the microcontroller.
Environment (please complete the following information):
- OS: Ubuntu 20.04
- Java Version: openjdk 11.0.11 2021-04-20
- Ghidra Version: 9.2 as well as 10.0_DEV
- Ghidra Origin: built from git sources
See my comments in #1276 about this - quick workaround is to edit the memory areas to set "SFR-BITS" to volatile.
The bytes at the bit locations are not seen by the decompiler as affecting anything. So the value is trimmed as not needed. It is an easy manual fix. You can go to the P1.1-P1.7 bit locations, create a byte at each location and then change the data settings mutability to "volatile". You can also set the volatile flag on an entire memory block.
The volatile attribute can be set in the processor specification .pspec file if the location should always be considered volatile, say a write only output register, a memory mapped IO port, or a timer register.
There is a really nice new improvement to the decompiler in the master branch and coming out soon in the next Ghidra release.
the read_volatile() and write_volatile() have been changed to normal C assignment syntax. The volatile accesses are colored to indicate the volatile nature of the read/write. With this change, putting volatile in any of the above ways is more palatable, as it doesn't munge up the data flow.
After setting the volatility flag as I've described, the function looks like this:
in 10.1.5 there would be many read_volatile(P1.x) and write_volatile(P1.x, value) pseudo function calls.
Cool, looking forward to try that. I would argue that for 8051 and many other mcus, the .pspec files should by default have this kind of IO reg as volatile. I can't see any scenario where it makes sense for them not to be volatile, whether reading or writing...
Cool, looking forward to try that. I would argue that for 8051 and many other mcus, the .pspec files should by default have this kind of IO reg as volatile. I can't see any scenario where it makes sense for them not to be volatile, whether reading or writing...
I want to say thank you @emteere for that very helpful post.
And I want to second the point of making volatile the default.
I think this change makes it an easier choice to specify what IO registers should be marked volatile. Previously it could make a mess of the decompiler output and data flow and was used sparingly.
Right now you can only make large address space areas volatile by address range, not individual register, or manually as described above.
There is an in progress change to allow a volatile tag on each individual entry in the .pspec file, so it would be good to wait until that change makes into into the baseline to make suggestions/PR. This one area that someone who knows the processor would know best what should be volatile and what should not and add these tags to the .pspec file, either as an entire range, or individually.
We can leave this open until after 10.2 goes out and the volatile tag is added. Then please feel free to submit a PR that tags the registers volatile individually, or a range or registers as it makes sense to do so.
In the current 10.2.x version you can also edit your 8051.pspec file from:
<volatile outputop="write_volatile" inputop="read_volatile">
<range space="SFR" first="0x0" last="0xFF"/>
</volatile>
to:
<volatile outputop="write_volatile" inputop="read_volatile">
<range space="SFR" first="0x0" last="0xFF"/>
<range space="BITS" first="0x80" last="0xFF"/>
</volatile>
We'll make this change in patch. It doesn't seem that making this change causes issues, unless there is a register in the group that should be non-volatile.
The changes in a new release will allow you to set a range of locations to volatile as above, and then set individual locations in the .pspec file to non-volatile, or to set locations individually to volatile if there are only a few instead of a range.
We'll make this change in patch.
Can you confirm, do you already have an internal ticket# tracking this or are you waiting for a PR here ?
The work on this ticket is complete. See the above commit associated with the ticket closing.
