I guess the new test covers the stub->bitcode lib dependence case. Is there a test that covers the codgen->libcall->stub case?

i.e. it seems like it would be simpler to just support processing the stubs once. or maybe more specifically, to handle them the same as other archive libraries (i.e. participate in the iterate-to-convergence phase of symbol resolution but not get special treatment for LTO?)

Do we really need to support stub libcalls (i.e. the kind that can be generated at codegen time)? These should just be compiler-rt and maybe a few core libc functions, right? Do we expect those to be implemented in JS and/or the environment?

cool, LGTM!

I think we can go ahead. We can always adjust emscripten's behavior as needed to synchronize with wasi-sdk

In D146568#4211390, @aheejin wrote:

I'm on the fence too. Are we going to (eventually) change all tests expectations? I think there are tests that can really benefit from the brevity (especially tests added with bugfixes, with which we only want to check a specific few lines of the output). But this test may not be one of them.

LGTM I guess. TBH I'm still a little on the fence about whether it's better to have fully auto-generated test expectations which are easy to update but check all the output, or more precisely targeted expectations which are more self-documenting (since they don't include output not relevant to the test at hand) and are less likely to require updating for unrelated changes...

When making DBG_VALUE/DBG_VALUE_INST instructions undefined

Otherwise this LGTM, but I'd still like to hear Dan's opinion if he's got one.

I guess you mean not stack_pointer but tls_base and __memory_base, right?

One other question about the test. Did you generate it from a C file? When I've made tests like this in the past I've usually included the C source as a comment at the top to make it easier to regenerate if needed.
Maybe we can land this as-is and see how it works in the debugger, and work on the test more if we improve the code.

Ah right. With regular codegen tests we don't have this kind of problem because the assembly output is symbolic; also some tests that test disassembly use objdump -dr which prints relocation info next to instructions that need relocations. But I don't think dwarfdump will do that.
I agree that moving this test to lld doesn't seem great either.

In D145626#4179867, @aheejin wrote:

Only debug info won't work. So the devtools DWARF extension (https://chrome.google.com/webstore/detail/cc%20%20-devtools-support-dwa/pdcpmagijalfljmkmjngeonclgbbannb) will display undefined or some garbage value for TLS variables in dynamic linking. But that's the status quo for them anyway.

I think this feature could also be useful in non-emscripten environments, e.g. you could have a stub library for each WASI API provided by the embedder. Does wasi-sdk just use --allow-undefined for this now? Stub libraries seem nicer than that.
@sunfish WDYT, does this look useful?

I love these kinds of debugging options...

• clang-format
• more cast?

Sorry about that, will fix.
And I guess I'll have to get into the habit of the pre-merge checks being useful!

• add comment and rename hashBuf

• clean up baked-in buffer size

I think this is ready for another review, PTAL.

Correctly rebase against main

Rebase, create valid UUID for fast hash build ID, remove md5

The general idea of this makes sense to me.
@pmatos and @asb have been thinking more about the type system recently, they might have an opinion.

Do those figures include using wasm-opt after linking?

LGTM with a test.

WebAssembly LGTM

@tlively does this test need the shuffle indices to test what it's intended to?

Sorry, no :)

It looks like all the feedback has been address for now, so LGTM.
We can always update the comments if we have more discussion.

I think the format should be the same for wasm64

Yeah, we had some of this discussion with @sunfish back in 2016 as well. I just ended up disabling some of our tests and accepting reduced optimizations in D24053 and we never came up with a way to get them back at the time.
I haven't had a chance to think about this more since my earlier comment but thanks for writing this down, I'll come back to this in a couple of weeks (feel free to ping me if I don't).

Sorry, I missed your comment 2 days ago. See my comment below.

In D140373#4008491, @aheejin wrote:

And another thing, I didn't add anything to copy handling because Wasm doesn't have copy instructions. (This COPY instruction is a pseudo instruction and should have been lowered into other instructions at this phase.)

This looks good to me overall with the plan for followup. Maybe we can wait to land for a bit to see if @jmorse has any followup comments.

LGTM. TBH i'm fine with this not having a test, since it only affects our LLVM development experience and not the output. but it's good as-is too.

Thanks for fixing this! I tested it on windows and it seems to work (with the caveat that it's not my primary platform either). I'll reland it. I'll add "reland" to the commit message but it also still has the link to this review issue, so that should be good enough for context.

Thanks! do you need me to commit this?

The code looks good to me. Could you add a test case to llvm/test/tools/llvm-objcopy/wasm/add-section.test?
I guess it could be like the 2nd test case (that creates a file using echo and adds it, then checks the YAML payload).

LGTM! nice investigation too.

I was actually thinking about something like the following:

IIUC in other asm formats, you can just e.g. switch to the data section (in the middle of generating code for a function), emit a data constant, and then switch back and finish the function. In theory we could do that for wasm too, as long as we do actually switch back and finish the function. but I get that would make this error checking a lot more complicated, and I don't know of any cases where we would actually want to do that. do you?

Don't forget to make a build of LLVM with all the targets to make sure you get everything.

This broke us in emscripten as well (building with trunk clang against a recent-but-not-trunk version of libcxx). I can test the fix if you want.

This looks good to me from the wasm perspective