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

Enforce DWARF v5 #14241

Open
pfaffe opened this issue May 20, 2021 · 9 comments
Open

Enforce DWARF v5 #14241

pfaffe opened this issue May 20, 2021 · 9 comments

Comments

@pfaffe
Copy link
Collaborator

pfaffe commented May 20, 2021

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.

@kripken
Copy link
Member

kripken commented May 20, 2021

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?).

@dschuff
Copy link
Member

dschuff commented May 25, 2021

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?
Based on your last sentence, I guess the answer is probably "no", but then I'm confused about how anything is working now. We are just generating invalid dwarf4 and it happens to work?

@pfaffe
Copy link
Collaborator Author

pfaffe commented May 25, 2021

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.

@kripken
Copy link
Member

kripken commented May 26, 2021

A quick test on binaryen's ability to handle DWARF 5:

$ emcc tests/hello_world.c -gdwarf-5 -s WASM_BIGINT
$ wasm-opt a.out.wasm -o b.out.wasm
error: invalid reference to or invalid content in .debug_str_offsets[.dwo]: length exceeds section sizecompile unit size was incorrect
UNREACHABLE executed at third_party/llvm-project/DWARFEmitter.cpp:200!
Aborted

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.

@sbc100
Copy link
Collaborator

sbc100 commented May 26, 2021

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.

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.

Currently we are in fact generating invalid dwarf4, but the debugger doesn't verify this and just does the right thing.

@dschuff
Copy link
Member

dschuff commented May 26, 2021

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.
One thing I'm not totally clear on, is just how broken is the dwarf4 debug info? Or really, what is the impact? Would it result in lots of debug info not being usable for users? Or result in lots of pain in lldb trying to support it? etc

@pfaffe
Copy link
Collaborator Author

pfaffe commented May 26, 2021

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).

@sbc100
Copy link
Collaborator

sbc100 commented May 26, 2021

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 -gdwarf-5 flag?

@aardappel
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants