Page MenuHomePhabricator

[WebAssembly] "atomics" feature requires shared memory
ClosedPublic

Authored by tlively on Mar 12 2019, 5:40 PM.

Details

Summary

Makes it a linker error if the "atomics" feature is used but the user
does not opt in to shared memory or if "atomics" is disallowed but the
user does opt in to shared memory. Also check that an appropriate max
memory size is supplied if shared memory is used.

Diff Detail

Repository
rL LLVM

Event Timeline

tlively created this revision.Mar 12 2019, 5:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2019, 5:40 PM

In the future, atomics will likely be enabled by default, however it will continue to be desirable to be able to select non-shared memory, since non-shared memory doesn't require a maximum size, and since it can make interoperating with JS and other external code simpler. Is it possible to structure this patch in a way that supports this?

In the future, atomics will likely be enabled by default, however it will continue to be desirable to be able to select non-shared memory, since non-shared memory doesn't require a maximum size, and since it can make interoperating with JS and other external code simpler. Is it possible to structure this patch in a way that supports this?

When building for non-shared memory, shouldn't input code check for "ifdef _REENTRANT" and not generate any atomic instructions?

The wasm spec says that atomics are simply not allowed when non-shared memory is used. Do we need to hide this fact from the user code?

In the future, atomics will likely be enabled by default, however it will continue to be desirable to be able to select non-shared memory, since non-shared memory doesn't require a maximum size, and since it can make interoperating with JS and other external code simpler. Is it possible to structure this patch in a way that supports this?

When building for non-shared memory, shouldn't input code check for "ifdef _REENTRANT" and not generate any atomic instructions?

That, or they could use -mthread-model single and lower atomics into non-atomics.

It sounds like the answer is, users that want non-shared memory should use -mno-atomics. I guess that works, it's just surprising if we think of -matomics as a pure target feature flag.

tlively added a comment.EditedMar 14 2019, 2:31 PM

It sounds like the answer is, users that want non-shared memory should use -mno-atomics. I guess that works, it's just surprising if we think of -matomics as a pure target feature flag.

Yes, that is a little surprising but it also makes the flags simpler. If -matomics was purely for the target and we also used the thread model flags, -matomics would become entirely redundant; there would be no reason to use -matomics without -mthread-model posix and vice versa. So as a simplification, we just have -matomics carry both meanings. The corollary of that is that we will never include -matomics by default in any CPU configuration. Thankfully, this is all entirely transparent to the user, who still uses -pthread if they want threads and don't use -pthread if they don't want threads and everything just works.

Here's a deeper explanation of why treating -matomics this way is fine as far as link-time feature validation is concerned. Consider object files in the following situations for a target that supports atomics:

A) application is single-threaded and atomic operations are lowered to non-atomic operations.
B) application is single-threaded but atomic operations are preserved
C) application is multithreaded so atomic operations are preserved

Here are our goals:

  1. A should link with B. This is safe because the applications is single-threaded and the target supports atomics.
  2. A should not link with C. It is not safe to link a module with atomics stripped into a multithreaded application.
  3. B and C are the same object. Otherwise users would have to build their libraries twice when the only difference is the threadedness of the final application.

These goals form a contradiction, so we can only choose two of them. 2 enforces an important safety property and dropping 3 would be a larger ergonomic loss than dropping 1, so we drop goal 1. The three possible situations above are then represented by two types of object files:

  • An object where atomic operations are stripped (A)
  • An object where atomic operations are preserved (B and C)

These two types of objects cannot in general be safely linked together, so the first is no different from an object that does not support atomics at all. QED.

What that argument does not capture is the case where atomic operations are neither stripped or preserved because there were none in the source to begin with. In this case it would be ideal to produce an object that can be linked into both mutithreaded and single-threaded applications. This is totally possible as far as the backend and linker are concerned*, but it is an open UI problem. The only way I see to achieve this is to detect the use of atomic operations in the backend and magically change the atomics feature linkage policy depending on their presence. And that seems perhaps too magical. WDYT?

(*) This would be achieved by making objects containing atomic ops use "+atomics" in their target feature section, making objects with any stripped atomics use "-atomics", and making objects with no atomics to begin with simply not mention "atomics".

jfb added a comment.Mar 14 2019, 2:34 PM

D) Application uses (non-existent in WebAssembly) signal handling support and atomics just enforce program order.
E) Application is single-threaded but does cooperative interleaving of functions (but doesn't call it threads).

In D59281#1430026, @jfb wrote:

D) Application uses (non-existent in WebAssembly) signal handling support and atomics just enforce program order.
E) Application is single-threaded but does cooperative interleaving of functions (but doesn't call it threads).

Thanks, JF! I definitely want to consider all cases. The cases you mention use atomics independently from threads, so I think they are further evidence that when the user uses -matomics we should never be stripping away their atomic operations and that it is dangerous to allow linking together objects with atomics both stripped and not stripped.

tlively retitled this revision from [WebAssembly] "atomics" feature implies shared memory to [WebAssembly] "atomics" feature requires shared memory.Mar 20 2019, 1:43 PM
tlively edited the summary of this revision. (Show Details)
sbc100 added inline comments.Mar 20 2019, 2:14 PM
lld/wasm/Writer.cpp
915 ↗(On Diff #190366)

I think we decided against this part right? i.e. atomics should require but not imply shared memory? So this should probably be an error instead.

tlively marked an inline comment as done.Mar 20 2019, 2:22 PM
tlively added inline comments.
lld/wasm/Writer.cpp
915 ↗(On Diff #190366)

Yes, sorry. I updated the name and description but I haven't updated the code yet. This CL is going to be blocked on an upcoming CL that detects whether any atomics were stripped. Landing that first will avoid unnecessary turn in some tests.

tlively updated this revision to Diff 191621.Mar 20 2019, 7:17 PM
  • Make using --shared-memory when atomics are disallowed or omitting it when atomics are used an error.

@sbc100 and @aheejin, this is unblocked and ready to land whenever you have a chance to review it.

sbc100 accepted this revision.Mar 28 2019, 10:27 PM
sbc100 added inline comments.
lld/test/wasm/shared-memory.yaml
80 ↗(On Diff #192750)

This block looks exactly the same as the one above. Perhaps you only need one?

lld/wasm/Writer.cpp
915 ↗(On Diff #190366)

Use single quotes to avoid the escape char? Here and below.

This revision is now accepted and ready to land.Mar 28 2019, 10:27 PM
This revision was automatically updated to reflect the committed changes.
tlively marked 2 inline comments as done.