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

Various fixes for Arrays #868

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ihromant
Copy link
Contributor

No description provided.

@Ihromant
Copy link
Contributor Author

Failures are related to ByteBuffer in wasi, it changed nothing there.

@konsoletyper
Copy link
Owner

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) {
Copy link
Owner

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.

Copy link
Contributor Author

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System?

Copy link
Contributor Author

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

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?

Copy link
Contributor Author

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.

@Ihromant
Copy link
Contributor Author

I don't know how to launch and debug WASI, in my config this tests are just off.

@konsoletyper
Copy link
Owner

What's the problem to learn to debug WASI? This "I don't know" is never a good excuse

@Ihromant
Copy link
Contributor Author

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).

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.

None yet

2 participants