Improve the error when SUPPORT_LONGJMP=0 but longjmp is used #12394
Conversation
|
(also added a change to not run the backend pass, see edited initial comment) |
src/library.js
Outdated
| longjmp: function(env, value) { | ||
| _setThrew(env, value || 1); | ||
| throw 'longjmp'; | ||
| }, | ||
| #else | ||
| longjmp__deps: [function() { | ||
| error('longjmp support was disabled (SUPPORT_LONGJMP=0), but it is required by the code (either set SUPPORT_LONGJMP=0, or remove uses of it in the project)'); |
There was a problem hiding this comment.
The advice there should be to remove SUPPORT_LONGJMP=0 right? Since the default is 1
There was a problem hiding this comment.
Right, thanks, I was off by 1...
| self.run_process([EMCC, path_from_root('tests', 'core', 'test_longjmp.c'), '-s', 'SUPPORT_LONGJMP=0', '-c', '-o', 'a.o']) | ||
| stderr = self.run_process([EMCC, 'a.o', '-s', 'SUPPORT_LONGJMP=0'], stderr=PIPE, check=False).stderr | ||
| self.assertContained('error: longjmp support was disabled (SUPPORT_LONGJMP=0), but it is required by the code (either set SUPPORT_LONGJMP=0, or remove uses of it in the project)', | ||
| stderr) |
There was a problem hiding this comment.
Do we need two different tests here? I mean internally the object will be created either way.
Also SUPPORT_LONGJMP is ignored during compile phase so only needed at link time isn't it?
There was a problem hiding this comment.
The PR makes the flag be noticed during compile time, as it doesn't send the pass to the backend when longjmp is disabled (i added this after the first comment, so maybe you viewed before that).
But yeah, the test does seem redundant as it is. I'll change it to cover a corner case of compiling with support but linking without.
| args += ['-enable-emscripten-sjlj'] | ||
| if Settings.SUPPORT_LONGJMP: | ||
| # asm.js-style setjmp/longjmp handling | ||
| args += ['-enable-emscripten-sjlj'] |
There was a problem hiding this comment.
Doesn't this change mean that we would break projects that set SUPPORT_LONGJMP only at link time?
Not sure that is great idea. Also its nice to be able to make as many flags as possible into link-only flags.
There was a problem hiding this comment.
Oh.. SUPPORT_LONGJMP=1 is the default so maybe not an issue?
There was a problem hiding this comment.
Yes, there is some risk of breakage, fair point. But OTOH if the flag is ignored at compile time, then just setting the flag at link time won't work for purposes of invokes - the object file will have been rewritten to use invokes already.
Maybe it's not worth making this change as proper invoke handling is landing any time now, but it does seem more correct to me, that if the flag is passed, we should assume there is no longjmp work to be done.
Before this change, the error would just be "undefined symbol: longjmp" which is
not that clear, and worse, if errors on undefined symbols were disabled, it would
just be a warning.
Also don't run the backend pass if longjmp support is disabled (not needed, but
seems more correct, and saves time).