This is an archive of the discontinued LLVM Phabricator instance.

[lld][WebAssembly] Do not require --shared-memory with --relocatable
ClosedPublic

Authored by tlively on Apr 13 2020, 7:20 PM.

Details

Summary

wasm-ld requires --shared-memory to be passed when the atomics feature
is enabled because historically atomic operations were only valid with
shared memories. This change relaxes that requirement for when
building relocatable objects because their memories are not
meaningful. This technically maintains the validity of object files
because the threads spec now allows atomic operations with unshared
memories, although we don't support that elsewhere in the tools yet.

This fixes and Emscripten build issue reported at
https://bugs.chromium.org/p/webp/issues/detail?id=463.

Diff Detail

Event Timeline

tlively created this revision.Apr 13 2020, 7:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2020, 7:20 PM

lgtm with a couple of questions..

lld/wasm/Writer.cpp
444

What about all these other tests that also depend on the sharedMemory flag?

I would have thought that in relocatable mode we simply never look at the sharedMemory flag. In fact it should probably be a link error to use --shared-memory and --relocatable at the same time?

Also. what about --check-features/--no-check-features do they make sense with --relocatable?

tlively marked an inline comment as done.Apr 14 2020, 11:42 AM
tlively added inline comments.
lld/wasm/Writer.cpp
444

What about all these other tests

I looked at them, but this was the only one requiring --shared-memory. The other checks are predicated on --shared-memory being present, so they won't run anyway when you pass --relocatable without --shared-memory.

In fact is should probably be a link error to use --shared-memory and --relocatable at the same time.

Yeah, I agree. I'll add a check for that.

What about --check-features/--no-check-features

I think that these do make sense. It is still dangerous to mix pthread code with atomic-stripped non-pthread code even when producing a relocatable object. One related issue is that right now the linker cannot emit target features with the disallowed prefix, so atomic-stripped non-pthread code involved in a --relocatable link is not later prevented from being mixed with pthread code. I can fix that in a follow-up CL.

tlively updated this revision to Diff 257443.Apr 14 2020, 12:13 PM
  • disallow --shared-memory with -r
sbc100 accepted this revision.Apr 14 2020, 12:52 PM
sbc100 added inline comments.
lld/wasm/Writer.cpp
433

Somehow this condition makes more sense when I read it if the relocatable check comes first. But maybe that just me ..

This revision is now accepted and ready to land.Apr 14 2020, 12:52 PM
tlively marked an inline comment as done.Apr 14 2020, 1:35 PM
tlively added inline comments.
lld/wasm/Writer.cpp
433

Sure, I'll swap it around.

This revision was automatically updated to reflect the committed changes.