This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Implement @llvm.global_ctors and @llvm.global_dtors
ClosedPublic

Authored by sunfish on Dec 1 2017, 5:09 PM.

Details

Reviewers
sbc100
Summary

[UPDATE] This patch

  • lowers @llvm.global_dtors by adding @llvm.global_ctors functions which register the destructors with __cxa_atexit.
  • impements @llvm.global_ctors with wasm start functions and linker metadata

See here for more background.

Diff Detail

Repository
rL LLVM

Event Timeline

sunfish created this revision.Dec 1 2017, 5:09 PM
sunfish updated this revision to Diff 125269.Dec 2 2017, 7:40 AM

Updated patch:

  • optional __dso_handle support
  • optional comdat support
  • tests
  • better naming
sunfish retitled this revision from [WebAssembly] Experiment with lowering @llvm.global_dtors to [WebAssembly] Lower @llvm.global_dtors.Dec 3 2017, 7:06 AM
sunfish edited the summary of this revision. (Show Details)
sunfish retitled this revision from [WebAssembly] Lower @llvm.global_dtors to [WebAssembly] Implement @llvm.global_ctors and @llvm.global_dtors.Dec 7 2017, 5:12 AM
sunfish edited the summary of this revision. (Show Details)
sunfish updated this revision to Diff 125936.Dec 7 2017, 5:16 AM

This update adds @llvm.global_ctors support to the wasm object writer, as layed out here, including init function priorities as sketched here.

sunfish edited the summary of this revision. (Show Details)Dec 7 2017, 5:17 AM

How would you feel about landing a simplified version of this change that doesn't use the start section (i.e. always put start functions in the metadata)? Seems a little strange to special case the single start function. Also, I like the idea of letting the linker decide if/when to switch to using the start section (for now anyway).

sbc100 accepted this revision.Dec 13 2017, 12:13 PM

I'd like to land this soon if possible (sans the start section if possible) so I start working on the lld side.

This revision is now accepted and ready to land.Dec 13 2017, 12:13 PM

Omitting the start section part for now is ok with me. It should be sufficient to just remove the Priority == UINT16_MAX special cases.

I'm also adding comments for a few minor issues in the patch that I thought of. The COMDAT thing would be nice to do now so that we don't have to coordinate adding it in the future.

I won't be able to land anything this week, so I'm ok if you want to land this without the start section parts right now.

lib/MC/WasmObjectWriter.cpp
966 ↗(On Diff #125936)

We should reserve room here for an optional COMDAT symbol too. For now, it's probably sufficient to just stub it out with a encodeULEB128(0, getStream()).

test/CodeGen/WebAssembly/lower-global-dtors.ll
16 ↗(On Diff #125936)

This should be named orig_dtor65535.

ncw added inline comments.Dec 13 2017, 1:40 PM
lib/MC/WasmObjectWriter.cpp
1378 ↗(On Diff #125936)

I think these changes are good, my stuff for .init_array support is obsolete really and I'll withdraw that change request.

It is a bit surprising (to me) that the .o files have a start function. If I'm reading this correctly, is it arbitrarily putting one of the constructors as the start function, and the rest as "extra start functions" in the linking data? That's oddly non-uniform, surely at the very least all of them should go in the linking data, rather than storing the first one somewhere else.

sbc100 added inline comments.Dec 13 2017, 1:58 PM
lib/MC/WasmObjectWriter.cpp
1378 ↗(On Diff #125936)

Yes, that was my feedback too. Especially since we are probably not going to be using the start section at all for the time being. So I'm proposing the land a version of this without the use of the start section (at least initially).

I was thinking of trying to land this today by forking this change, removing the start section and add a obj<->yaml test for these.

ncw added a comment.Dec 16 2017, 5:12 AM

I've implemented the Musl side of this change. It was pretty simple really, I've just added the following to crt1.o, which is pulled in by the default link-line:

extern void __init_libc(char **envp, char *pn);

__attribute__((constructor(1)))
void __libc_ctor()
{
        __init_libc((char**)&__wasm_init_data.envp0, (char*)__wasm_init_data.argv0);
}

It sounds like Sam is working on the LLD-side of this change, to actually use the init-data section to synthesise an entrypoint. I look forward to testing with it!

lib/MC/WasmObjectWriter.cpp
1350 ↗(On Diff #125936)

Using UINT16_MAX as the default priority for constructors has always been a bit imperfect.

I don't suppose it could be changed? (Here and previously in TargetLoweringObjectFileWasm::getStaticCtorSection, where the default priority has already been assigned.) It might just be something we have to live with, but it's a longstanding gripe against GCC/ELF that you can't designate code to run after the default priority.

The default should be UINT16_MAX/2, nicely in the middle of the allowed range. I don't really have an urgent use-case currently for this change, but I know that people do find it frustrating sometimes. I was wondering whether we could avoid repeating GCC's mistake for a new platform like Wasm

sbc100 closed this revision.Dec 18 2017, 11:07 AM

I landed a modified version of this change in rL320774. I assume its OK to close this now.