[PATCH] D78072: [lld][WebAssembly] Do not require --shared-memory with --relocatable

[Thomas Lively via Phabricator via llvm-commits]

tlively marked an inline comment as done.
tlively added inline comments.


================
Comment at: lld/wasm/Writer.cpp:436
 
   if (disallowed.count("atomics") && config->sharedMemory)
     error("'atomics' feature is disallowed by " + disallowed["atomics"] +
----------------
sbc100 wrote:
> 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`?
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78072/new/

https://reviews.llvm.org/D78072