In D88827#2346567, @jhenderson wrote:

In D88827#2344916, @sbc100 wrote:

In D88827#2344900, @echristo wrote:

I really do think this needs to be under object. It can be separate in there as lib/Object/Copy if you want, but I don't think it should be a parallel directory. Hopefully the updates for the move aren't overly onerous here.

-eric

Won't that mean that any tool that just wants to read in object files (e.g. lld) will need to build this extra code for no reason? Not a huge deal I guess (especially for lld which due to LTO pulls in all of llvm!) but I imagine there are many tools that just want to read (and not modify) objects.

That will depend on the archiver/linker behaviour, I expect, but at least for standard ld.lld behaviour (I can't speak for others), only the objects that actually are needed are pulled from the archive. Therefore, for example, if LLD didn't reference anything in the objcopy codebase, it wouldn't actually use those archive members, and therefore it wouldn't be added to the tools code. Even if it did, linker options like --gc-sections or equivalent likely would cause the unused code to be removed at link time, if the tool is built appropriately.

This mostly lgtm. I would love it if we could avoid adding the new MVT value for now since then we could avoid changing the shared code on llvm/lib/CodeGen/ValueTypes.cpp.. but perhaps that absolutely needed for this change?

In D88827#2344900, @echristo wrote:

I really do think this needs to be under object. It can be separate in there as lib/Object/Copy if you want, but I don't think it should be a parallel directory. Hopefully the updates for the move aren't overly onerous here.

-eric

No I wouldn't format any code you didn't write.. nor assume that clang-format always knows best in all cases.

In general I'm loving how small this change is. I don't really grok the tablegen stuff but everything else looks pretty good to me.

Other than that this looks good to land

I seeing:

I think mind is slightly less intrusive.. and I like having the single test. So I'll land this then.

Looks like we raced to write this patch :)

Wow you are fast :) do you prefer this solution over mine?

Fix (I believe) is in https://reviews.llvm.org/D89290

Looks like we have a specific emscripten test for this since it started failing on our llvm roller:
https://github.com/emscripten-core/emscripten/blob/bd8e5b5296721e33cfad98322e6b70b9985cfd2d/tests/test_other.py#L900

• typo

comment

bsymbol is the default baviour for executables

lgtm % test nits

It looks like there are a bunch of wasm-ld tests that need similar updates.

Is this ready to land now?

Revert readobj part

In D89152#2324938, @sbc100 wrote:

In D89152#2324369, @jhenderson wrote:

sbc100 edited reviewers, ... removed: jhenderson.

(I've got a herald rule to auto-add myself to reviews touching llvm-readobj amongst other tools - no point trying to work around it!)

Sorry, didn't mean to try to work around your herald stuff, I also use herald to be notified about WebAssembly related thing. I think I was just trying to be precise about who I thought would be good reviewer. I won't remove your name anymore from that field.

In general, would it would make sense to add yourself to subscriber rather than reviewer if you want to monitor all changes in these areas? You could then add yourself as a reviewer to change that you care about? Or do you want to actually be a reviewer all of all change to lld? For example, this specific change, do you want to be a reviewer?

In D89152#2324369, @jhenderson wrote:

sbc100 edited reviewers, ... removed: jhenderson.

(I've got a herald rule to auto-add myself to reviews touching llvm-readobj amongst other tools - no point trying to work around it!)

revert

feedback

This change will allows us to remove the corresponding binaryen code: https://github.com/WebAssembly/binaryen/pull/3211 and instead pass -Bsymbolic when linking MAIN_MODULE's in emscripten.

comments

comments

Looks great! Just a couple of nits

In PR description: final wasm times -> but final wasm types

docs

The description needs updating to reflect the fact that this change no longer adds the --emscripten flag.

In D88506#2301679, @tlively wrote:

I think it's kind of confusing that export-all doesn't actually export all the symbols. What if we instead made export-all exactly equivalent to individually exporting all the symbols so that it errors out if mutable-globals is not enabled and any of the symbols is a mutable global? Then we could add a --no-export=<symbol> flag that users or drivers could use to filter out the mutable global symbols if they can't enable mutable-globals and need to avoid the error.

Typeos

I think this is probably just historical. llvm/Object/WasmTraits.h is relatively new I think. My original plan was to keep the public interface to llvm/Object/ very dumb without any classes or methods, but I think it has evolved into something a little more rich now.

Can you update the mapfile test to tickle this?

But I guess its only temporary as soon engines will catch up and the error will go away.

Imagine a project that links statically without threads but its as dependency on some autoconf or cmake library that happens to throw in the -pthread... such users would hit this error today so I guess its only new users who do this that would be a runtime rather than link time error after this change.

Awesome thanks!

This is already supported like via -mllvm. E.g. wasm-ld -mllvm -debug or wasm-ld -mllvm -debug-only=lld

Do you have commit access or should I land this for you?

Great!

Thanks for fixing this! Sorry it took me so long to get back to you. This lgtm % some comments.

I don't really grok the TargetFrameLowering::DwarfFrameBase part but everything else LGTM

Might even be worth backporting such as simple but useful fix to the 11 release?

I'd love to see this land so we can drop our downstream patch in emscripten and also fix the outstanding wasi-sdk issue.

Seems reasonable. Do you think this way is cleaner than the way elf does it? Looks like ELF creates two different ELFWriter inside the ELFDwoObjectWriter subclass right?

This came up again in wasi-sdk: https://github.com/WebAssembly/wasi-sdk/issues/153

This came up again in wasi-sdk: https://github.com/WebAssembly/wasi-sdk/issues/153

revert part

feedback

In D87666#2272988, @tlively wrote:

Should F and G be lowercased? It looks like the surrounding code uses camelCase fairly uniformly.

fix

feedback

Feedback