Decompiler: Improve support of in-place C operations #8635
+159
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit is based off of an initial patch made by @RootCubed here: NationalSecurityAgency#5601 (comment)
This commit adds support for in-place operations on values in memory using a LOAD/OP/STORE sequence. The implementation is quite hacky, for two main reasons: 1. CPUI_STORE was never meant as going through PrintC::emitInplaceOp, so it takes a bit of a weird path through the function, which makes the function quite unreadable. 2. If the CPUI_STORE is emitted as an in-place op, there might still be a spurious CPUI_LOAD left, which has already been emitted. If the loaded value is not used elsewhere, the load is now useless. To avoid emitting this load, we need to also detect the structure when the load instruction goes through PrintC::emitInplaceOp. And if we detect the structure, we need to not emit anything. Also avoiding the semi- colon and newline being printed requires abusing the print modifiers by repurposing the 'pending_brace' modifier to also indicate that the previous expression did not emit anything.
ryanmkurtz
added
Feature: Decompiler
Status: Triage
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #7283
These two commits improve the support of in-place operations by
LOADed, then modified using an in-place operation, and finallySTOREd to the same location.Especially the second commit is quite hacky, since
STOREoperations were never meant to be passed toPrintC::emitInplaceOp, and adding support for it adds some extra complexity. The ugliest hack is caused by theLOADin the sequence however, since thatLOADneeds to not be emitted when the loaded value was only used for the in-place operation. This basically requires detecting the same pattern twice - once from theSTOREperspective, and once from theLOADperspective. Also signalling that nothing has been emitted to the other parts of the C-genereration code is done in a very hacky way by abusing thepending_braceprint modifier and then checking for that modifier in several places.I'm afraid especially the second commit disqualifies this PR from ever being merged, but since it seems to work decently well, I hope it'll be useful to some people running their own builds of Ghidra.