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

unexpected false: Bulk memory operation (bulk memory is disabled) #254

Closed
LinusU opened this issue Sep 26, 2022 · 20 comments
Closed

unexpected false: Bulk memory operation (bulk memory is disabled) #254

LinusU opened this issue Sep 26, 2022 · 20 comments

Comments

@LinusU
Copy link

LinusU commented Sep 26, 2022

When upgrading from SDK 15 to 16, I'm running into the following problem whilst compiling a c file:

 "/usr/bin/wasm-ld" -m wasm32 -L/share/wasi-sysroot/lib/wasm32-wasi/llvm-lto/14.0.4 -L/share/wasi-sysroot/lib/wasm32-wasi /share/wasi-sysroot/lib/wasm32-wasi/crt1-reactor.o --entry _initialize --export=malloc --export=free --export=lodepng_decode32 --export=lodepng_encode32 --strip-all /tmp/lodepng-67f409.o -lc /usr/lib/clang/14.0.4/lib/wasi/libclang_rt.builtins-wasm32.a -o lodepng.wasm
 "/usr/bin/wasm-opt" lodepng.wasm -Oz -o lodepng.wasm
[wasm-validator error in function 10] unexpected false: Bulk memory operation (bulk memory is disabled), on 
(memory.copy
 (local.get $0)
 (local.get $1)
 (local.get $2)
)
[wasm-validator error in function 11] unexpected false: Bulk memory operation (bulk memory is disabled), on 
(memory.fill
 (local.get $0)
 (local.get $1)
 (local.get $2)
)
Fatal: error validating input
clang-14: error: linker command failed with exit code 1 (use -v to see invocation)

This is the command I'm running:

clang --sysroot=/share/wasi-sysroot --target=wasm32-unknown-wasi -flto -Oz     -o lodepng.wasm -DLODEPNG_NO_COMPILE_DISK -DLODEPNG_NO_COMPILE_CPP -mexec-model=reactor -fvisibility=hidden -Wl,--export=malloc,--export=free,--export=lodepng_decode32,--export=lodepng_encode32,--strip-all -v -- lodepng/lodepng.c

Here is a Dockerfile that can be used to reproduce this. As can be seen, this used to work with SDK 15:

LinusU/cwasm-lodepng@3d9ab53


I've tried to work around this for quite a while, but couldn't find anything that worked. I've tried passing -mbulk-memory to clang, which seems to change the error slightly, but still doesn't work. Any help, or pointers on what to Google, would be much appreciated!

@sbc100
Copy link
Member

sbc100 commented Sep 28, 2022

The wasm binary generated by the linker should contain features section which contains bulk memory.

I think the correct fix here is to pass --detect-features to wasm-opt. @tlively should find a way to make that the default? It seems like it should the default and we could have an opt of with something like --ignore-features or --override-features?

@LinusU
Copy link
Author

LinusU commented Sep 28, 2022

I actually didn't know that clang invoked wasm-opt before I ran into this, I used to run it separately after clang was done 😅

Although since clang is running it, I'm not sure how to pass flags to it 🤔


Seems like it is run if wasm-opt exists in the path, and there isn't a way to pass arguments:

https://github.com/llvm/llvm-project/blob/940e290860894d274c986c88bea2dcc17f1e93c3/clang/lib/Driver/ToolChains/WebAssembly.cpp#L133

Should this be a patch to llvm? 🤔

@sbc100
Copy link
Member

sbc100 commented Sep 28, 2022

We could consider ways to make the automatic running of wasm-opt optional, or add extra flags, but I also think we might get push back if we try to add too much complexity there (e.g. use environment variables, or some kind of -Wo,... flag syntax).

I think make wasm-opt read the features section by default is probably good way to fix the proximate issue, and as a very short term workaround you could remove wasm-opt from your PATH when running the link step.

@LinusU
Copy link
Author

LinusU commented Sep 28, 2022

There is a feature request for an opt out flag here:

llvm/llvm-project#55781

Personally, I'm not a fan of "do x if some random binary exists, otherwise do y", especially when there aren't any warnings printed. Upon learning that wasm-opt was run automatically I thought something like: "Nice, wasm-opt is part of the SDK now, then I can remove the part in my docker file that installs it, and remove running of it manually", which I did in two other projects, but I now realise that wasm-opt wasn't being run at all then 😅


It seems like passing --detect-features to wasp-opt doesn't fix the issue:

 > [8/8] RUN wasm-opt --detect-features -O3 bcrypt.wasm -o bcrypt.wasm:                                                                                                                                   
#11 0.273 [wasm-validator error in function 9] unexpected false: Bulk memory operation (bulk memory is disabled), on                                                                                      
#11 0.273 (memory.copy
#11 0.273  (local.get $0)
#11 0.273  (local.get $1)
#11 0.273  (local.get $2)
#11 0.273 )
#11 0.274 Fatal: error validating input

@sbc100
Copy link
Member

sbc100 commented Sep 28, 2022

What flags did you pass when running wasm-ld (can you run clang with -v so it prints the full link command)? Its possible the features section is not being generated (I think this only happens if you passs --strip-all to wasm-ld).

@LinusU
Copy link
Author

LinusU commented Sep 28, 2022

Ah, I do pass --strip-all 😅

My goal is to have the smallest output so I'm passing -Oz and -Wl,--strip-all. You can see the full invocation here:

https://github.com/LinusU/cwasm-lodepng/blob/3d9ab53031d13bec662ffa8f50b57ac49d106e6a/Dockerfile#L33


Maybe it was a stroke of luck that I accidentally didn't run wasm-opt in the other project btw. because it seems like it decreased performance 😱

wasm Openwall x 4.12 ops/sec ±0.06% (15 runs sampled)
wasm Openwall2 x 3.92 ops/sec ±0.05% (14 runs sampled)

The second case here is the same .wasm file, but after running wasm-opt on it like so:

wasm-opt --enable-bulk-memory -O3 bcrypt.wasm -o bcrypt.wasm

If I understand it correctly, it's the linker that invokes wasm-opt. Since it's also the linker that I pass --strip-all to, I assume that it would be able to know which features are present? In that case, could the linker be updated to pass the correct --enable/--disable flags to wasm-opt?

@sbc100
Copy link
Member

sbc100 commented Sep 28, 2022

Its the clang driver, when running in link mode, that runs wasm-opt. However, the clang driver doesn't actually know about the features used since they could be embedded in object files.

BTW, you can use -Wl,--strip-debug to strip debug into but keep the features sections.

@sbc100
Copy link
Member

sbc100 commented Sep 28, 2022

Out of interest, how to the file sizes compare for you optimized and non-optimized builds? If wasm-opt doesn't give a win in either size or performance that would be very surprising.

@LinusU
Copy link
Author

LinusU commented Sep 28, 2022

It gives a win in file size!

bcrypt.wasm          28654
bcrypt-opt.wasm      25721

I did open an issue here WebAssembly/binaryen#5088, but then maybe the answer just is that wasm-opt optimises for file size over speed, even when passing -O3 or -O4 🤔

@LinusU
Copy link
Author

LinusU commented Sep 28, 2022

BTW, you can use -Wl,--strip-debug to strip debug into but keep the features sections.

Thanks for the tip! Do you know if there is any advantage to keeping the features sections in when shipping a wasm file intended to run under a specific runtime (Node.js) to users?

@sbc100
Copy link
Member

sbc100 commented Sep 28, 2022

The features section mostly exists tools further down the pipeline to read (e.g. wasm-opt). I don't know of any other reason to keep it around... I guess a node runtime could use it to set certain node flags (i.e. enable certain experimental features).

@tlively
Copy link
Member

tlively commented Sep 28, 2022

This seems like a bad problem for anyone using --strip-all. I don't see a great solution available today, since having clang pass --all-features to wasm-opt could produce uses of features the user didn't intend. I guess we would want to bring back wasm-opt's deprecated --detect-features flag and have it detect features based on what is used in the input module.

@kripken
Copy link
Member

kripken commented Sep 28, 2022

Can clang run wasm-opt before it does the strip perhaps?

@sbc100
Copy link
Member

sbc100 commented Sep 28, 2022

Not really.. the stripping is part of the linking process.

Is --detect-features deprecated in wasm-opt? Why?

@sbc100
Copy link
Member

sbc100 commented Sep 28, 2022

I guess the clang driver could inject some kind of --no-strip-features flag to wasm-ld if it knows its going to run wasm-opt after.

@tlively
Copy link
Member

tlively commented Sep 28, 2022

Not really.. the stripping is part of the linking process.

Is --detect-features deprecated in wasm-opt? Why?

Because all it used to do was read the target features section, which is the default behavior anyway. It's a good thing it's deprecated because now we can reuse it and get the behavior we actually want.

@sbc100
Copy link
Member

sbc100 commented Sep 28, 2022

Not really.. the stripping is part of the linking process.
Is --detect-features deprecated in wasm-opt? Why?

Because all it used to do was read the target features section, which is the default behavior anyway. It's a good thing it's deprecated because now we can reuse it and get the behavior we actually want.

Oh I was confused.. I thought the target features section was being ignored... but it was actually being stripped. In that case I think the solution here is to have clang ask wasm-ld to keep this section if it knows its going to run wasm-opt.

Are you talking about deriving features based on their usage? Can binaryen do that already?

@tlively
Copy link
Member

tlively commented Sep 28, 2022

Are you talking about deriving features based on their usage? Can binaryen do that already?

Yes, that's what I had in mind. But no, Binaryen can't already do that.

@LinusU
Copy link
Author

LinusU commented Sep 28, 2022

In that case I think the solution here is to have clang ask wasm-ld to keep this section if it knows its going to run wasm-opt.

Would be neat if wasm-opt would then strip that section out, so that --strip-all is still being honoured from the users perspective...

(also, thanks everyone for the discussion here, I've learnt so much today ❤️)

@abrown
Copy link
Collaborator

abrown commented Aug 22, 2023

It seems like this should be an upstream issue on LLVM. @sunfishcode opened llvm/llvm-project#64909 to track this. I'll close this here and we can continue the discussion there.

@abrown abrown closed this as completed Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants