-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Test and fix for incompatible HEAP transforms in acorn-optimizer #24309
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 3785 | ||
| 3786 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 7816 | ||
| 7827 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 3985 | ||
| 3989 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 8247 | ||
| 8258 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12235,6 +12235,24 @@ def test_asan_strncpy(self): | |
| # https://github.com/emscripten-core/emscripten/issues/14618 | ||
| self.do_runf('other/test_asan_strncpy.c', emcc_args=['-fsanitize=address']) | ||
|
|
||
| @parameterized({ | ||
| 'asan': ['AddressSanitizer: null-pointer-dereference', '-fsanitize=address'], | ||
| 'safe_heap': ['Aborted(segmentation fault storing 1 bytes at address 0)', '-sSAFE_HEAP'], | ||
| }) | ||
| @parameterized({ | ||
| '': [], | ||
| 'memgrowth': ['-pthread', '-sALLOW_MEMORY_GROWTH', '-Wno-pthreads-mem-growth'], | ||
| }) | ||
| def test_null_deref_via_js(self, expected_output, *args): | ||
| # Multiple JS transforms look for pattern like `HEAPxx[...]` and transform it. | ||
| # This test ensures that one of the transforms doesn't produce a pattern that | ||
| # another pass can't find anymore, that is that features can work in conjunction. | ||
| self.do_runf( | ||
| 'other/test_safe_heap_user_js.c', | ||
| emcc_args=args, | ||
| assert_returncode=NON_ZERO, | ||
| expected_output=[expected_output]) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps a single test with all 3 features enabled is enough?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be enough to reproduce the issue, but I found that having separate tests helps to narrow down which particular transform is at fault. Just really putting "unit" into the "unit tests". |
||
|
|
||
| @node_pthreads | ||
| def test_proxy_to_pthread_stack(self): | ||
| # Check that the proxied main gets run with STACK_SIZE setting and not | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1372,7 +1372,10 @@ function isHEAPAccess(node) { | |
| function asanify(ast) { | ||
| recursiveWalk(ast, { | ||
| FunctionDeclaration(node, c) { | ||
| if (node.id.type === 'Identifier' && node.id.name.startsWith('_asan_js_')) { | ||
| if ( | ||
| node.id.type === 'Identifier' && | ||
| (node.id.name.startsWith('_asan_js_') || node.id.name === 'establishStackSpace') | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How did we get away without this before this change?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without fixes in this PR some heap patterns (in particular, in memory growth + asan tests) were never asanified. That's what the PR was testing & fixing after all, and once fixed, it started hitting new code paths. |
||
| ) { | ||
| // do not recurse into this js impl function, which we use during | ||
| // startup before the wasm is ready | ||
| } else { | ||
|
|
||
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 did this change?
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.
It was the easy way to add another
node.id.namecheck to skip theestablishStackSpacehelper too, as we skip specific function declarations.I could instead add a match to find variable declarator with a specific name and with arrow function expression initializer, but it would look somewhat more complex.