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

asm2wasm assertion error on configure test #9257

Open
Beuc opened this issue Aug 19, 2019 · 11 comments

Comments

@Beuc
Copy link
Contributor

commented Aug 19, 2019

emcc -o conftest -O3 -Wall  conftest.c
conftest.c:57:6: warning: incompatible redeclaration of library function 'memset' [-Wincompatible-library-redeclaration]
char memset ();
     ^
conftest.c:57:6: note: 'memset' is a builtin with type 'void *(void *, int, unsigned long)'
1 warning generated.
asm2wasm: /b/s/w/ir/cache/builder/emscripten-releases/binaryen/src/passes/OptimizeInstructions.cpp:189: wasm::Index wasm::getMaxBits(wasm::Expression *, LocalInfoProvider *) [LocalInfoProvider = wasm::OptimizeInstructions]: Assertion `false' failed.
shared:ERROR: '/home/me/workdir/emtests/emsdk/fastcomp/bin/asm2wasm conftest.temp.asm.js --total-memory=16777216 --trap-mode=allow -O3 --mem-init=conftest.js.mem --mem-base=1024 --wasm-only -o conftest.wasm --mvp-features' failed (-6)

conftest.c.zip

This have been introduced between 1.38.37 and 1.38.38 AFAICT.

This breaks detection of memset and memcpy in configure scripts.

Beuc added a commit to renpy/renpyweb that referenced this issue Aug 19, 2019
@kripken

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

This fails on 1.38.37 for me as well, so it doesn't look like a regression between 1.38.37 and 1.38.38.

It looks like what happens is fastcomp emits code that assumes memset returns a value. I think it assumes that because LLVM tells it it does (since it does in libc). And that code ends up invalid, confusing the optimizer later. Probably the fault is fastcomp for emitting that code.

We are trying not to work on fastcomp at this point, as our focus is on the wasm backend. If this is a recent regression we can revert something, though, but as mentioned above this doesn't seem like a recent change? We can also try to debug an urgent problem even if it isn't a regression, but it would be good to understand the severity and whether there is a workaround before that.

@Beuc

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

Works for me with 1.38.37 (and prior versions):

$ emcc --version
emcc (Emscripten gcc/clang-like replacement) 1.38.37 (commit d94e9fbc89fce94a7f4135786f551ef7e00bf161)
Copyright (C) 2014 the Emscripten authors (see AUTHORS.txt)
This is free and open source software under the MIT license.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ emcc -o conftest -O3 -Wall  conftest.c
conftest.c:57:6: warning: incompatible redeclaration of library function 'memset' [-Wincompatible-library-redeclaration]
char memset ();
     ^
conftest.c:57:6: note: 'memset' is a builtin with type 'void *(void *, int, unsigned long)'
1 warning generated.

(Make sure you don't hit emscripten-core/emsdk#320 if using emsdk...)

@Beuc

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

I see you also asked for a rationale: upgrading emscripten (1.38.32->1.38.41) e.g. broke cross-compiling libjpeg-turbo, since ./configure improperly detected missing memset/memcpy and switched to bsd-style functions, eventually resulting in missing headers and compilation errors.

I don't know of a work-around, short of patching all my dependencies' config.h between ./configure and make.

I don't want to imagine what silent changes this regression introduced in my build process, so I'm sticking to 1.38.37 at the moment.

@kripken

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Interesting, I just can't reproduce this ever working in an earlier version. I tried earlier versions as well, clearing the cache and emsdk zips, etc. etc. Always fails for me, weird...

Can you bisect this on your machine between those two versions? Finding the commit might help. I'd bisect once in emscripten, and, if that doesn't help, once in binaryen as well.

@Beuc

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

It is:
08d5e0c
Remove WASM=0 hack for configure scripts. Similar to #8851 but this one was only for fastcomp. Removing this makes the two backends behave the same. (#8863)

I don't understand how your tests kept failing though, even when explicitely passing WASM=1, emcc<=1.38.37 overwrites that setting with WASM=0, making the compilation succeed.

@sbc100

This comment has been minimized.

Copy link
Collaborator

commented Aug 21, 2019

Is there any reason you can't switch the wasm backend to work around this? We want to avoid working on fastcomp if we can these days.

@Beuc

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

The reason is I use Emterpreter, with wildcard matching, neither of which is available in "upstream" (and switching to new/experimental async will be somewhat difficult, if possible at all).

I plan to experiment some more and possibly switch the backend, but it took me much time to just upgrade Emscripten so this will have to wait :/

@sbc100

This comment has been minimized.

Copy link
Collaborator

commented Aug 21, 2019

In that case perhaps you can just locally revert 08d5e0c until you can switch the backend.

I also think there is a way to pre-seed cmake to stop if running those tests.

@kripken

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

@Beuc

Thanks for bisecting! Ok, that commit explains things - this was always broken in fastcomp, but that commit exposed the fastcomp wasm bug (since it wasn't a bug in asm.js).

As @sbc100 mentioned I think it's simplest for you to revert that change locally - fixing fastcomp would take effort that seems unjustified, and reverting that change here in upstream would add a difference between the two backends that adds complexity for other users.

Btw, I think I know why I couldn't reproduce this - I was running the quoted command before,

$ emcc -o conftest -O3 -Wall  conftest.c

but it only fails in emconfigure I guess? That would explain why I failed to see it.

The reason is I use Emterpreter, with wildcard matching, neither of which is available in "upstream"

Wildcard matching is present now in upstream+Asyncify, at least for suffixes (like foo_bar_* will match all functions starting with foo_bar_). Do you need anything additional?

@Beuc

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

In that case perhaps you can just locally revert 08d5e0c until you can switch the backend.

I was thinking so, I'll try that to upgrade .37->.42.

but it only fails in emconfigure I guess? That would explain why I failed to see it.

No, I isolated with bare emcc (keeping the 'conftest.c' filename) - see both failing and working output above.

Wildcard matching is present now in upstream+Asyncify, at least for suffixes (like foo_bar_* will match all functions starting with foo_bar_). Do you need anything additional?

Thanks for the info. Currently I match in the middle:

"___pyx_pf_5renpy_2gl_6gldraw_6GLDraw_*draw_screen", "___pyx_pw_5renpy_2gl_6gldraw_6GLDraw_*draw_screen", "___Pyx_PyObject_CallNoArg_*", 

More importantly, Emterpreter provides a detailed stack trace whenever trying to pause in the wrong place, which is invaluable to populate the WHITELIST in complex code with indirect calls; here AFAICT the ASYNCIFY'd application will just skip the emscripten_sleep() and crash mysteriously later.

@kripken

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Yeah, those are reasonable requests, I agree. As mentioned on the mailing list please open an issue for those if you haven't already (I have >100 emails in my box after returning from vacation, so not sure what's going on yet...).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.