Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clang compiling to WebAssembly with multivalue and wasm-opt fails #60613

Closed
fluffyball9 opened this issue Feb 9, 2023 · 16 comments
Closed

Clang compiling to WebAssembly with multivalue and wasm-opt fails #60613

fluffyball9 opened this issue Feb 9, 2023 · 16 comments
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' lld:wasm

Comments

@fluffyball9
Copy link

Compiling to WebAssembly using clang with multivalue and wasm-opt on path build fails because clang doesn't automatically pass --enable-multivalue to wasm-opt. This parameter should be automatically passed to wasm-opt.

@llvmbot
Copy link
Member

llvmbot commented Feb 9, 2023

@llvm/issue-subscribers-backend-webassembly

@EugeneZelenko EugeneZelenko changed the title CLang compiling to WebAssembly with multivalue and wasm-opt fails Clang compiling to WebAssembly with multivalue and wasm-opt fails Feb 9, 2023
@sbc100
Copy link
Collaborator

sbc100 commented Feb 9, 2023

I think the expect behaviour is that the "target-features" section of the binary should include "multi-value". This should be enough to inform wasm-opt about the features to enable.

@tlively
Copy link
Collaborator

tlively commented Feb 9, 2023

@fluffyball9, can you share a clang driver command that reproduces the issue? I wonder if this is caused by the target features section being stripped somehow.

@fluffyball9
Copy link
Author

The simplest arguments that reproduces the issue are --target=wasm32 -mmultivalue -Xclang -target-abi -Xclang experimental-mv -nostdlib -O3 -s -Wl,--export-dynamic,--allow-undefined,--gc-sections. Based on what I've tried, it fails with any level of optimization except of O0.

@fluffyball9
Copy link
Author

If I run clang with -c and use wasm-dis, the sections at the end of the file are

 ;; custom section "linking", size 579
 ;; custom section "reloc.CODE", size 299
 ;; custom section "producers", size 93
 ;; features section: mutable-globals, sign-ext, multivalue

@tlively
Copy link
Collaborator

tlively commented Feb 16, 2023

That features section correctly contains "multivalue". @fluffyball9, what version of wasm-opt do you have in your path? Can you run wasm-opt --help and see if it supports the multivalue feature flag? If it doesn't, then it wouldn't support reading the feature out of the features section, either.

@fluffyball9
Copy link
Author

@tlively I'm using wasm-opt version 112. After linking, it seems like the features section is removed.

@tlively
Copy link
Collaborator

tlively commented Feb 21, 2023

This seems to work for me.

test.c:

struct pair {
  int a;
  int b;
};

struct pair mv_func(void);

int main() {
  return mv_func().a;
}

Compiling with:
PATH=~/code/binaryen:$PATH clang --verbose --target=wasm32 -mmultivalue -Xclang -target-abi -Xclang experimental-mv -nostdlib -O3 -s -Wl,--export-dynamic,--allow-undefined,--gc-sections,--no-entry test.c -o test.wasm

Finishes with no error, and I verified that it ran wasm-opt.

Can you provide a complete reproducer?

@tlively
Copy link
Collaborator

tlively commented Feb 21, 2023

Ah, I noticed that it had optimized main out entirely. If I add --export=main, then I can reproduce the error. investigating further now.

@tlively
Copy link
Collaborator

tlively commented Feb 21, 2023

Ok, so the problem is that the -s argument tells the linker to strip all sections, so the target feature section is being stripped. @sbc100, should we make it so that --strip-all does not actually strip the target features section? IIRC, wasm-objdump and wasm-ld are inconsistent here.

@dschuff
Copy link
Member

dschuff commented Feb 21, 2023

(did you mean wasm-objcopy rather than wasm-objdump?)

@dschuff
Copy link
Member

dschuff commented Feb 21, 2023

I confirmed that llvm-opjcopy/llvm-strip does not strip the target feature section with --strip-all (see https://github.com/llvm/llvm-project/blob/main/llvm/lib/ObjCopy/wasm/WasmObjcopy.cpp#L76). It only strips sections that it actually understands (linking/reloc, debug, name, producers). Probably makes sense for wasm-ld to do the same?

@sbc100
Copy link
Collaborator

sbc100 commented Feb 22, 2023

To me that would seem to contradict that meaning of --strip-all. I think if you pass --strip-all the linker you are kind of opting out of being able to run any post-link optimizations . If you want to do post-link stuff like wasm-opt, you should probably be relying on that tool to do that final stripping.

I guess that it might be more ergonomic just to never strip the target-features section, but what if I'm a user who actually wants to pass --strip-all and then ship the result of that to production. I think having a target-features section in that case would go against the user intent.

How about this: Clang can detect if its going to run wasm-opt after wasm-ld and in that case inject some kind of flag (perhaps a new --preserve-features) to tell wasm-ld about this.

@dschuff
Copy link
Member

dschuff commented Feb 28, 2023

Yeah, I guess it's the driver's job to manage the tool pipeline, including any flags that affect post-link opt, so moving the logic to the driver makes sense. I think llvm-objcopy/llvm-strip's -keep-section flag overrides the stripping and remove-section flags (see link above), but I'm not sure if lld has any logic like that.

@sbc100 sbc100 closed this as completed in 89d5635 Nov 2, 2023
@EugeneZelenko EugeneZelenko added clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' lld:wasm and removed backend:WebAssembly labels Nov 2, 2023
@llvmbot
Copy link
Member

llvmbot commented Nov 2, 2023

@llvm/issue-subscribers-lld-wasm

Author: None (fluffyball9)

Compiling to WebAssembly using clang with multivalue and wasm-opt on path build fails because clang doesn't automatically pass `--enable-multivalue` to wasm-opt. This parameter should be automatically passed to wasm-opt.

@llvmbot
Copy link
Member

llvmbot commented Nov 2, 2023

@llvm/issue-subscribers-clang-driver

Author: None (fluffyball9)

Compiling to WebAssembly using clang with multivalue and wasm-opt on path build fails because clang doesn't automatically pass `--enable-multivalue` to wasm-opt. This parameter should be automatically passed to wasm-opt.

qihangkong pushed a commit to rvgpu/llvm that referenced this issue Apr 18, 2024
This flag causes wasm-ld preserve a section even in the face of
`--strip-all`.  This is useful, for example, to preserve the
target_features section in the ase of clang (which can run wasm-opt
after linking), and emcc (which performs a bunch of post-link work).

Fixes: llvm/llvm-project#60613
Fixes: llvm/llvm-project#55781

Differential Revision: https://reviews.llvm.org/D149917
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' lld:wasm
Projects
None yet
Development

No branches or pull requests

6 participants