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

Add --uncheckedBehavior to customize the use of unchecked() contexts #2575

Merged
merged 2 commits into from
Dec 22, 2022

Conversation

CountBleck
Copy link
Member

@CountBleck CountBleck commented Nov 22, 2022

This new option allows users to use unchecked() as usual, ignore it
entirely, and force unchecked contexts everywhere. Ignoring unchecked
is useful for debugging, and, if you're certain you want to accept the
risk, forcing unchecked could be beneficial for performance.

@MaxGraey
Copy link
Member

MaxGraey commented Dec 1, 2022

I think it's better to have several options. Like:

  • "uncheckedBehaviour": "default" (current scenario)
  • "uncheckedBehaviour": "never" (ignore all unchecked access)
  • "uncheckedBehaviour": "always" (use unchecked always)

wdyt?

@CountBleck
Copy link
Member Author

CountBleck commented Dec 4, 2022

I think it's better to have several options. Like:

* `"uncheckedBehaviour": "default"` (current scenario)

* `"uncheckedBehaviour": "never"` (ignore all unchecked access)

* `"uncheckedBehaviour": "always"` (use unchecked always)

wdyt?

I got this working, but the compiler doesn't want to bootstrap with --uncheckedBehavior always. For some reason, the Function's locals array doesn't contain ~lib/rt/common/OBJECT#get:rtId~this when compiling ~lib/rt/common/OBJECT#get:rtId using the Wasm variant. However, the corresponding Signature still has a this type. This leads to a negative length being passed to new Array(), which promptly aborts.

The last commit I added demonstrates this issue. When this is fixed, I'll force-push it away before this PR is merged.

@CountBleck CountBleck changed the title Add an option to ignore unchecked() expressions Add --uncheckedBehavior to customize the use of unchecked() contexts Dec 4, 2022
@CountBleck CountBleck force-pushed the disable-unchecked branch 2 times, most recently from 77e4e73 to 4734f33 Compare December 5, 2022 01:30
@CountBleck
Copy link
Member Author

After changing some code in the compiler, it appears bootstrap can compile all the tests using --uncheckedBehavior always.

@CountBleck CountBleck force-pushed the disable-unchecked branch 4 times, most recently from 49b7ec3 to 809a4d4 Compare December 5, 2022 18:41
@CountBleck
Copy link
Member Author

This should be ready-to-merge, unless @dcodeIO wants to remove the {}= overload for Array.

src/program.ts Outdated Show resolved Hide resolved
@CountBleck
Copy link
Member Author

CountBleck commented Dec 6, 2022 via email

src/flow.ts Outdated Show resolved Hide resolved
@CountBleck CountBleck force-pushed the disable-unchecked branch 3 times, most recently from 3e41b48 to ac6c182 Compare December 6, 2022 15:56
@CountBleck
Copy link
Member Author

CountBleck commented Dec 17, 2022

Even with the unchecked operator removed, something is somehow broken.
I don't know how to fix this, and I'm not sure how to investigate this.

@dcodeIO
Copy link
Member

dcodeIO commented Dec 18, 2022

The assert being hit is here (combining two flows that are attached to different functions). Interestingly this only fails for bootstrap:release but not for bootstrap:debug, which might hint at some sort of memory corruption. Perhaps, logging around the assert, seeing what the values are, gives some hints?

@CountBleck
Copy link
Member Author

I just played around with git checkout -p 6c1b60d1fc9e8e3f78b50c0d1d3ce4ae0576c0ff src/*.ts and git restore --staged -p and git restore . and eventually got some results that I don't understand.

@CountBleck CountBleck force-pushed the disable-unchecked branch 2 times, most recently from 718a287 to 4468eda Compare December 18, 2022 03:54
@CountBleck
Copy link
Member Author

--disableUnchecked always works in bootstrap now, but omitting it causes breakage. Can we merge this without having --disableUnchecked always work in bootstrap?

@CountBleck CountBleck force-pushed the disable-unchecked branch 2 times, most recently from b05502a to 544fd64 Compare December 18, 2022 04:58
@dcodeIO
Copy link
Member

dcodeIO commented Dec 18, 2022

Given the problems observed with always, I wonder if this mode might generally not be a good idea. Would this all work (and be good enough) if we'd go back to a simple --noUnchecked option?

@CountBleck
Copy link
Member Author

Given the problems observed with always, I wonder if this mode might generally not be a good idea. Would this all work (and be good enough) if we'd go back to a simple --noUnchecked option?

Going back to disabling unchecked would be fine, but describing the mode to be "unsafe" or "experimental" or just outright saying it breaks things should be warning enough.

@dcodeIO I'll do it if it's necessary.

@dcodeIO
Copy link
Member

dcodeIO commented Dec 19, 2022

Given that unchecked will likely not stick around for long anyway, that there are unexplainable issues with the third mode, and if that's fine, going back to a simple --noUnchecked option (this name would fit well with other options) is what I'd prefer, yes :)

@dcodeIO dcodeIO mentioned this pull request Dec 20, 2022
2 tasks
@dcodeIO
Copy link
Member

dcodeIO commented Dec 20, 2022

Same assertion is hit in another PR (linked above), which is otherwise unrelated. Interestingly, the issue can only be seen on CI, and only on boostrap:release.

This new option allows users to use unchecked() as usual, ignore it
entirely, and force unchecked contexts everywhere. Ignoring unchecked
is useful for debugging, and, if you're certain you want to accept the
risk, forcing unchecked could be beneficial for performance.
@CountBleck CountBleck force-pushed the disable-unchecked branch 2 times, most recently from 7f439ac to 478895f Compare December 22, 2022 19:48
@CountBleck
Copy link
Member Author

Bootstrapping with --uncheckedBehavior always definitely works now.

src/builtins.ts Outdated Show resolved Hide resolved
If the context is unchecked, something very wrong must have happened.

Co-authored-by: dcode <dcode@dcode.io>
@dcodeIO dcodeIO merged commit 7a35ff8 into AssemblyScript:main Dec 22, 2022
@dcodeIO
Copy link
Member

dcodeIO commented Dec 22, 2022

Thanks :)

@CountBleck CountBleck deleted the disable-unchecked branch October 9, 2023 00:53
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.

3 participants