Page MenuHomePhabricator

[libunwind][MIPS]: Add support for unwinding in O32 and N64 processes.
ClosedPublic

Authored by bsdjhb on Sep 20 2017, 4:30 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

bsdjhb created this revision.Sep 20 2017, 4:30 PM

I have not managed to get the libunwind tests to run, but I was able to run test C++ programs on FreeBSD for both O32 and N64 that caught exceptions. The first test program just uses _Unwind_Backtrace() with a callback to print out the PC values, throws an integer whose value is output in a catch clause, and finally throws a structure whose members are output in a catch clause. I have also built and run a larger real-world C++ program which makes extensive use of exceptions (gdb) on both O32 and N64.

If desired, I can split the O32 vs N64 into two separate patches. I also have another patchset for MIPS N32 that I will post in a separate review once this one has been reviewed.

Note: My n64 changes (and, by the look of it, the o32 changes) only supported the soft-float ABI and so need a check for defined(__mips_soft_float), and should still #error if we're not saving the floating point registers. We're currently using soft float for CHERI, I wasn't able to test a hard-float version (though it should be fairly trivial to add).

sdardis edited subscribers, added: cfe-commits; removed: llvm-commits.Sep 27 2017, 1:48 AM

+CC cfe-commits, -CC llvm-commits.

Some comments inlined.

I've managed to run the test-suite on one of my machines here and I'm seeing 3 failures:

libunwind :: libunwind_01.pass.cpp
libunwind :: libunwind_02.pass.cpp
libunwind :: unw_getcontext.pass.cpp

This is with a GCC toolchain, 6.3. Did you test with clang?

include/__libunwind_config.h
54 ↗(On Diff #116112)

Can you avoid using the mips_o32 macro and instead use defined(mips__) && _MIPS_SIM == _ABIO32 ?

It appears the mips_o32 is a FreeBSD specific macro. Also, test for mips_soft_float .

59 ↗(On Diff #116112)

Likewise "defined(mips) && _MIPS_SIM == _ABI64 && __mips_soft_float"

src/UnwindRegistersRestore.S
492 ↗(On Diff #116112)

defined(mips_o32) -> defined(mips__) && _MIPS_SIM == _ABIO32

500 ↗(On Diff #116112)

Prefix this with:

.set push
.set nomacro

Also, the assembler directives should come after the function declaration / defines.

539 ↗(On Diff #116112)
.set pop
540 ↗(On Diff #116112)

defined(mips) && _MIPS_SIM == _ABI64

550 ↗(On Diff #116112)

See my comments on the o32 assembly.

src/UnwindRegistersSave.S
90 ↗(On Diff #116112)

See my previous comments about this preprocessor macro.

101 ↗(On Diff #116112)

Wrap this with

.set push
.set noat
.set nomacro
120 ↗(On Diff #116112)

and:

.set pop
121 ↗(On Diff #116112)

Again, see my previous comments on this preprocessor define.

132 ↗(On Diff #116112)

Wrap this with

.set push
.set noat
.set nomacro
151 ↗(On Diff #116112)
.set pop
src/libunwind.cpp
66 ↗(On Diff #116112)

The MIPS architecture is not supported with this ABI and environment!

bsdjhb updated this revision to Diff 117438.Oct 2 2017, 3:44 PM
  • Fixes from review feedback.
bsdjhb marked 14 inline comments as done.Oct 2 2017, 3:48 PM

I have only tested this (test programs as mentioned earlier) with clang 5.0.0 (with a few patches) on o32 and n64. I am in the process of performing the same tests with GCC 6.3.0. I will also spend some time figuring out how to cross-build libunwind tests and run them inside of a qemu instance.

bsdjhb added a comment.Oct 5 2017, 9:55 AM

FYI, I was able to do simple testing (C++ programs throwing exceptions) using GCC 6.3 to compile a FreeBSD userland for both O32 and N64. Still working on cross-compiling LLVM so I can run the tests under qemu.

I keep running into issues trying to cross-build LLVM (it keeps wanting to use /usr/bin/cc or host libraries even though cross-compiling is enabled). I wonder if the failures you saw Simon might be due to https://bugs.llvm.org/show_bug.cgi?id=33858? Could you try applying the patch from D37484 along with this patchset to see if it fixes the test failures for you?

sdardis edited edge metadata.Oct 10 2017, 12:41 PM

Thanks for the pointer to that patch, I'll take a look tomorrow.

src/UnwindRegistersRestore.S
492 ↗(On Diff #117438)

Needs checking for soft-float.

543 ↗(On Diff #117438)

Needs checking for soft-float.

src/UnwindRegistersSave.S
90 ↗(On Diff #117438)

Needs checking for soft-float.

125 ↗(On Diff #117438)

Needs checking for soft-float.

bsdjhb updated this revision to Diff 118638.Oct 11 2017, 10:42 AM
  • Add more soft-float checks.
bsdjhb marked 4 inline comments as done.Oct 11 2017, 10:42 AM

I have tested this on one of my machines after removing the checks for soft float (my debian install doesn't have the necessary headers for soft-float). With the patch you've pointed out and my inline comments addressed (bar the HI / LO register comments), it passes the supplied test suite.

Can you change the checks for ABI from '_MIPS_SIM == $ABI' to 'defined($ABI)'? I mistakenly assumed they were always defined and that _MIPS_SIM was defined to be one of them but they are only defined when that particular ABI is being used.

include/libunwind.h
584 ↗(On Diff #118638)

Requires HI / LO registers.

src/Registers.hpp
2014 ↗(On Diff #118638)

This appears to be missing the HI / LO registers.

2039 ↗(On Diff #118638)

FIXME: Hard float, DSP accumulator registers, MSA registers

src/UnwindRegistersSave.S
120–122 ↗(On Diff #118638)

Move the last store out of the delay slot and put 'or $2, $zero, $zero' in the delay slot to return UNW_ESUCCESS.

155–157 ↗(On Diff #118638)

Move the last store out of the delay slot and put 'or $2, $zero, $zero' in the delay slot to return UNW_ESUCCESS.

sdardis requested changes to this revision.Oct 13 2017, 6:11 AM

Marking this as changes required to clear up my review queue - no further comments.

This revision now requires changes to proceed.Oct 13 2017, 6:11 AM
bsdjhb marked 5 inline comments as done.Oct 13 2017, 12:43 PM
bsdjhb updated this revision to Diff 118960.Oct 13 2017, 12:44 PM
bsdjhb edited edge metadata.
  • Check _ABI* rather than _MIPS_SIM.
  • Save and restore lo/hi.
  • Expand FIXME comment for more missing registers.
  • Return UNW_SUCCESS from unw_getcontext().
  • Use correct DWARF numbers for hi and lo and put hi first.
  • Bump highest DWARF number for hi and lo.
sdardis added a subscriber: compnerd.

Two last inlined comments and I think that's everything. @compnerd Have I missed anything?

src/UnwindRegistersSave.S
100 ↗(On Diff #118960)

After looking at another implementation of libunwind and the other platforms that are supported, it seems to me that we need to save all the registers.

142 ↗(On Diff #118960)

Similarly here as well.

bsdjhb updated this revision to Diff 119245.Oct 16 2017, 9:29 PM
  • Save all of the general purpose registers.
bsdjhb marked 2 inline comments as done.Oct 16 2017, 9:55 PM
bsdjhb added inline comments.
src/UnwindRegistersSave.S
100 ↗(On Diff #118960)

I was about to ask if that would be better as well.

bsdjhb marked an inline comment as done.Oct 24 2017, 2:25 AM

Ping?

bsdjhb updated this revision to Diff 120623.Oct 27 2017, 8:54 AM
  • Rebase for recent change to MAX_REGISTER meaning.
bsdjhb edited the summary of this revision. (Show Details)Oct 27 2017, 8:59 AM
compnerd added inline comments.Oct 27 2017, 9:47 AM
include/__libunwind_config.h
69 ↗(On Diff #120623)

Can we sink the two cases into the __mips__ case? Something like:

#if defined(__mips__)
# if defined(_ABIO32)
# elif defined(_ABIO64)
# elif defined(_ABIN32)
# elif defined(_ABI64)
# else
#   error "unknown MIPS ABI"
# endif
#else
src/Registers.hpp
2063 ↗(On Diff #120623)

I believe we have a macro for this.

bsdjhb updated this revision to Diff 120798.Oct 30 2017, 4:31 AM
bsdjhb marked an inline comment as done.
  • Rebase on more MAX_REGISTER changes.
  • Use macro for lastDwarfRegisterNumber.
  • Move MIPS ABI constants under a single #ifdef mips.
bsdjhb marked an inline comment as done.Oct 30 2017, 4:32 AM

Just a heads up WRT this patch; we're discussing changing the size of unw_word_t to match uintptr_t in D39365. Does that break anything for your case? It shouldn't affect what's stored in the Register class, only pointers in the unw_proc_info_t struct. Not sure which patch will get completed/merged first though.

Just a heads up WRT this patch; we're discussing changing the size of unw_word_t to match uintptr_t in D39365. Does that break anything for your case? It shouldn't affect what's stored in the Register class, only pointers in the unw_proc_info_t struct. Not sure which patch will get completed/merged first though.

Yes, I saw that. It will probably cause some breakage for the cursor size depending on which path (uint64_t always vs uintptr_t). I'm not quite sure which of those approaches is more correct to be honest. I also have an N32 patch in review but will wait until this one is finally committed before updating that further.

bsdjhb updated this revision to Diff 121396.Nov 2 2017, 3:20 PM
  • Rebase.
  • Update O32 cursor size after unw_word_t change.
compnerd accepted this revision.Dec 6 2017, 2:37 PM

LGTM if @sdardis is good with it

sdardis accepted this revision.Dec 6 2017, 4:46 PM

LGTM.

This revision is now accepted and ready to land.Dec 6 2017, 4:46 PM
This revision was automatically updated to reflect the committed changes.