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

WASM GC: Fix backwards array copy function #975

Merged
merged 1 commit into from
Nov 30, 2024

Conversation

lax1dude
Copy link
Contributor

The source and target offset parameters in the System.arraycopy implementation are passed to array.copy in the wrong order

@lax1dude
Copy link
Contributor Author

I don't know why the PR unit test failed, the unit tests that ran when I pushed the commit 30 minutes before passed, and the branch I made was based on your latest commit, I think the failure is unrelated.

lax1dude@75ab3be

@konsoletyper
Copy link
Owner

Seems it's a false positive. For some reason tests sometimes fail on github, however never could reproduce this locally. Can you please provide tests for this fix?

@lax1dude
Copy link
Contributor Author

Seems it's a false positive. For some reason tests sometimes fail on github, however never could reproduce this locally. Can you please provide tests for this fix?

Regarding if the patch works or not, my app wasn't working before and was crashing immediately due to an out of bounds array access trap on a copy, and when I swapped those two parameters it worked perfectly and now it the app loads all the way to the title screen with no issues.

Are you asking me to go and revise your existing classlib unit tests so that arraycopy is tested with separate src/dst offset values? If that's the case I can take care of that sometime when I also clean up my fix for the string pool being too large based on your recommendation from #971.

@konsoletyper
Copy link
Owner

Are you asking me to go and revise your existing classlib unit tests so that arraycopy is tested with separate src/dst offset values

Yes, exactly. This should be super-easy, why don't you do it right now? It should take 15 minutes.

@konsoletyper konsoletyper merged commit e4c3268 into konsoletyper:master Nov 30, 2024
5 of 6 checks passed
@konsoletyper
Copy link
Owner

Ok, I added tests myself. And it took 5 minutes to write them. Writing tests is an important part of software engineering culture, so please, if you submit PR and there's no reason not to write tests (like writing good test would take hours), you should add tests.

@lax1dude
Copy link
Contributor Author

lax1dude commented Nov 30, 2024

Ok, I added tests myself. And it took 5 minutes to write them. Writing tests is an important part of software engineering culture, so please, if you submit PR and there's no reason not to write tests (like writing good test would take hours), you should add tests.

Okay, sorry I didn't take care of it, I was going to do it today if you hadn't already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants