Skip to content

Commit ff64da7

Browse files
skylotskylot
committed
fix: remove useless PHI for duplicate moves (#2813)
1 parent 00196e4 commit ff64da7

6 files changed

Lines changed: 174 additions & 11 deletions

File tree

‎jadx-core/src/main/java/jadx/core/dex/visitors/regions/TernaryMod.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,7 @@ private static void replaceWithTernary(MethodNode mth, IfRegion ifRegion, BlockN
335335
ternInsn.simplifyCondition();
336336

337337
InsnRemover.unbindAllArgs(mth, phiInsn);
338+
InsnRemover.delistPhi(mth, phiInsn);
338339
InsnRemover.unbindResult(mth, insn);
339340
InsnList.remove(block, insn);
340341
header.getInstructions().clear();

‎jadx-core/src/main/java/jadx/core/dex/visitors/ssa/SSATransform.java‎

Lines changed: 56 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,8 @@ private static void process(MethodNode mth) {
5757
renameVariables(mth);
5858
fixLastAssignInTry(mth);
5959
removeBlockerInsns(mth);
60-
markThisArgs(mth.getThisArg());
6160
tryToFixUselessPhi(mth);
61+
markThisArgs(mth.getThisArg());
6262
hidePhiInsns(mth);
6363
removeUnusedInvokeResults(mth);
6464
}
@@ -312,6 +312,20 @@ private static boolean fixPhiWithSameArgs(MethodNode mth, BlockNode block, PhiIn
312312
if (allSame) {
313313
return replacePhiWithMove(mth, block, phi, phi.getArg(0));
314314
}
315+
SSAVar sameVar = isSameMove(phi);
316+
if (sameVar != null) {
317+
RegisterArg sameArg = sameVar.getAssign().duplicate();
318+
if (inlinePhiInsn(mth, block, phi, sameArg)) {
319+
for (InsnArg arg : phi.getArguments()) {
320+
InsnNode moveInsn = ((RegisterArg) arg).getAssignInsn();
321+
if (moveInsn != null) {
322+
moveInsn.add(AFlag.REMOVE);
323+
InsnRemover.remove(mth, moveInsn);
324+
}
325+
}
326+
return true;
327+
}
328+
}
315329
return false;
316330
}
317331

@@ -330,6 +344,32 @@ private static boolean isSameArgs(PhiInsn phi) {
330344
return allSame;
331345
}
332346

347+
private static SSAVar isSameMove(PhiInsn phi) {
348+
SSAVar var = null;
349+
int argsCount = phi.getArgsCount();
350+
for (int i = 0; i < argsCount; i++) {
351+
RegisterArg arg = phi.getArg(i);
352+
if (arg.getSVar().getUseCount() != 1) {
353+
return null;
354+
}
355+
InsnNode assignInsn = arg.getAssignInsn();
356+
if (assignInsn == null || assignInsn.getType() != InsnType.MOVE) {
357+
return null;
358+
}
359+
InsnArg moveArg = assignInsn.getArg(0);
360+
if (!moveArg.isRegister()) {
361+
return null;
362+
}
363+
SSAVar moveVar = ((RegisterArg) moveArg).getSVar();
364+
if (var == null) {
365+
var = moveVar;
366+
} else if (var != moveVar) {
367+
return null;
368+
}
369+
}
370+
return var;
371+
}
372+
333373
private static boolean removePhiList(MethodNode mth, List<PhiInsn> insnToRemove) {
334374
for (BlockNode block : mth.getBasicBlocks()) {
335375
PhiListAttr phiList = block.get(AType.PHI_LIST);
@@ -372,7 +412,7 @@ private static boolean replacePhiWithMove(MethodNode mth, BlockNode block, PhiIn
372412
argVar.removeUsedInPhi(phi);
373413
}
374414
// try inline
375-
if (inlinePhiInsn(mth, block, phi)) {
415+
if (inlinePhiInsn(mth, block, phi, phi.getArg(0))) {
376416
insns.remove(phiIndex);
377417
} else {
378418
assign.removeUsedInPhi(phi);
@@ -387,29 +427,34 @@ private static boolean replacePhiWithMove(MethodNode mth, BlockNode block, PhiIn
387427
return true;
388428
}
389429

390-
private static boolean inlinePhiInsn(MethodNode mth, BlockNode block, PhiInsn phi) {
430+
private static boolean inlinePhiInsn(MethodNode mth, BlockNode block, PhiInsn phi, RegisterArg inlineArg) {
391431
SSAVar resVar = phi.getResult().getSVar();
392432
if (resVar == null) {
393433
return false;
394434
}
395-
RegisterArg arg = phi.getArg(0);
396-
if (arg.getSVar() == null) {
435+
if (inlineArg.getSVar() == null) {
397436
return false;
398437
}
399438
List<RegisterArg> useList = resVar.getUseList();
400439
for (RegisterArg useArg : new ArrayList<>(useList)) {
401440
InsnNode useInsn = useArg.getParentInsn();
402-
if (useInsn == null || useInsn == phi || useArg.getRegNum() != arg.getRegNum()) {
441+
if (useInsn == null || useInsn == phi) {
403442
return false;
404443
}
405-
// replace SSAVar in 'useArg' to SSAVar from 'arg'
406-
// no need to replace whole RegisterArg
407-
useArg.getSVar().removeUse(useArg);
408-
arg.getSVar().use(useArg);
444+
if (useArg.getRegNum() == inlineArg.getRegNum()) {
445+
// replace SSAVar in 'useArg' to SSAVar from 'arg'
446+
// no need to replace whole RegisterArg
447+
useArg.getSVar().removeUse(useArg);
448+
inlineArg.getSVar().use(useArg);
449+
} else {
450+
if (!useInsn.replaceArg(useArg, inlineArg)) {
451+
return false;
452+
}
453+
}
409454
}
410455
if (block.contains(AType.EXC_HANDLER)) {
411456
// don't inline into exception handler
412-
InsnNode assignInsn = arg.getAssignInsn();
457+
InsnNode assignInsn = inlineArg.getAssignInsn();
413458
if (assignInsn != null && !assignInsn.isConstInsn()) {
414459
assignInsn.add(AFlag.DONT_INLINE);
415460
}

‎jadx-core/src/main/java/jadx/core/utils/DebugChecks.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ public static void checkMethod(MethodNode mth) {
6767
}
6868
}
6969
checkSSAVars(mth);
70+
quickCheckPhiInsn(mth);
7071
// checkPHI(mth);
7172
}
7273

@@ -228,6 +229,34 @@ private static void checkRegisterArg(MethodNode mth, RegisterArg reg) {
228229
}
229230
}
230231

232+
public static void quickCheckPhiInsn(MethodNode mth) {
233+
if (mth.getSVars().isEmpty()) {
234+
return;
235+
}
236+
for (BlockNode block : mth.getBasicBlocks()) {
237+
PhiListAttr phiListAttr = block.get(AType.PHI_LIST);
238+
if (phiListAttr != null) {
239+
for (PhiInsn phiInsn : phiListAttr.getList()) {
240+
checkPhiArg(mth, phiInsn, phiInsn.getResult(), () -> "result");
241+
int argsCount = phiInsn.getArgsCount();
242+
for (int i = 0; i < argsCount; i++) {
243+
int argNum = i;
244+
checkPhiArg(mth, phiInsn, phiInsn.getArg(argNum), () -> "arg_" + argNum);
245+
}
246+
}
247+
}
248+
}
249+
}
250+
251+
private static void checkPhiArg(MethodNode mth, PhiInsn phiInsn, RegisterArg arg, Supplier<String> argName) {
252+
if (arg == null) {
253+
throw new JadxRuntimeException("Null " + argName.get() + " in PHI insn: " + phiInsn);
254+
}
255+
if (arg.getSVar() == null) {
256+
throw new JadxRuntimeException("Null SSA variable in " + argName.get() + " in PHI insn: " + phiInsn);
257+
}
258+
}
259+
231260
private static void checkPHI(MethodNode mth) {
232261
for (BlockNode block : mth.getBasicBlocks()) {
233262
List<PhiInsn> phis = new ArrayList<>();

‎jadx-core/src/main/java/jadx/core/utils/InsnRemover.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010

1111
import jadx.core.Consts;
1212
import jadx.core.dex.attributes.AFlag;
13+
import jadx.core.dex.attributes.AType;
14+
import jadx.core.dex.attributes.nodes.PhiListAttr;
1315
import jadx.core.dex.instructions.InsnType;
1416
import jadx.core.dex.instructions.PhiInsn;
1517
import jadx.core.dex.instructions.args.InsnArg;
@@ -277,4 +279,13 @@ public static void remove(MethodNode mth, BlockNode block, int index) {
277279
unbindInsn(mth, instructions.get(index));
278280
instructions.remove(index);
279281
}
282+
283+
public static void delistPhi(MethodNode mth, PhiInsn phiInsn) {
284+
for (BlockNode block : mth.getBasicBlocks()) {
285+
PhiListAttr phiListAttr = block.get(AType.PHI_LIST);
286+
if (phiListAttr != null) {
287+
phiListAttr.getList().removeIf(i -> i == phiInsn);
288+
}
289+
}
290+
}
280291
}

‎jadx-core/src/test/java/jadx/tests/integration/variables/TestThisBranchDup.java‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
package jadx.tests.integration.variables;
2+
3+
import org.junit.jupiter.api.Test;
4+
5+
import jadx.tests.api.SmaliTest;
6+
7+
import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat;
8+
9+
public class TestThisBranchDup extends SmaliTest {
10+
11+
@Test
12+
public void test() {
13+
disableCompilation();
14+
assertThat(getClassNodeFromSmali())
15+
.code()
16+
.containsOne("this(") // constructor type correctly detected
17+
.countString(6, "(i & "); // ternary used and inlined
18+
}
19+
}

‎jadx-core/src/test/smali/variables/TestThisBranchDup.smali‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
.class public final Lvariables/TestThisBranchDup;
2+
.super Ljava/lang/Object;
3+
4+
.method public constructor <init>(ZZZLh3/t;ZZILkotlin/jvm/internal/DefaultConstructorMarker;)V
5+
.registers 10
6+
7+
and-int/lit8 p8, p7, 0x1
8+
if-eqz p8, :cond_5
9+
const/4 p1, 0x0
10+
11+
:cond_5
12+
and-int/lit8 p8, p7, 0x2
13+
const/4 v0, 0x1
14+
if-eqz p8, :cond_b
15+
move p2, v0
16+
17+
:cond_b
18+
and-int/lit8 p8, p7, 0x4
19+
if-eqz p8, :cond_10
20+
move p3, v0
21+
22+
:cond_10
23+
and-int/lit8 p8, p7, 0x8
24+
if-eqz p8, :cond_16
25+
.line 11
26+
sget-object p4, Lh3/t;->Inherit:Lh3/t;
27+
28+
:cond_16
29+
and-int/lit8 p8, p7, 0x10
30+
if-eqz p8, :cond_1b
31+
move p5, v0
32+
33+
:cond_1b
34+
and-int/lit8 p7, p7, 0x20
35+
if-eqz p7, :cond_27
36+
move p8, v0
37+
move-object p6, p4
38+
move p7, p5
39+
move p4, p2
40+
move p5, p3
41+
move-object p2, p0
42+
move p3, p1
43+
goto :goto_2e
44+
45+
:cond_27
46+
move p8, p6
47+
move p7, p5
48+
move p5, p3
49+
move-object p6, p4
50+
move p3, p1
51+
move p4, p2
52+
move-object p2, p0
53+
54+
.line 12
55+
:goto_2e
56+
invoke-direct/range {p2 .. p8}, Lvariables/TestThisBranchDup;-><init>(ZZZLh3/t;ZZ)V
57+
return-void
58+
.end method

0 commit comments

Comments
 (0)