-
-
Notifications
You must be signed in to change notification settings - Fork 269
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
Various fixes for Arrays #868
base: master
Are you sure you want to change the base?
Conversation
Failures are related to ByteBuffer in wasi, it changed nothing there. |
Did you try to debug failures in WASI? |
int sz = TMath.min(length, array.length); | ||
for (int i = 0; i < sz; ++i) { | ||
result[i] = array[i]; | ||
if (length < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shoud not be done here. Line below instantiates array and JVM should throw exception there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JVM don't throw required exception. I investigated create array instructions and they indeed just create array without any check (and we get error in JS). Still, I don't know how to add preliminary check and throw. Maybe some example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/konsoletyper/teavm/blob/master/core/src/main/java/org/teavm/parsing/ProgramParser.java#L478 here this anewarray is converted to instruction. But I don't see in code where length checks and throws are added then.
for (int i = 0; i < sz; ++i) { | ||
result[i] = (T) componentType.cast(original[i]); | ||
} | ||
TSystem.arraycopy((TObject) (Object) original, 0, (TObject) (Object) result, 0, sz); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is made intentionally. In exactly this case no fastArrayCopy optimization should be applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, if TeaVM optimizes out some check it's not supposed to optimize out, then may be you investigate issue in the optimizer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no difference between copyOf(T[] arr, int from, int to) and copyOf(T[] arr, int from, int to, Class cl) from bytecode standpoint. They both are Object[] in bytecode, so optimizer will make no difference. Still, in first case we know that fastArrayCopy is safe while in second case we don't.
I don't know how to launch and debug WASI, in my config this tests are just off. |
What's the problem to learn to debug WASI? This "I don't know" is never a good excuse |
From my earlier experience with C/WASM backend, it's either "you try some magic and pray for it working" (what I previously did) or spend 2-3 months of learning this platform to understand what's happening (which I don't have unfortunately). |
No description provided.