retro-LGTM

(btw if there's a link in the commit message to the review, it should close automatically after commit)

Right, I was actually referring to the VM's internal tracking of exception stacks. I'm assuming it will use its usual mechanism for JS exceptions, which does keep a stack trace that you can access from JS code. Rethrowing rather than a new throw might keep that one intact. It doesn't help with the C++ state, but it's still better than nothing.

Or you could just call it a revert of the previous patch, if you wanted.

So are we going to say that the mere existence of segment names tells the linker it should merge them as though they were separate data sections? (or, is there any other possible use?) Is there other potential per-MCSection metadata we'd want that would make this metadata end up more generic?

So, it just occurred to me; we don't have 2-phase unwinding, which is useful to get good stack traces for debugging (because if you have unhandled exceptions you can abort before the second phase, rather than after the stack has been unwound). If the VM has an exception object from the original throw, it should have a stack trace attached to it. If we rethrow rather than throwing anew each time, then it will be preserved. So could we try to use rethrow instead of throw where possible? If we think it even might be feasible, it might be worth leaving rethrow in for now.

Should the patch title be "create data *segments* based on MCSections"?

Oh also a nit: It's spelled 'separate' :D

Do wer have any tests with global vars that have an explicit section? We should have one, that tests that they get merged into a segment in the object file.

I think the reasoning was really just that code size reduction is even more important on wasm than other platforms, and because it's a new ABI there's no one depending on the default behavior that would break.

Shouldn't the LLVM-side tests be using the LLVM utils and not clang? Or did you mean lld tests, or waterfall tests or what?

I guess the reason for removing this is that we don't really have an analogue of different text sections in the wasm object format, but that dead function stripping can be done by any linker smart enough to link wasm at all. But this isn't necessarily true of data sections though, is it?

Ping!

• remove spurious change

otherwise LGTM

but "Fixes PRNNN" is fine to indicate that a bug is fixed, although now that I think of it, a real link (in either form) is probably even better. Either way there's no auto-linking. Not everyone even uses LLVM's bug tracker anyway.

It's just a bug number. PR I guess is a "Problem Report"? Also https://llvm.org/PR34392 is a shortcut to https://bugs.llvm.org/show_bug.cgi?id=34392

ah we raced. LGTM :)

Cool, I guess this fixes PR34392, you should put that in the commit description too.

• address comments: remove atomic rules with constant offsets

• address review comments

• revert extra change

Also looking back and my previous comment,

OK nevermind, GlobalVariable doesn't make sense because you have to load it, and similarly for GEP. We could probably do inttoptr though, and maybe select, to get a function pointer and call that.

Are there any other constants/constexprs besides bitcast that we might encounter, that we should test? Looking at the constexprs, I don't see too much that looks likely. Maybe GEP? I guess you can indirectly call a GlobalVariable, which ought to look the same as a local function pointer, maybe we should have a test for that too (presumably dag isel handles that too).
Also that this patch really does is just bail from fast-isel when the callee is a constant (which is fine), and that has the effect of causing calls of bitcasts to be lowered as direct calls.

Thanks!

Actually, maybe an even better idea:
getLibcallName() returns a StringRef and asserts rather than returning a failure code. getLIbcallNameIfAvailable() returns an Expected<StringRef> and can fail.
The former matches 90% of the existing uses. This would let us move away from char* for the external symbol APIs (assuming that's a good thing) but most of the existing uses wouldn't need to actually change, and those that check failure would still be simple.

So it looks like switching the API away from char*-based would be, maybe not super-trivial, but probably not too bad. It's interlinked essentially with other APIs relating to external symbols:
mostly SelectionDAG::getExternalSymbol(), and also MachineInstrBuilder::addExternalSymbol() but those could be converted too, and I don't think I'd mind doing it as general cleanup. I wonder if Expected<StringRef> is a little heavyweight though. It seems to have a really strong convention that the result should be checked first, and most use cases just don't care (arguably they could assert, but requiring that would add a lot of boilerplate asserts for probably not a lot of benefit). StringRef by itself (with empty string taking the place of the current use of nullptr) wouldn't be too bad, although having the empty string as a sort of sentinel value only used by a small fraction of the uses doesn't seem ideal either. I might still be inclined to prefer that over Expected though. Any thoughts?

• remove redundant unsupported elements and simplify initialization

• clarify comments

Yes; the wasm backend currently has a partial duplication of the libcall names that can go away (in fact, maybe i'll even roll that into this patch), and it also has signature information for each function. The signatures could go into the .def file as well, and the other users of the def file could just define a HANDLE_LIBCALL that ignores them.

LGTM.
Looking forward to reversing all of this soon :)

OK, I got it; this is for the linker's use of the lib/object abstraction, that's what I was missing.

So just to be sure I understand, we need this because we only have segments for initialized data (and there is implicitly BSS in between), so the offset can't just be calculated from the sizes of the segments?

Cool, LGTM

So trivial.obj.wasm already has the updated linking section or this patch just depends on updating it?

Code looks good. Maybe also add a test to test/MC/WebAssembly/reloc-data.ll or one of those others too?

One nit, otherwise LGTM. Don't forget to update Linking.md

LGTM with a nit. In the summary, did you mean that we were *not* setting their params and return types?

This looks good. I guess we need to land the intrinsics on the LLVM side first though.

otherwise LGTM