-
Notifications
You must be signed in to change notification settings - Fork 171
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
Avoid stdlib printing to stdout/stderr? #190
Comments
I guess this comes from libc++'s terminate handler, which does |
@sbc100 Wondering if you have an idea on how to solve this? (Is Emscripten’s libc++ patched to handle this differently somehow?) |
Yes, in emscripten there are some patches to remove what feels like excessive logging, e.g. But I am not sure that's the problem here. The possible terminate in this code is due to a In wasi-libc, unless you've added a new_handler, if you have exceptions disabled then it should just return 0 (weird, but debatable - in emscripten as you can see in the patch there, we abort instead), and if you have them enabled then it will throw |
|
I just re-built wasi-sdk |
Can you see where those imports are being called from if you look at the disassembly? You should able to track down the reason they are needed that way. You can also using |
I am not quite sure how to get meaningful output from the symbol trace:
Looking at the .wasm, though, it seems that the WASI I/O functions are exclusively invoked via indirect calls (with a bunch of wrappers in-between.
Are the indirect calls preventing DCE? Just in case that’s helpful, here’s the whole call graph for the binary:
|
I don’t mean to nag, but I do want to find a solution to this :D Is there anything else I can provide to isolate what the cause is? Is it the indirect calls? |
Yes it looks like the indirect calls. This are pulled in by usage of stdin/stdout/stderr. And also fdopen (although I suspect is a stdout/stderr reference in this case): You could try linking with |
Yeah, I was more looking for insight what to do about it. Looking at the source you linked (super helpful!) I could compile WASI SDK with |
You could try changing the libcxxabi code to:
Or maybe even |
Looks like |
After looking into it a bit more, it seems that So you can either follow @sbc100’s suggestion and monkey-patch - #if !defined(NDEBUG) || !defined(LIBCXXABI_BAREMETAL)
+ #if (!defined(NDEBUG) || !defined(LIBCXXABI_BAREMETAL)) && !defined(__wasm__) or you edit WASI SDK’s LIBCXXABI_CMAKE_FLAGS = \
# ... snip ...
-DUNIX:BOOL=ON \
+ -DLIBCXXABI_BAREMETAL=ON \
+ -DLIBCXXABI_ENABLE_ASSERTIONS=OFF \
--debug-trycompile AFACIT, |
It seems building without |
Actually wouldn't you need both Its not clear to me exactly what |
Yeah, for this specific issue, you need to both disable assertions and enable baremetal. I also don’t quite understand what baremetal entails exactly, but I feel like disabling assertions in general would be a good default to set in WASI SDK. Do you think it’s worth/possible to add new flag to libcxxabi upstream to disable any from of stdout/stderr usage? |
These message and assertions can be very useful to developers, so removing them by default sounds wrong. It sounds like we might need two different build configuration since I imagine most folks would be sad not have these messages present (at least in their debug builds). |
To be clear: Disabling assertions by default wouldn’t remove these error message. For that, developers would also have to opt in to baremetal. I agree that we shouldn’t disable these messages by default, but I am suggesting adding a specific new flag to opt out of logging that isn’t |
I think its reasonable to be able to opt out of abort_message() printing and formatting explicitly. What is more we might want to make abort_message into a macro in the this case so that the string constants themselves are also removed from the build. There are already exists Then we could do modify
|
|
If that is acceptable upstream that is great. However it could be that folks want to silence just |
I went down the rabbit hole a little bit because I was confused as to why these table slots were being included at all given that To help debug I built with I turns out that
Files that reference
The issue is a limitation of the powers of I can think of a few ways to make this better: Solution 1Modify Solution 2Split This causes abort_message to be pulled in by the normal linking rules and then removed by Solution 3Try to make I suggest we push for both (1) and (2) for now and consider (3) for future work. |
Actually solution (2) doesn't fix he issue on it own since there is another reference to |
@sbc100, do you think this is still an issue? |
I guess it depends how much we really case about module size in these types of cases. IIUC such modules don't contains any calls to these functions, they just occupy table slots. Perhaps we can close as won't fix for now and re-open if there is appetite to fix it? (is there any harm is just leaving it open as a feature request?) |
Yeah, I think leaving it open as a feature request is fine. |
@surma are you embedding the wasm code in javascript?
And then:
|
A very minimal code sample like this:
compiled with WASI SDK like this:
results in a Wasm module that expects
wasi_snapshot_preview1::{fd_close, fd_seek, fd_write}
as imports, which feels unexpected to me. I am not sure why these imports are there, because I can’t find any embedded debugging strings, but I am assuming it’s because failed allocations etc will print to stderr? Is there any way to disable that and just fail withunreachable
instead?The text was updated successfully, but these errors were encountered: