fix decompiler assignment optimization issue #17008 #17120
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
junxzm1990
force-pushed
the
jun/decompiler-issue-17087
branch
from
c90d427 to
2d404c8
Compare
junxzm1990
force-pushed
the
jun/decompiler-assignment-opt
branch
2 times, most recently
from
31332e9 to
1a86009
Compare
junxzm1990
force-pushed
the
jun/decompiler-issue-17087
branch
from
2d404c8 to
e6cf841
Compare
junxzm1990
force-pushed
the
jun/decompiler-assignment-opt
branch
from
1a86009 to
6bacdef
Compare
junxzm1990
force-pushed
the
jun/decompiler-issue-17087
branch
from
e6cf841 to
5cc0a94
Compare
junxzm1990
force-pushed
the
jun/decompiler-assignment-opt
branch
from
6bacdef to
1d06027
Compare
junxzm1990
force-pushed
the
jun/decompiler-issue-17087
branch
from
5cc0a94 to
c60be0d
Compare
junxzm1990
force-pushed
the
jun/decompiler-assignment-opt
branch
from
1d06027 to
9b9a80d
Compare
junxzm1990
force-pushed
the
jun/decompiler-issue-17087
branch
from
c60be0d to
f7078a2
Compare
junxzm1990
force-pushed
the
jun/decompiler-assignment-opt
branch
from
9b9a80d to
8f239c3
Compare
| impl AssignTransformer<'_> { | ||
| /// Check if an expression is safe to eliminate, assuming its result is never used. | ||
| /// [TODO]: refine the rules to consider other side-effect-carrying operations. | ||
| fn safe_to_eliminate(&self, exp: &Exp) -> bool { |
There was a problem hiding this comment.
I think this leaves out whole lot of cases: e.g., addition would be considered safe to eliminate (even though it is not), blocks with side-effects would be disregarded.
I wonder if a a better approach would be to have an allow-list for those constructs that are known to be safe: e.g., Value, LocalVar, Temporary, etc. Otherwise, it would lead to subtle bugs.
There was a problem hiding this comment.
Agreed. Using an allow-list is easier to maintain safety. At this stage, the AST is fairly simple: it should not have nested expressions and complex ones like blocks. So, by even just considering Value, LocalVar, and Temporary, the optimization effects should largely remain. Let me try.
There was a problem hiding this comment.
Done. It matched my expectations.
junxzm1990
force-pushed
the
jun/decompiler-assignment-opt
branch
from
8f239c3 to
c75710e
Compare
Description
The decompiler optimizes the following pattern
t1 = rhs; ... use(t1)===>
... use(rhs)If
t1is never used (i.e.,use(t1)does not exist), the decompiler will eliminatet1 = rhsentirely. This is not safe asrhscan have side effects (e.g.,rhsbeing a function call). This PR fixes this issue by checking ifrhscarries potential side effects. Currently, it only checks ifrhsis aCall(Operation::MoveFunction)orInvoke.Close issue #17008.
How Has This Been Tested?
Type of Change
Which Components or Systems Does This Change Impact?