-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Improve JS library error reporting #25089
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
base: main
Are you sure you want to change the base?
Conversation
Ideally we could hook this up to EMCC_DEBUG and/or -save-temps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use EMCC_DEBUG here as you suggest? I think node can just read the env to get it?
src/modules.mjs
Outdated
@@ -269,22 +282,22 @@ export const LibraryManager = { | |||
this.library = userLibraryProxy; | |||
} | |||
pushCurrentFile(filename); | |||
let preprocessed_name = filename.replace(/\.\w+$/, '.preprocessed$&') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let preprocessed_name = filename.replace(/\.\w+$/, '.preprocessed$&') | |
let preprocessedName = filename.replace(/\.\w+$/, '.preprocessed$&') |
src/modules.mjs
Outdated
if (processed) { | ||
if (VERBOSE) { | ||
fs.writeFileSync(preprocessed_name, processed); | ||
error(`See preprocessed source in ${preprocessed_name}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmh, looking at the example error messages in default and verbose, they look quite the same.
After this PR, in order to examine the failing JS code, one has to first build with -sVERBOSE
, and then locate the .js file to examine in the output, and then open it in an editor. That is adding more steps to get to examining the source?
I wonder if it might make sense to always save the preprocessed output file in nonverbose mode, and in verbose mode, the output would be printed to stdout? Although hmm, then there would be a problem with leaving behind littering files in default output.
It is of course quite different from regular C/C++ errors/warnings to dump the whole file to stdout, though at least in the context of Closure warnings/errors, getting the full Closure input file dumped to stdout, along with line numbers is a great time saver. I typically re-run the failing build with a emcc ... > log.txt 2>&1
to get the full line-numberized Closure output into a file. That workflow feels faster than needing to locate the file manually, and then selecting that with mouse and copy-pasting to open in editor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I thought of saving the file by default too, but I didn't want to litter the tmp directory whenever a failure happened.
Regarding closure, I almost always use EMCC_DEBUG=1
which leaves the temp files around. Then the error message from closure directly correspond to the file and you can just open it and jump to the offending line. I find this way nicer than stderr redirection, especially since the line numbers are always correct.
(I wasn't aware of a mode where I closure dumps full file to stdout/stderr, but if its anything like this -sVERBOSE
option I think I would still prefer the separate file with matching line numbers).
That workflow feels faster than needing to locate the file manually, and then selecting that with mouse and copy-pasting to open in editor?
You don't need to "locate the file" per since the error message itself simply points direclty to it, just like a normal compiler error. e.g You can see /tmp/emcc-jscompiler-3XmDDr/libcore.js:1087
reported in the error above. The same goes for closure errors when you build with EMCC_DEBUG=1
.
If you personal prefer the stdout approach I can look into keeping that option around if you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this way nicer than stderr redirection, especially since the line numbers are always correct.
The line numbers are also correct with Closure errors. E.g.
a.c
#include <emscripten.h>
int main() { EM_ASM({nonExisting();}); }
EMCC_DEBUG=2 emcc a.c -o a.js --closure 1
prints
building:ERROR: Closure compiler run failed:
1: // include: shell.js
2: /**
3: * @license
4: * Copyright 2010 The Emscripten Authors
5: * SPDX-License-Identifier: MIT
6: */
7: // The Module object: Our interface to the outside world. We import
8: // and export values on it. There are various ways Module can be used:
9: // 1. Not defined. We create it here
10: // 2. A function parameter, function(moduleArg) => Promise<Module>
11: // 3. pre-run appended it, var Module = {}; ..generated code..
...
1492: var ASM_CONSTS = {
1493: 65536: () => { nonExisting(); }
1494: };
...
1662: preInit();
1663: run();
1664:
1665: // end include: postamble.js
1666:
building:ERROR: C:/Users/clb/AppData/Local/Temp/emscripten_temp/a.js:1493:17: ERROR - [JSC_UNDEFINED_VARIABLE] variable nonExisting is undeclared
1493| 65536: () => { nonExisting(); }
^^^^^^^^^^^
Now I notice here a discrepancy between Closure JS errors using EMCC_DEBUG=2
env. var. and JS errors using -sVERBOSE
(I wonder if EMCC_DEBUG=2
env.var. implies -sVERBOSE
?)
Closure errors don't offer this hint currently to use EMCC_DEBUG=2
it looks like:
C:\emsdk\emscripten\main>emcc a.c -o a.js --closure 1
building:ERROR: Closure compiler run failed:
building:ERROR: C:/Users/clb/AppData/Local/Temp/emscripten_temp_fp4_y3jx/a.js:1493:17: ERROR - [JSC_UNDEFINED_VARIABLE] variable nonExisting is undeclared
1493| 65536: () => { nonExisting(); }
^^^^^^^^^^^
1 error(s), 0 warning(s)
emcc: error: closure compiler failed (rc: 1)
and filename C:/Users/clb/AppData/Local/Temp/emscripten_temp_fp4_y3jx/a.js
above points to a file that doesn't exist after compilation.. a lot of room to improve the UX here for sure.
Thinking more about it, I don't think it is necessarily a big deal if the output goes to file or stdout. I do find that getting line-numberized output directly to stdout feels one less step (and would also be easier to capture on a CI run as well, e.g. if needed to get a debug output from a remote command?), but don't mind strongly either way here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and filename
C:/Users/clb/AppData/Local/Temp/emscripten_temp_fp4_y3jx/a.js
above points to a file that doesn't exist after compilation.. a lot of room to improve the UX here for sure.
That is kind of the main thing this change fixes. It makes the errors point to a real file, at least when -sVEBOSE
is specified.
But we already have the |
In terms of the best UI for surfacing I think we should probably (eventually) move to being consistent and choose one of these three options to use in call cases.
Currently (1) is used for JS compiler and (2) is used for closure compiler, but (3) is the standard gcc/clang name. I will open a separate bug for this. |
Interesting idea, but no, I wasn't thinking that. My position is
This feels more like EMCC_DEBUG than VERBOSE. But, I don't feel strongly. As long as it's documented, which you have done, sgtm either way. |
I've opened #25092 to consolidate the different options. This change is focused on making the filename int he |
If there are errors in JS library code we now have an options to save the preprocessed output to a temp location. From there lookup the error message and corresponding line number. For example: ``` $ emcc ~/test//hello.c error: /usr/local/google/home/sbc/dev/wasm/emscripten/src/lib/libcore.js: failure to execute js library "/usr/local/google/home/sbc/dev/wasm/emscripten/src/lib/libcore.js": error: /usr/local/google/home/sbc/dev/wasm/emscripten/src/lib/libcore.js: use -sVERBOSE to see more details Internal compiler error JS compiler Please create a bug report at https://github.com/emscripten-core/emscripten/issues/ with a log of the build and the input files used to run. Exception message: "/usr/local/google/home/sbc/dev/wasm/emscripten/src/lib/libcore.preprocessed.js:1087 ds = fd ^^^^^^^ SyntaxError: Invalid shorthand property initializer at new Script (node:vm:117:7) at createScript (node:vm:269:10) at Module.runInContext (node:vm:300:10) at runInMacroContext (file:///usr/local/google/home/sbc/dev/wasm/emscripten/src/utility.mjs:324:13) at Object.load (file:///usr/local/google/home/sbc/dev/wasm/emscripten/src/modules.mjs:291:9) at Module.runJSify (file:///usr/local/google/home/sbc/dev/wasm/emscripten/src/jsifier.mjs:354:18) at file:///usr/local/google/home/sbc/dev/wasm/emscripten/tools/compiler.mjs:97:17 ``` Building with -sVERBOSE then generates: ``` $ emcc ~/test//hello.c -sVERBOSE processing system library: /usr/local/google/home/sbc/dev/wasm/emscripten/src/lib/libint53.js processing system library: /usr/local/google/home/sbc/dev/wasm/emscripten/src/lib/libcore.js error: /usr/local/google/home/sbc/dev/wasm/emscripten/src/lib/libcore.js: failure to execute js library "/usr/local/google/home/sbc/dev/wasm/emscripten/src/lib/libcore.js": error: /usr/local/google/home/sbc/dev/wasm/emscripten/src/lib/libcore.js: See preprocessed source in /tmp/emcc-jscompiler-3XmDDr/libcore.js Internal compiler error JS compiler Please create a bug report at https://github.com/emscripten-core/emscripten/issues/ with a log of the build and the input files used to run. Exception message: "/tmp/emcc-jscompiler-3XmDDr/libcore.js:1087 ds = fd ^^^^^^^ SyntaxError: Invalid shorthand property initializer at new Script (node:vm:117:7) at createScript (node:vm:269:10) at Module.runInContext (node:vm:300:10) at runInMacroContext (file:///usr/local/google/home/sbc/dev/wasm/emscripten/src/utility.mjs:324:13) at Object.load (file:///usr/local/google/home/sbc/dev/wasm/emscripten/src/modules.mjs:291:9) at Module.runJSify (file:///usr/local/google/home/sbc/dev/wasm/emscripten/src/jsifier.mjs:354:18) at file:///usr/local/google/home/sbc/dev/wasm/emscripten/tools/compiler.mjs:97:17 ``` No you can open the file mentioned in the error message (/tmp/emcc-jscompiler-3XmDDr/libcore.js) and go to line 1087
If there are errors in JS library code we now have an option to save the preprocessed output to a temp location. From there lookup the error message and corresponding line number.
For example:
Building with -sVERBOSE then generates:
No you can open the file mentioned in the error message (/tmp/emcc-jscompiler-3XmDDr/libcore.js) and go to line 1087.
Without this change the file referenced in the error message will always be non-existant, and we would instead dump the whole file (which could be huge) to stderr.