-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Implement SAFE_HEAP using acorn, and use it on user JS #12450
Conversation
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.
Do we still need the JS library technique of using {{{ makeGetValue }}}
or can that now be removed in favor of this?
tests/test_other.py
Outdated
@@ -1716,7 +1716,7 @@ def test_extern_prepost(self): | |||
self.assertGreater(len(js), 100 * SLACK) | |||
|
|||
def test_js_optimizer(self): | |||
ACORN_PASSES = ['JSDCE', 'AJSDCE', 'applyImportAndExportNameChanges', 'emitDCEGraph', 'applyDCEGraphRemovals', 'growableHeap', 'unsignPointers', 'asanify'] | |||
ACORN_PASSES = ['JSDCE', 'AJSDCE', 'applyImportAndExportNameChanges', 'emitDCEGraph', 'applyDCEGraphRemovals', 'growableHeap', 'unsignPointers', 'asanify', 'safeHeap'] |
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.
Time to split this one one-element-per-line, or at least multiple lines?
We can maybe remove it, yeah. There is some advantage to the makeGetValue notation, in that you don't need to write the |
I wasn't suggesting removing |
Oh, yes, it is redundant atm. Rather than remove it, though, I'd prefer to eventually remove |
Previously we instrumented the compiled code, and also code in JS libraries
using
{{{ makeGetValue }}}
etc., but not arbitraryHEAP8
etc. usage in userJS code. This fixed that by adding a
safeHeap
pass to the acorn optimizer.This found a few issues in our JS code too, which have been fixed in
recent PRs already.