Skip to content

Commit

Permalink
fix: better code styling for if-else blocks (#1455)
Browse files Browse the repository at this point in the history
  • Loading branch information
skylot committed Apr 26, 2022
1 parent 3366bf3 commit a71b3a7
Show file tree
Hide file tree
Showing 17 changed files with 753 additions and 115 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ public enum AFlag {
RERUN_SSA_TRANSFORM,

METHOD_CANDIDATE_FOR_INLINE,
USE_LINES_HINTS, // source lines info in methods can be trusted

DISABLE_BLOCKS_LOCK,

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,16 @@ public List<InsnNode> collectInsns() {
return list;
}

public int getSourceLine() {
for (InsnNode insn : collectInsns()) {
int line = insn.getSourceLine();
if (line != 0) {
return line;
}
}
return 0;
}

@Nullable
public InsnNode getFirstInsn() {
if (mode == Mode.COMPARE) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
package jadx.core.dex.visitors.debuginfo;

import java.util.HashMap;
import java.util.List;
import java.util.Map;

import jadx.api.plugins.input.data.IDebugInfo;
import jadx.api.plugins.input.data.ILocalVar;
import jadx.core.dex.attributes.AFlag;
import jadx.core.dex.attributes.nodes.LocalVarsDebugInfoAttr;
import jadx.core.dex.attributes.nodes.RegDebugInfoAttr;
import jadx.core.dex.instructions.InsnType;
import jadx.core.dex.instructions.args.ArgType;
import jadx.core.dex.instructions.args.InsnArg;
import jadx.core.dex.instructions.args.RegisterArg;
Expand All @@ -17,6 +20,7 @@
import jadx.core.dex.visitors.JadxVisitor;
import jadx.core.dex.visitors.blocks.BlockSplitter;
import jadx.core.dex.visitors.ssa.SSATransform;
import jadx.core.utils.ListUtils;
import jadx.core.utils.exceptions.JadxException;

@JadxVisitor(
Expand Down Expand Up @@ -52,17 +56,30 @@ private void attachSourceLines(MethodNode mth, Map<Integer, Integer> lineMapping
if (lineMapping.isEmpty()) {
return;
}
Map<Integer, Integer> linesStat = new HashMap<>(); // count repeating lines
for (Map.Entry<Integer, Integer> entry : lineMapping.entrySet()) {
try {
Integer offset = entry.getKey();
InsnNode insn = insnArr[offset];
if (insn != null) {
insn.setSourceLine(entry.getValue());
int line = entry.getValue();
insn.setSourceLine(line);
if (insn.getType() != InsnType.NOP) {
linesStat.merge(line, 1, (v, one) -> v + 1);
}
}
} catch (Exception e) {
mth.addWarnComment("Error attach source line", e);
}
}
// 3 here is allowed maximum for lines repeat,
// can occur in indexed 'for' loops (3 instructions with same line)
List<Map.Entry<Integer, Integer>> repeatingLines = ListUtils.filter(linesStat.entrySet(), p -> p.getValue() > 3);
if (repeatingLines.isEmpty()) {
mth.add(AFlag.USE_LINES_HINTS);
} else {
mth.addDebugComment("Don't trust debug lines info. Repeating lines: " + repeatingLines);
}
}

private void attachDebugInfo(MethodNode mth, List<ILocalVar> localVars, InsnNode[] insnArr) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,47 +38,74 @@ private static class ProcessIfRegionVisitor extends AbstractRegionVisitor {
public boolean enterRegion(MethodNode mth, IRegion region) {
if (region instanceof IfRegion) {
IfRegion ifRegion = (IfRegion) region;
simplifyIfCondition(ifRegion);
moveReturnToThenBlock(mth, ifRegion);
moveBreakToThenBlock(ifRegion);
markElseIfChains(ifRegion);
orderBranches(mth, ifRegion);
markElseIfChains(mth, ifRegion);
}
return true;
}
}

private static class RemoveRedundantElseVisitor implements IRegionIterativeVisitor {
@Override
public boolean visitRegion(MethodNode mth, IRegion region) {
if (region instanceof IfRegion) {
return removeRedundantElseBlock(mth, (IfRegion) region);
@SuppressWarnings("UnnecessaryReturnStatement")
private static void orderBranches(MethodNode mth, IfRegion ifRegion) {
if (RegionUtils.isEmpty(ifRegion.getElseRegion())) {
return;
}
if (RegionUtils.isEmpty(ifRegion.getThenRegion())) {
invertIfRegion(ifRegion);
return;
}
if (mth.contains(AFlag.USE_LINES_HINTS)) {
int thenLine = RegionUtils.getFirstSourceLine(ifRegion.getThenRegion());
int elseLine = RegionUtils.getFirstSourceLine(ifRegion.getElseRegion());
if (thenLine != 0 && elseLine != 0) {
if (thenLine > elseLine) {
invertIfRegion(ifRegion);
}
return;
}
return false;
}
}

private static void simplifyIfCondition(IfRegion ifRegion) {
if (ifRegion.simplifyCondition()) {
IfCondition condition = ifRegion.getCondition();
if (condition.getMode() == Mode.NOT) {
if (condition != null && condition.getMode() == Mode.NOT) {
invertIfRegion(ifRegion);
}
}
IContainer elseRegion = ifRegion.getElseRegion();
if (elseRegion == null || RegionUtils.isEmpty(elseRegion)) {
int thenSize = insnsCount(ifRegion.getThenRegion());
int elseSize = insnsCount(ifRegion.getElseRegion());
if (isSimpleExitBlock(mth, ifRegion.getElseRegion())) {
if (isSimpleExitBlock(mth, ifRegion.getThenRegion())) {
if (elseSize < thenSize) {
invertIfRegion(ifRegion);
return;
}
}
boolean lastRegion = ifRegion == RegionUtils.getLastRegion(mth.getRegion());
if (elseSize == 1 && lastRegion && mth.isVoidReturn()) {
// single return at method end will be removed later
return;
}
if (!lastRegion) {
invertIfRegion(ifRegion);
}
return;
}
boolean thenIsEmpty = RegionUtils.isEmpty(ifRegion.getThenRegion());
if (thenIsEmpty || hasSimpleReturnBlock(ifRegion.getThenRegion())) {
boolean thenExit = RegionUtils.hasExitBlock(ifRegion.getThenRegion());
boolean elseExit = RegionUtils.hasExitBlock(ifRegion.getElseRegion());
if (elseExit && (!thenExit || elseSize < thenSize)) {
invertIfRegion(ifRegion);
return;
}

if (!thenIsEmpty) {
// move 'if' from then to make 'else if' chain
if (isIfRegion(ifRegion.getThenRegion())
&& !isIfRegion(elseRegion)) {
invertIfRegion(ifRegion);
}
// move 'if' from 'then' branch to make 'else if' chain
if (isIfRegion(ifRegion.getThenRegion())
&& !isIfRegion(ifRegion.getElseRegion())
&& !thenExit) {
invertIfRegion(ifRegion);
return;
}
// move 'break' into 'then' branch
if (RegionUtils.hasBreakInsn(ifRegion.getElseRegion())) {
invertIfRegion(ifRegion);
return;
}
}

Expand All @@ -88,33 +115,16 @@ private static boolean isIfRegion(IContainer container) {
}
if (container instanceof IRegion) {
List<IContainer> subBlocks = ((IRegion) container).getSubBlocks();
if (subBlocks.size() == 1 && subBlocks.get(0) instanceof IfRegion) {
return true;
}
return subBlocks.size() == 1 && subBlocks.get(0) instanceof IfRegion;
}
return false;
}

private static void moveReturnToThenBlock(MethodNode mth, IfRegion ifRegion) {
if (!mth.isVoidReturn()
&& hasSimpleReturnBlock(ifRegion.getElseRegion())
/* && insnsCount(ifRegion.getThenRegion()) < 2 */) {
invertIfRegion(ifRegion);
}
}

private static void moveBreakToThenBlock(IfRegion ifRegion) {
if (ifRegion.getElseRegion() != null
&& RegionUtils.hasBreakInsn(ifRegion.getElseRegion())) {
invertIfRegion(ifRegion);
}
}

/**
* Mark if-else-if chains
*/
private static void markElseIfChains(IfRegion ifRegion) {
if (hasSimpleReturnBlock(ifRegion.getThenRegion())) {
private static void markElseIfChains(MethodNode mth, IfRegion ifRegion) {
if (isSimpleExitBlock(mth, ifRegion.getThenRegion())) {
return;
}
IContainer elsRegion = ifRegion.getElseRegion();
Expand All @@ -127,6 +137,16 @@ private static void markElseIfChains(IfRegion ifRegion) {
}
}

private static class RemoveRedundantElseVisitor implements IRegionIterativeVisitor {
@Override
public boolean visitRegion(MethodNode mth, IRegion region) {
if (region instanceof IfRegion) {
return removeRedundantElseBlock(mth, (IfRegion) region);
}
return false;
}
}

private static boolean removeRedundantElseBlock(MethodNode mth, IfRegion ifRegion) {
if (ifRegion.getElseRegion() == null
|| ifRegion.contains(AFlag.ELSE_IF_CHAIN)
Expand Down Expand Up @@ -162,16 +182,16 @@ private static void invertIfRegion(IfRegion ifRegion) {
}
}

private static boolean hasSimpleReturnBlock(IContainer region) {
if (region == null) {
private static boolean isSimpleExitBlock(MethodNode mth, IContainer container) {
if (container == null) {
return false;
}
if (region.contains(AFlag.RETURN)) {
if (container.contains(AFlag.RETURN) || RegionUtils.isExitBlock(mth, container)) {
return true;
}
if (region instanceof IRegion) {
List<IContainer> subBlocks = ((IRegion) region).getSubBlocks();
return subBlocks.size() == 1 && subBlocks.get(0).contains(AFlag.RETURN);
if (container instanceof IRegion) {
List<IContainer> subBlocks = ((IRegion) container).getSubBlocks();
return subBlocks.size() == 1 && RegionUtils.isExitBlock(mth, subBlocks.get(0));
}
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ private static boolean makeTernaryInsn(MethodNode mth, IfRegion ifRegion) {
InsnNode thenInsn = tb.getInstructions().get(0);
InsnNode elseInsn = eb.getInstructions().get(0);

if (thenInsn.getSourceLine() != elseInsn.getSourceLine()) {
if (mth.contains(AFlag.USE_LINES_HINTS)
&& thenInsn.getSourceLine() != elseInsn.getSourceLine()) {
if (thenInsn.getSourceLine() != 0 && elseInsn.getSourceLine() != 0) {
// sometimes source lines incorrect
if (!checkLineStats(thenInsn, elseInsn)) {
Expand Down Expand Up @@ -134,6 +135,8 @@ private static boolean makeTernaryInsn(MethodNode mth, IfRegion ifRegion) {
InsnArg thenArg = InsnArg.wrapInsnIntoArg(thenInsn);
InsnArg elseArg = InsnArg.wrapInsnIntoArg(elseInsn);
TernaryInsn ternInsn = new TernaryInsn(ifRegion.getCondition(), resArg, thenArg, elseArg);
int branchLine = Math.max(thenInsn.getSourceLine(), elseInsn.getSourceLine());
ternInsn.setSourceLine(Math.max(ifRegion.getSourceLine(), branchLine));

InsnRemover.unbindResult(mth, elseInsn);

Expand Down
27 changes: 24 additions & 3 deletions jadx-core/src/main/java/jadx/core/utils/BlockUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,16 @@ public static InsnNode getLastInsnWithType(IBlock block, InsnType expectedType)
return null;
}

public static int getFirstSourceLine(IBlock block) {
for (InsnNode insn : block.getInstructions()) {
int line = insn.getSourceLine();
if (line != 0) {
return line;
}
}
return 0;
}

@Nullable
public static InsnNode getFirstInsn(@Nullable IBlock block) {
if (block == null) {
Expand All @@ -207,11 +217,22 @@ public static InsnNode getLastInsn(@Nullable IBlock block) {
}

public static boolean isExitBlock(MethodNode mth, BlockNode block) {
BlockNode exitBlock = mth.getExitBlock();
if (block == exitBlock) {
if (block == mth.getExitBlock()) {
return true;
}
return exitBlock.getPredecessors().contains(block);
return isExitBlock(block);
}

public static boolean isExitBlock(BlockNode block) {
List<BlockNode> successors = block.getSuccessors();
if (successors.isEmpty()) {
return true;
}
if (successors.size() == 1) {
BlockNode next = successors.get(0);
return next.getSuccessors().isEmpty();
}
return false;
}

public static boolean containsExitInsn(IBlock block) {
Expand Down
Loading

0 comments on commit a71b3a7

Please sign in to comment.