-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Enforce DWARF v5 #14241
Comments
At what time do you think this change would make sense? I mean, how stable would we require DWARF5 to be in LLVM? Another factor is binaryen. We don't know at compile time if binaryen will be run at link, so changing the default could affect some use cases where binaryen does run (not common use cases, but debugging with asyncify, and release builds with debug info; maybe it's ok to do poorly with those?). |
Is there any reason we shouldn't make dwarf5 the default in clang itself? i.e. is there any reason why a non-emscripten clang users would want dwarf 4 instead? |
A reason I've heard against making dwarf5 the default is that the tooling is not up to speed yet. Kim just recently started landing missing dwarf5 features in llvm-dwp for example. Currently we are in fact generating invalid dwarf4, but the debugger doesn't verify this and just does the right thing. |
A quick test on binaryen's ability to handle DWARF 5:
Not sure how much work it would be to fix. Regardless I am not opposed to this, if want to say that release+debug (and asyncify+debug) builds are left for future work. |
But aren't you suggesting that we make dwarf5 the default in emscripten? How can it be be a good idea.. and a bad idea to switch the default? Derek, I assume you were suggesting that we switch clang to use dwarf5 only for the emscripten target? Not changing the overall default in clang for all targets. If dwarf4 really is broken for wasm it seems like that should be our long term goal.. but as alon points out our tooling (binaryen) doesn't support dwarf5 today so its not an option yet.
|
Well, I was suggesting it for the wasm target. If all of the tools that we know about in the wasm ecosystem support dwarf5 (including emscripten and the tools we expect its users to use e.g. Binaryen and llvm-dwp; lldb; chrome devtools; wasmtime; are there others in the BA ecosystem? I'm not sure I know of any others), then it would make sense to do that for all wasm targets. Otherwise if emscripten/web-focused tools support it then we could do it just for emscripten. And yes, I agree that this is the long-term goal. For the current default it seems like there are tradeoffs to make. Currently it sounds like dwarf5 will only work if you also use ERROR_ON_WASM_CHANGES_AFTER_LINK or are compatible with that. But it also gives clear scaling benefits. |
I'm suggesting only switching the default for wasm. Wasm DWARF doesn't have a big ecosystem right now and we're the ones building all the tools, so we're in a great place to make that switch. Regarding binaryen, it also doesn't support dwarf4 really. It's not widely tested, and we've repeatedly been finding bugs. Moreover, debug fission (-gsplit-dwarf) cannot be easily supported in binaryen with dwarf4 altogether (but it's possible with dwarf5). |
In that case I think we call all agree that if/when we do switch the default for wasm it should probably be done in upstream clang (but only for wasm targets)? @sunfishcode does wasmtime supports dwarf5? Do you use the |
not an expert on the matter, so take this with a grain of salt, but I would guess it be nice to make a hard switch in LLVM, i.e. stop supporting dwarf 4, to simplify our testing etc. |
When compiling with
-g
, this should be forwarded as-gdwarf-5
to clang. DWARF v5 is necessary for generating valid debug information: The wasm specifics of the symbols are based on https://yurydelendik.github.io/webassembly-dwarf/, which is an extension to the DWARF v5 standard. Location expressions for wasm values as defined in that extension are not valid in DWARF v4 because they need to be typed, otherwise expressions would be limited to 32bit (in wasm32) which couldn't support 64bit integers or floats, or simd values.The text was updated successfully, but these errors were encountered: