Skip to content

Allow atomic operations on non-shared memory in SafeHeap#8890

Open
dschuff wants to merge 1 commit into
WebAssembly:mainfrom
dschuff:agy-fix-safeheap-atomics
Open

Allow atomic operations on non-shared memory in SafeHeap#8890
dschuff wants to merge 1 commit into
WebAssembly:mainfrom
dschuff:agy-fix-safeheap-atomics

Conversation

@dschuff

@dschuff dschuff commented Jul 9, 2026

Copy link
Copy Markdown
Member

SafeHeap needs to generate atomic helper functions even if the memory
is not shared, because atomic instructions can still be used.

TAG=agy
CONV=75c799ef-6dca-4712-8ffe-d8d1dfaffd0b

SafeHeap needs to generate atomic helper functions even if the memory
is not shared, because atomic instructions can still be used.

TAG=agy
CONV=75c799ef-6dca-4712-8ffe-d8d1dfaffd0b
@dschuff dschuff requested a review from a team as a code owner July 9, 2026 20:05
@dschuff dschuff requested review from stevenfontanella and removed request for a team July 9, 2026 20:05
Comment thread src/passes/SafeHeap.cpp
bool
isPossibleAtomicOperation(Index align, Index bytes, bool shared, Type type) {
return align == bytes && shared && type.isInteger();
isPossibleAtomicOperation(Index align, Index bytes, bool /*shared*/, Type type) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If any human or bot gets a chance to look at this, please remove the newly unused parameter.

@dschuff

dschuff commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

Yeah I was discussing this with @brendandahl and we think the first fix to the emscripten breakage that prompted this (see triage_20260708_084511.html on the internal x20 link) would be to ensure that we link with -pthread when linking with -fopenmp, which will ensure that the memory is actually shared.
But I'm still a little unclear on exactly what is happening with the shared-memory-lowering that LLVM does and how the whole thing interacts with SafeHeap, so maybe there will eventually be something to do in Binaryen; I'm not sure.

@tlively

tlively commented Jul 9, 2026

Copy link
Copy Markdown
Member

When libraries using atomic are compiled with atomics enabled, those atomics are not lowered away. Then we allow those libraries to be linked into single-threaded applications, which will not have shared memories. Presumably SafeHeap will want to instrument the resulting atomic memory accesses to unshared memories the same as it would any other memory access, so I think this PR is a correct fix.

@dschuff

dschuff commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

True; but IIRC there were some cases where we do lower atomics away after linking (for browsers that don't support atomic at all). I wasn't sure whether this case (where were "incorrectly" failling to use -pthread together with openMP) was supposed to have that happen. Or maybe it would have happened after this particular pass.

@dschuff

dschuff commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

Going back to the change itself, this just means that we'll generate atomic helper functions in many cases where we don't today, but that seems fine. I can go ahead and fix this up, write a test, etc.

@tlively

tlively commented Jul 10, 2026

Copy link
Copy Markdown
Member

True; but IIRC there were some cases where we do lower atomics away after linking

Oh interesting, I wasn't aware of that. I agree this change is correct regardless of that, though.

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