Page MenuHomePhabricator

[libunwind] Support dwarf unwinding on i386 windows
ClosedPublic

Authored by mstorsjo on Oct 9 2017, 2:04 AM.

Details

Summary

This builds on some parts from D33601 - some of them were commented on before. This patch contains comments and explanations about those non-obvious parts.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Oct 9 2017, 2:04 AM
rnk added inline comments.Oct 9 2017, 10:22 AM
src/AddressSpace.hpp
388–389 ↗(On Diff #118187)

".eh_frame" is 9 characters, right? I thought mingw linkers took sections with long names and moved them to an extended symbol table. Does that not apply to .eh_frame?

mstorsjo added inline comments.Oct 9 2017, 12:09 PM
src/AddressSpace.hpp
388–389 ↗(On Diff #118187)

Yes, they do, so this actually only matches .eh_fram. I didn't yet look at how to navigate the IMAGE_*_HEADERS structs to find the coresponding full long name.

jroelofs added inline comments.
src/AddressSpace.hpp
521 ↗(On Diff #118187)

Would it work to implement the win32 side of this via SymFromAddr?

src/UnwindRegistersRestore.S
29 ↗(On Diff #118187)

Please invert the condition, and swap the if/else on this. That will make it more straightforward to adjust this for other platforms later.

mstorsjo added inline comments.Oct 9 2017, 12:32 PM
src/AddressSpace.hpp
521 ↗(On Diff #118187)

Hmm, I guess that would work.

src/UnwindRegistersRestore.S
29 ↗(On Diff #118187)

Ok, will do.

rnk added inline comments.Oct 9 2017, 12:40 PM
src/AddressSpace.hpp
388–389 ↗(On Diff #118187)

Can you add a FIXME here? No need to read the long-form symbol table yet, I just want to document that we will need that for compatibility with ld.bfd.

mstorsjo added inline comments.Oct 9 2017, 1:15 PM
src/AddressSpace.hpp
521 ↗(On Diff #118187)

... actually, I'm not sure how useful it is - it requires initializing the symbol handler with SymInitialize and point to a path to find the symbols. Plus that the symbol handler is single threaded and any calls to that would need to be guarded with a global mutex. So I think I'd defer that for now at least.

mstorsjo updated this revision to Diff 118263.Oct 9 2017, 1:18 PM

Added a fixme comment about the truncated section name, flipped the ifdef in the assembly source. Didn't implement findFunctionName.

jroelofs added inline comments.Oct 9 2017, 1:19 PM
src/AddressSpace.hpp
521 ↗(On Diff #118187)

alright, fine with me.

mstorsjo added inline comments.Oct 9 2017, 1:19 PM
docs/index.rst
53 ↗(On Diff #118263)

FWIW, for this to actually work correct which this docs change claims, this also depends on D38680.

Are there any further comments on this, or can someone of those who commented on it before approve it?

This revision is now accepted and ready to land.Oct 11 2017, 7:30 AM

LGTM

Thanks, will push this one without the docs update since it's still buggy in practice on windows until D38680 gets resolved.

This revision was automatically updated to reflect the committed changes.