[PATCH] D81689: [WebAssembly] New-style command support

[Dan Gohman via Phabricator via llvm-commits]

sunfish added inline comments.


================
Comment at: lld/test/wasm/command-exports-no-tors.ll:5
+
+; Like command-exports.ll, but with no ctors or dtors, so there should be no
+; __wasm_call_ctors, __cxa_atexit, or wrappers.
----------------
sbc100 wrote:
> 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 .
`no-tors.ll` has no ctors *or* dtors. I've now converted these to `.s` tests.


================
Comment at: lld/test/wasm/init-fini-gc.ll:8
 
-; Test that the __wasm_call_ctor function if not referenced
+; Test that we emit wrappers and call __wasm_call_ctor when not referenced.
 
----------------
sbc100 wrote:
> I guess we should rename this test...?  
Renamed.


================
Comment at: lld/test/wasm/init-fini-gc.ll:27
 
-define i32 @__cxa_atexit(i32 %func, i32 %arg, i32 %dso_handle) {
+define hidden i32 @__cxa_atexit(i32 %func, i32 %arg, i32 %dso_handle) {
   ret i32 0
----------------
sbc100 wrote:
> Why this?
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.


================
Comment at: lld/test/wasm/init-fini-gc.ll:72
+; CHECK-NEXT:      - Index:           7
+; CHECK-NEXT:        Name:            _start.command_export
 
----------------
sbc100 wrote:
> What do we actually want to test for here?
I've now added comments to these CHECK lines.


================
Comment at: lld/wasm/Writer.cpp:624
+  // call, don't wrap the exports.
+  if (initFunctions.empty() && !config->isPic && WasmSym::callDtors == NULL)
+    return;
----------------
sbc100 wrote:
> config->isPic is already checked at the call site.
I've now changed it to an assert.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81689/new/

https://reviews.llvm.org/D81689