Skip to content

Test 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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

RReverser
Copy link
Collaborator

@RReverser RReverser commented May 12, 2025

While working on #24295, I noticed I could accidentally prevent ASan and SAFE_HEAP from finding any HEAPxx[n] accesses from JS when ALLOW_MEMORY_GROWTH is enabled, and no tests would catch it, because no tests verify that acorn-optimizer passes which change HEAPxx[n] into something else can work in conjunction with each other.

After adding the test, I found that an existing SUPPORT_BIG_ENDIAN feature was already broken in the same way, because it transforms HEAPxx[n] accesses into DataView methods, and since it was running before ASan and SAFE_HEAP, those latter transforms became no-ops. I fixed that in the same PR by moving the big-endian transform to the end.

In the future we might want to consolidate those transforms in a more reliable way (e.g. only run a single code transform that does HEAPxx[n] -> __loadHeap/__storeHeap where those functions do all the work using #ifs), but at least for now this PR fixes an immediate problem and catches potential future regressions.

@RReverser RReverser requested review from kripken and sbc100 May 12, 2025 17:29
@RReverser RReverser force-pushed the test-heap-transforms-combos branch from 4baf35f to 22719e3 Compare May 12, 2025 17:31
@RReverser
Copy link
Collaborator Author

Actually, if I add -pthread, there are even more combos that currently unexpectedly succeed... changing into draft until I fix them too.

@RReverser RReverser marked this pull request as draft May 12, 2025 17:39
@RReverser
Copy link
Collaborator Author

Ah yeah, looks like ALLOW_MEMORY_GROWTH doesn't work correctly with ASan / SAFE_HEAP for the same reasons even before my change in #24295.

'other/test_null_deref_via_js.c',
emcc_args=args,
assert_returncode=NON_ZERO,
expected_output=[expected_output])
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a single test with all 3 features enabled is enough?

Copy link
Collaborator Author

@RReverser RReverser May 12, 2025

Choose a reason for hiding this comment

The 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".

@kripken
Copy link
Member

kripken commented May 12, 2025

I guess it's not surprising some of these combinations don't work, given the possible interactions, and that we don't have exhaustive testing...

I'd say that only memory growth is a high priority, of the three (asan is run locally, and devs can work around feature combinations, and big-endian likely has very few users).

@RReverser
Copy link
Collaborator Author

RReverser commented May 12, 2025

I'd say that only memory growth is a high priority

Yeah I agree, combo of memory growth + ASan is the first one I noticed and then thought it's worth checking other combinations too.

I'd like to merge #24291 + #24295 + one more upcoming PR for simplifying ASan #24314 first, and then I'll fix these combo interactions as it will be somewhat easier to do on top of simpler transforms.

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.

2 participants