Improve error message with 'undefined top-level compiled C/C++ code' #20536
Conversation
| Btw, what is the status of longjmp support with Wasm exception handling? It looks like it is disabled by default, and I have to pair |
cc @aheejin |
kripken left a comment
kripken left a comment There was a problem hiding this comment.
Approach lgtm - LINKER_INPUTS seems reasonable to me actually (as an internal setting, I see no problem there).
| logger.info('logging stderr in js compiler phase into %s' % stderr_file) | ||
| stderr_file = open(stderr_file, 'w') | ||
| else: | ||
| stderr_file = subprocess.PIPE |
There was a problem hiding this comment.
iiuc this stashes stderr so you can read it below, but I don't see you print it out later, so that means stderr is no longer visible to the user? There are warnings that come from the JS compiler that we need to show iirc.
| I'm a little confused by this change since I've spent a lot of effort to get wasm-ld to report these errors precisely, so we should need this kind of thing: For a long time I had this behind So in the general case I think this problem is already solved. There must be something about your build that is preventing wasm-ld from reporting these errors? Or there must be something specific about those symbols? |
| It looks like the LLD_REPORT_UNDEFINED change was back in 3.1.28: I think maybe the symbols in question are somehow special since they are generated by the compiler and this problem is solved for normal symbols. I think maybe if I can reproduce this then I can fix this on the llvm side. I'm hoping we can have wasm-ld be the one that reports all these errors in single place. |
| Yup, it looks like this is the issue in LLVM: https://github.com/llvm/llvm-project/blob/c3629923aa2c28c912a08556950e45225ddc8db7/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp#L449-L465 This assigns import names to these symbols which tells the linker to assume it can import them (and therefore assume they are defined). Perhaps we can remove this function completely. |
| Sorry for all the comments :) Firstly this issue (IIUC) only effects the magic symbol related to exceptions and SJLJ and is not a widespread/general issue. I've got an llvm patch out that should fix this (at least for all the symbols listed in this bug): llvm/llvm-project#71599. |
Sorry for the delayed reply. The longjmp setting defaults to whatever EH setting you are using. So if you have |
…ated function declarations These days we would prefer that the linker report errors when these symbols are not defined. Assigning import names like this tells the linker not to report errors when these symbols are missing and simply import them. However, this leads to much less useful undefined symbol errors after the linker is done. Note that keep the old behavior for the `invoke_xxx` functions since its not possible for is to declare all possible permutations ahead of time in a library. See emscripten-core/emscripten#20536
juj force-pushed the improve_c_to_js_undef_error_messages branch from 7366a34 to de80de4 Compare | Updated this PR now. Though I am a bit confused when writing a test for this. In our Unity codebase we saw the C->JS linkage being handled by compiler.js, and hence getting those "referenced by top-level compiled C/C++ code" errors. We are using Emscripten 3.1.38 there, so that should have had the change from 3.1.28. I find in #19588 Sam changed the error message from
into
I am not completely sure how to coax that error path out now in order to add a test. I started trying with something like this, but that code flow does go to |
| The reason I changed the error message from |
Yes, this is exactly the hope. The idea is that wasm-ld will always give good error messages and there should be no need to parse its output or perform llvm-nm. At least that is my hope. There were some recent improvements to error report for JS library symbol depenedencies made in #19843 which landed in 3.1.44. There was also a specific fixes that I landed on the llvm side, relating to the internal SJLJ symbols: llvm/llvm-project#71599. I believe these were ones that were causing the problems in the orginal bug report above. |
| In other words, as far as I know, undefined symbols in native code should always now be reported by wasm-ld. |
| Closing old stale PR. |
4 participants
In #20534 the error message from the link produces an unhelpful
In our huge Unity build system, I am likewise troubleshooting the same above errors, and also a separate build error with
In a vast codebase that is built with several different build systems from external third party libraries, the above makes it impossible to understand where the source is coming from.
This PR improves Emscripten to report more helpful error messages:
that is a massive aid for the developer figure out where to go look at.
Needs a better solution than the
settings.LINKER_INPUTSvariable, feedback welcome. Also could not useshared.run_js_toolbecause it does not allow receivingstderrback.