Skip to content

Correctly update .debug_addr DWARF section. #3461

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bmeurer
Copy link

@bmeurer bmeurer commented Dec 23, 2020

This is a minimal patch to add support for also updating the addresses
stored in the .debug_addr DWARF section, which is used to perform
address resolution at debug time with debug fission. This is minimal in
the sence that it only supports the pre-DWARFv5 .debug_addr format that
is emitted by Emscripten currently, and which only consists of a simple
list of addresses.

Ideally the copy of DWARFYAML in the Binaryen tree should be updated
with LLVM ToT at some point, which will provide full support for DWARFv5
index tables, including the new .debug_addr format.

Ref: #3460, emscripten-core/emscripten#13099
Bug: https://crbug.com/1161422

@bmeurer bmeurer force-pushed the issue/3640_debug_addr branch from dc96cd3 to f90fc1d Compare December 23, 2020 08:49
This is a minimal patch to add support for also updating the addresses
stored in the .debug_addr DWARF section, which is used to perform
address resolution at debug time with debug fission. This is minimal in
the sence that it only supports the pre-DWARFv5 .debug_addr format that
is emitted by Emscripten currently, and which only consists of a simple
list of addresses.

Ideally the copy of DWARFYAML in the Binaryen tree should be updated
with LLVM ToT at some point, which will provide full support for DWARFv5
index tables, including the new .debug_addr format.

Ref: WebAssembly#3460
Ref: emscripten-core/emscripten#13099
Bug: https://crbug.com/1161422
@bmeurer bmeurer force-pushed the issue/3640_debug_addr branch from f90fc1d to 5ca145c Compare December 23, 2020 09:43
@bmeurer
Copy link
Author

bmeurer commented Dec 23, 2020

We just realized that this naive approach causes breakage for globals, since with the pre-DWARFv5 .debug_addr, there's no way to tell which addresses refer to CODE (in the Wasm binary) and which ones refer to DATA (in linear memory), and wasm-emscripten-finalize will apply location updates for CODE motion to DATA addresses incorrectly.

Ideally for debugging, there'd be no post-processing by Binaryen, and just leave the binary produced by clang as-is. The only viable alternative to unblock Debug Fission seems to be moving to DWARFv5 and using the segment selectors to distinguish code and data addresses. But that does of course come with it's own set of problems (also a much larger surgery to Binaryen I suppose).

cc @pfaffe @dschuff @kripken @sbc100

@sbc100 sbc100 requested a review from kripken December 23, 2020 17:27
@kripken
Copy link
Member

kripken commented Jan 4, 2021

The code here looks good to me, but sounds like we think the direction is not. I had not known this stuff about .debug_addr, interesting...

Ideally for debugging, there'd be no post-processing by Binaryen, and just leave the binary produced by clang as-is.

I agree. Is something preventing that atm? Building with -sWASM_BIGINT and with less than -O2 would do that. Do we have an urgent use case that can't do those things?

@bmeurer
Copy link
Author

bmeurer commented Jan 7, 2021

Ah, indeed, you're right, I forgot to pass -sWASM_BIGINT. Passing that flag, it starts to work for the split-dwarf-demo example. This is BTW quite confusing, since I'm not using any int64s, so I wasn't expecting -sWASM_BIGINT to have any effect.

@kripken
Copy link
Member

kripken commented Jan 7, 2021

@bmeurer Yes, sorry about that, it is indeed confusing. We need to handle i64s in some basic syscalls, sadly, so even if the user doesn't have i64s themselves, WASM_BIGINT makes a difference in the build.

Base automatically changed from master to main January 19, 2021 21:59
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

Successfully merging this pull request may close these issues.

2 participants