Page MenuHomePhabricator

[WebAssembly] New-style command support
ClosedPublic

Authored by sunfish on Jun 11 2020, 1:48 PM.

Details

Reviewers
sbc100
Summary

This adds support for new-style command support. In this mode, all exports
are considered command entrypoints, and the linker inserts calls to
__wasm_call_ctors and __wasm_call_dtors for all such entrypoints.

This enables support for:

  • Command entrypoints taking arguments other than strings and return values other than int.
  • Multicall executables without requiring on the use of string-based command-line arguments.

This new behavior is disabled when the input has an explicit call to
__wasm_call_ctors, indicating code not expecting new-style command
support.

This change does mean that wasm-ld no longer supports DCE-ing the
__wasm_call_ctors function when there are no calls to it. If there are no
calls to it, and there are ctors present, we assume it's wasm-ld's job to
insert the calls. This seems ok though, because if there are ctors present,
the program is expecting them to be called. This change affects the
init-fini-gc.ll test.

Diff Detail

Event Timeline

sunfish created this revision.Jun 11 2020, 1:48 PM
sunfish updated this revision to Diff 270386.Jun 12 2020, 7:06 AM

Use the return value of handleUndefined instead of doing a separate lookup, which avoids a redundant lookup and handles LazySymbols properly.

sunfish updated this revision to Diff 270412.Jun 12 2020, 8:38 AM

Run clang-format and fix lint warnings.

Interesting direction! I'm only skimmed it for now. Not strongly opposed to this direction although it makes me sad to see more codegen/magic in the linker.

lld/wasm/Driver.cpp
476

Can we make this a separate change? Its seems pretty distinct.

lld/wasm/MarkLive.cpp
76

Is this just a de-duplication with the code below? It is related to this change to independent?

lld/wasm/Writer.cpp
1190

Can you replace these 2 lines with WasmSym::callCtors->isExported() ?

sunfish updated this revision to Diff 270489.Jun 12 2020, 12:21 PM
  • Address review feedback
  • Mark callCtors live in the MarkLive pass rather than in the Writer pass.
sunfish marked 6 inline comments as done.Jun 12 2020, 12:48 PM
sunfish added inline comments.
lld/wasm/Driver.cpp
476

Ok, I've removed it, and I'll submit it in a separate patch.

lld/wasm/MarkLive.cpp
76

I guess it is deduplication and possibly separable, but it's closely related to the rest of the patch.

Previously, when the input didn't have a __wasm_call_ctors call, we'd treat the init functions as dead code and delete them. With this patch, when the input doesn't have a __wasm_call_ctors call, we assume it's wasm-ld's job to insert __wasm_call_ctors calls. So it no longer makes sense to say "if __wasm_call_ctors is live then the init functions are live". The init functions are now just always live.

lld/wasm/Writer.cpp
1190

Done.

sbc100 added inline comments.Jun 26 2020, 4:45 PM
lld/test/wasm/command-exports-no-tors.ll
6

no-tors.ll -> no-ctors.ll?

Also, I've been converting existing tests over to the .s format. For an example of this see ctor_return_value.s. If possible, I'd prefer to add .s tests only going forward .

lld/test/wasm/command-exports.ll
3

Ditto .s if possible.

lld/test/wasm/init-fini-gc.ll
8

I guess we should rename this test...?

27

Why this?

72

What do we actually want to test for here?

lld/wasm/Writer.cpp
625

config->isPic is already checked at the call site.

Description needs updating after --emscripten was split out.

sbc100 added inline comments.Jun 26 2020, 5:02 PM
lld/wasm/Writer.cpp
947

The comment say *or* but the code says "&&".

Can this part just be reverted? Since callCtors should not be live unless its needed.

sunfish updated this revision to Diff 282304.Jul 31 2020, 1:27 PM
sunfish marked 9 inline comments as done.

Address review feedback.

lld/test/wasm/command-exports-no-tors.ll
6

no-tors.ll has no ctors *or* dtors. I've now converted these to .s tests.

lld/test/wasm/init-fini-gc.ll
8

Renamed.

27

That's how __cxa_atexit will typically be defined. And we don't want it being exported to prevent GC, which might defeat the purpose of the test.

72

I've now added comments to these CHECK lines.

lld/wasm/Writer.cpp
625

I've now changed it to an assert.

Friendly ping!

sbc100 accepted this revision.Sep 30 2020, 5:17 PM

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

lld/test/wasm/command-exports-no-tors.s
8 ↗(On Diff #282304)

You can remove the .section directives for all functions I believe to make the tests most concise.

10 ↗(On Diff #282304)

And these explicit .type entries for functions. (the .functype on its own below I think is enough).

18 ↗(On Diff #282304)

And these two lines can go too.

lld/test/wasm/command-exports.s
94 ↗(On Diff #282304)

No need for __dso_handle here or above.

This revision is now accepted and ready to land.Sep 30 2020, 5:17 PM
sunfish edited the summary of this revision. (Show Details)Sep 30 2020, 5:19 PM
sunfish closed this revision.Sep 30 2020, 7:06 PM

Thanks! Description updated, fixes applied, and landed in https://reviews.llvm.org/rG6cd8511e5932