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

Add better error message when mixing exceptions modes. NFC #23738

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions src/lib/libexceptions.js
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ var LibraryExceptions = {
},

#endif

#if WASM_EXCEPTIONS || !DISABLE_EXCEPTION_CATCHING
$getExceptionMessageCommon__deps: ['__get_exception_message', 'free', '$stackSave', '$stackRestore', '$stackAlloc'],
$getExceptionMessageCommon: (ptr) => {
Expand All @@ -277,17 +278,23 @@ var LibraryExceptions = {
return [type, message];
},
#endif

#if WASM_EXCEPTIONS
#if ASSERTIONS
// Try to give a more useful error message than simply `Undefined reference to `emscripten_longjmp`
emscripten_longjmp__deps: [() => error('undefined reference to `emscripten_longjmp`. One or more object files was not compiled with `-fwasm-exceptions`. Build with `-sASSERTIONS=0` to have wasm-ld report which one.')],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, -sASSERTIONS=0 is the default right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-sASSERTIONS=1 is the default for -O0 but -sASSERTIONS=0 is the default of -O1 and above.

emscripten_longjmp: () => {},
#endif

$getCppExceptionTag: () =>
// In static linking, tags are defined within the wasm module and are
// exported, whereas in dynamic linking, tags are defined in library.js in
// JS code and wasm modules import them.
#if RELOCATABLE
___cpp_exception // defined in library.js
___cpp_exception, // defined in library.js
#else
wasmExports['__cpp_exception']
wasmExports['__cpp_exception'],
#endif
,

#if EXCEPTION_STACK_TRACES
// Throw a WebAssembly.Exception object with the C++ tag with a stack trace
Expand Down
2 changes: 1 addition & 1 deletion src/lib/libexceptions_stub.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ var LibraryExceptions = {};
LibraryExceptions[name] = function() { abort(); };
#if !INCLUDE_FULL_LIBRARY
// This method of link-time error generation is not compatible with INCLUDE_FULL_LIBRARY
LibraryExceptions[name + '__deps'] = [function() {
LibraryExceptions[name + '__deps'] = [() => {
error(`DISABLE_EXCEPTION_THROWING was set (likely due to -fno-exceptions), which means no C++ exception throwing support code is linked in, but such support is required by symbol '${name}'. Either do not set DISABLE_EXCEPTION_THROWING (if you do want exception throwing) or compile all source files with -fno-except (so that no exceptions support code is required); also make sure DISABLE_EXCEPTION_CATCHING is set to the right value - if you want exceptions, it should be off, and vice versa.`);
}];
#endif
Expand Down
16 changes: 16 additions & 0 deletions test/test_other.py
Original file line number Diff line number Diff line change
Expand Up @@ -9438,6 +9438,22 @@ def test_exceptions_c_linker(self):
stderr = self.expect_fail([EMCC, '-sSTRICT', test_file('other/test_exceptions_c_linker.c')])
self.assertContained('error: undefined symbol: __cxa_find_matching_catch_1', stderr)

def test_exceptions_mismatch(self):
create_file('main.c', '''
#include <setjmp.h>
int main() {
jmp_buf buf;
longjmp(buf, 8);
}
''')
self.emcc('main.c', ['-c'])
err = self.expect_fail([EMCC, 'main.o', '-fwasm-exceptions'])
self.assertContained('error: undefined reference to `emscripten_longjmp`. One or more object files was not compiled with `-fwasm-exceptions`. Build with `-sASSERTIONS=0` to have wasm-ld report which one.', err)

err = self.expect_fail([EMCC, 'main.o', '-fwasm-exceptions', '-sASSERTIONS=0'])
self.assertContained('wasm-ld: error: main.o: undefined symbol: emscripten_longjmp', err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a big improvement to me.

When Emscripten invokes wasm-ld it would be nice if it could detect this error and add the extra context about this since one doesn't see the instruction to pass -fwasm-exceptions as a cflag here. I assume we are already redirecting stderr when we invoke wasm-ld?

If it's too inconvenient this by itself is quite helpful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emscripten doesn't currently capture and parse the output of wasm-ld. Its just fails if/when wasm-ld fails.

I'm not sure I want to go down the route of trying to get fancy with parsing the error output of wasm-ld. I decided that showing the more specific error when -O0 / -sASSERTIONS=1 are enabled was a good compromise.



@with_all_eh_sjlj
def test_exceptions_stack_trace_and_message(self):
src = r'''
Expand Down