Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[jvm] Assign dynamic method only if it's null #11530

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

Simn
Copy link
Member

@Simn Simn commented Feb 1, 2024

We found a disabled test that fails on the JVM target:

class MyDynamicClass {
	var v:Int;

	public function new(v) {
		this.v = v;
	}

	public dynamic function add(x, y) {
		return v + x + y;
	}
}

class MyOtherDynamicClass extends MyDynamicClass {
	public function new(v) {
		add = function(x, y) return x + y + 10;
		super(v);
	}
}

function main() {
	var inst = new MyOtherDynamicClass(0);
	trace(inst.add(1, 2)); // 3, should be 13
}

The problem is that the generated constructor code always assigns the dynamic method default, while here it shouldn't do that because the subclass already assigns it.

This PR adds a null-check to that, but it currently fails with this error:

Exception in thread "main" java.lang.ClassFormatError: StackMapTable format error: bad type array size in method 'void unit.issues.Issue5124.<init>(haxe.jvm.EmptyConstructor)'
        at unit._TestMain.TestMain_Fields_.main(src/unit/TestMain.hx:16)
        at unit._TestMain.TestMain_Fields_.main(src/unit/TestMain.hx:1)

The StackMap looks normal to me though, at least in jclasslib:

jclasslib_3Jq68NtI0L

Strangely, if I disable the generation of Append entries and instead create Full, it works:

RHQJNTKDpY

To my understanding, these two should be equivalent. I can't find anything in the specification that would suggest otherwise, so I don't currently know what the problem is.

@Simn Simn added the platform-jvm Everything related to JVM label Feb 1, 2024
@Simn
Copy link
Member Author

Simn commented Feb 1, 2024

Isolated minimal example:

class Parent {
	public function new() {}
}

class Child extends Parent {
	public dynamic function f() {}
}

function main() {
	new Child().f();
}
Exception in thread "main" java.lang.ClassFormatError: StackMapTable format error: bad type array size in method 'void haxe.root.Child.<init>(haxe.jvm.EmptyConstructor)'
        at _Main.Main_Fields_.main(source/Main.hx:10)
        at _Main.Main_Fields_.main(source/Main.hx:1)

What might happen here is that the initial implicit frame already has the this entry as uninitialized local, and then the append would add that again and confuse the verifier. That doesn't seem to be what the error is actually saying though.

@Simn
Copy link
Member Author

Simn commented Feb 1, 2024

Found it! The problem was that the constructors were missing calls to finalize_arguments. This is required so that genjvm knows the initial, implicit stack map and can generate any deltas to it accordingly. I think it is correct to generate a Full entry here because this changes from uninitialized to initialized after the super constructor call.

Edit: Also (rant incoming), this whole StackMapTable business is just generally insane. Forcing compilers to generate this data and thus essentially replicating the stack management accurately creates a pretty high barrier of entry. I feel like the verifier should be able to infer these stack maps in a single pass, because apparently it can detect problems with the generated stack maps. I'm quite curious why it is designed like that.

@Simn Simn marked this pull request as ready for review February 1, 2024 05:53
@Simn Simn merged commit 8e0855c into development Feb 1, 2024
122 checks passed
@Simn Simn deleted the jvm_conditional_dynamic_method_assignment branch February 1, 2024 06:34
@skial skial mentioned this pull request Feb 1, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform-jvm Everything related to JVM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant