[core] CodegenException when generating code for synthetic constructor call

[huku-]

When decompiling a major Android application, I get several errors like the one below:

JADX ERROR: Method code generation error
jadx.core.utils.exceptions.CodegenException: Error generate insn: 0x0007: CONSTRUCTOR  (r0v5 ? I:BuggyConstructor) =  call: BuggyConstructor.<init>():void type: CONSTRUCTOR in method: Test.<clinit>():void, dex: ssa_1_test.dex
    at jadx.core.codegen.InsnGen.makeInsn(InsnGen.java:256)
    at jadx.core.codegen.InsnGen.makeInsn(InsnGen.java:221)
    at jadx.core.codegen.RegionGen.makeSimpleBlock(RegionGen.java:109)
    at jadx.core.codegen.RegionGen.makeRegion(RegionGen.java:55)
    at jadx.core.codegen.RegionGen.makeSimpleRegion(RegionGen.java:92)
    at jadx.core.codegen.RegionGen.makeRegion(RegionGen.java:58)
    at jadx.core.codegen.RegionGen.makeRegionIndent(RegionGen.java:98)
    at jadx.core.codegen.RegionGen.makeIf(RegionGen.java:142)
    at jadx.core.codegen.RegionGen.makeRegion(RegionGen.java:62)
    at jadx.core.codegen.RegionGen.makeSimpleRegion(RegionGen.java:92)
    at jadx.core.codegen.RegionGen.makeRegion(RegionGen.java:58)
    at jadx.core.codegen.MethodGen.addRegionInsns(MethodGen.java:211)
    at jadx.core.codegen.MethodGen.addInstructions(MethodGen.java:204)
    at jadx.core.codegen.ClassGen.addMethodCode(ClassGen.java:318)
    at jadx.core.codegen.ClassGen.addMethod(ClassGen.java:271)
    at jadx.core.codegen.ClassGen.lambda$addInnerClsAndMethods$3(ClassGen.java:240)
    at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1540)
    at java.base/java.util.stream.SortedOps$RefSortingSink.end(SortedOps.java:395)
    at java.base/java.util.stream.Sink$ChainedReference.end(Sink.java:258)
    at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:485)
    at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
    at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
    at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
    at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
    at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497)
    at jadx.core.codegen.ClassGen.addInnerClsAndMethods(ClassGen.java:236)
    at jadx.core.codegen.ClassGen.addClassBody(ClassGen.java:227)
    at jadx.core.codegen.ClassGen.addClassCode(ClassGen.java:112)
    at jadx.core.codegen.ClassGen.makeClass(ClassGen.java:78)
    at jadx.core.codegen.CodeGen.wrapCodeGen(CodeGen.java:44)
    at jadx.core.codegen.CodeGen.generateJavaCode(CodeGen.java:33)
    at jadx.core.codegen.CodeGen.generate(CodeGen.java:21)
    at jadx.core.ProcessClass.generateCode(ProcessClass.java:61)
    at jadx.core.dex.nodes.ClassNode.decompile(ClassNode.java:287)
    at jadx.core.dex.nodes.ClassNode.decompile(ClassNode.java:266)
Caused by: jadx.core.utils.exceptions.JadxRuntimeException: Code variable not set in r0v5 ?
    at jadx.core.dex.instructions.args.SSAVar.getCodeVar(SSAVar.java:189)
    at jadx.core.codegen.InsnGen.makeConstructor(InsnGen.java:620)
    at jadx.core.codegen.InsnGen.makeInsnBody(InsnGen.java:364)
    at jadx.core.codegen.InsnGen.makeInsn(InsnGen.java:250)
    ... 35 more

I analyzed this bug and managed to create a minimal test-case consisting of 2 smali files. The first one, named Test.smali is shown below:

.class public LTest;
.super Ljava/lang/Object;
.source "Test.java"

.field public static final A00:Ljava/lang/Object;

.method public static constructor <clinit>()V
    .locals 1
    new-instance v0, LBuggyConstructor;
    invoke-direct {v0}, LBuggyConstructor;-><init>()V
.end method

While the second, named BuggyConstructor.smali, is shown below:

.class public LBuggyConstructor;
.super Ljava/lang/Object;
.source "BuggyConstructor.java"

.implements LInterfaceClass;

.method public synthetic constructor <init>()V
    .locals 0
    invoke-direct {p0}, Ljava/lang/Object;-><init>()V
    return-void
.end method

To reproduce the decompilation error, the following commands can be used:

java -jar smali.jar assemble -a 27 -o ssa_1_test.dex Test.smali BuggyConstructor.smali
jadx ssa_1_test.dex

If the synthetic keyword is removed from BuggyConstructor.smali, the decompilation error disappears.

After studying the source code, I concluded that the problem seems to be in ConstructorVisitor and more specifically in method processInvoke().

ConstructorInsn replace = processConstructor(mth, co);
if (replace != null) {
    remover.addAndUnbind(co);
    co = replace;
}

BlockUtils.replaceInsn(mth, block, indexInBlock, co);

As far as I understand, the call to addAndUnbind() in the if-clause destroys the SSA information for the result variable of the constructor call. This seems to be happening in unbindResult() of InsnRemover via a call to removeSsaVar(). However, this information is required in the call to replaceInsn().

Removing the call to addAndUnbind() seems to be fixing the problem. I'm currently trying to fully understand the semantics of this visitor in order to propose a better solution.

[huku-]

So, instead of removing the call to addAndUnbind(), addWithoutUnbind() can be used, which, as far as I understand, is the correct thing to do here:

if (replace != null) {
    remover.addWithoutUnbind(co);
    co = replace;
}

@skylot any better ideas?

[skylot]

@huku- thanks for test case!
You are right in this particular case this fix is correct, because we reuse result arg and there are no other args to unbind. But in more general case if other args exists I prefer approach with duplicating all args needed in new instruction and after that, we can safely unbind old insn with all args.
So, to fix this issue it is better just add .duplicate() to make new result arg:

ConstructorInsn newInsn = new ConstructorInsn(defCtr.getMethodInfo(), co.getCallType());
newInsn.setResult(co.getResult().duplicate());
return newInsn;

Check my fix here: 0221380

Also, the funny part is that you reduce your test case so much that there are no non-synthetic constructor and jadx trying to replace constructor with the same one, so I added additional check to prevent that:

if (defCtr == null || defCtr.equals(callMth) || defCtr.getAccessFlags().isSynthetic()) {
    return null;
}

And in result in this test case we can't replace synthetic constructor at all :)