This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] Fix building for ARM with dwarf exception handling
ClosedPublic

Authored by mstorsjo on Oct 24 2017, 11:56 AM.

Details

Summary

The previous definition of _LIBUNWIND_HIGHEST_DWARF_REGISTER seems to be a copy of the ARM64 value (introduced in SVN r276128); since the code actually hasn't compiled properly for arm in dwarf mode before, this hasn't actually been used. Set it to the correct value based on the UNW_ARM_* enum values.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Oct 24 2017, 11:56 AM
compnerd edited edge metadata.Oct 24 2017, 9:21 PM

Whats the motivation for adding DWARF based unwinding on ARM? What environment is using this?

src/Registers.hpp
1342 ↗(On Diff #120110)

Can we not use _LIBUNWIND_HIGHEST_DWARF_REGISTER here?

src/UnwindLevel1.c
79 ↗(On Diff #120110)

Please convert these to %p instead. The casting and conversion is kinda ... unnecessary.

Whats the motivation for adding DWARF based unwinding on ARM? What environment is using this?

AFAIK NetBSD does.

And my actual target is for MinGW/ARM; it seemed to be less effort to make libunwind work for ARM/DWARF than to make the COFF/ARM backend in LLVM output EHABI (but I might have been mistaken).

src/Registers.hpp
1342 ↗(On Diff #120110)

I guess we could - we could do that consistently throughout all the Registers_* classes then. Or even omit the method at all and just check _LIBUNWIND_HIGHEST_DWARF_REGISTER in the caller? Or perhaps the last part clashes with part of the (incomplete afaik?) intended design to support unwinding foreign systems as well.

src/UnwindLevel1.c
79 ↗(On Diff #120110)

Using %p wouldn't work when on 32 bit systems currently, while most of these fields are defined to be uint64_t except for the arm ehabi case. I guess I could look at how hard it would be to switch it to uint64_t for the arm/dwarf case as well.

mstorsjo added inline comments.Oct 25 2017, 1:32 AM
src/UnwindLevel1.c
79 ↗(On Diff #120110)

This part can be avoided if we consistently use unw_word_t == uint64_t, as done in D39280.

mstorsjo added inline comments.Oct 25 2017, 1:39 AM
src/Registers.hpp
1342 ↗(On Diff #120110)

This gets taken care of for the existing ones in D39281 - if that one goes through I'll update this patch accordingly.

aprantl removed a subscriber: aprantl.Oct 25 2017, 9:02 AM
mstorsjo updated this revision to Diff 120863.Oct 30 2017, 12:15 PM
mstorsjo edited the summary of this revision. (Show Details)

Avoiding duplicating the highest register number, avoiding any further casts in log lines.

mstorsjo marked an inline comment as done.Oct 30 2017, 12:15 PM
compnerd accepted this revision.Nov 1 2017, 10:33 PM
compnerd added inline comments.
src/Registers.hpp
1481 ↗(On Diff #120863)

Why the change to mark these as mutable?

This revision is now accepted and ready to land.Nov 1 2017, 10:33 PM
mstorsjo added inline comments.Nov 2 2017, 12:29 AM
src/Registers.hpp
1481 ↗(On Diff #120863)

These are touched from within getRegister, but we need to make getRegister const since it's used on a const Registers object in the dwarf codepath.

This revision was automatically updated to reflect the committed changes.