This is an archive of the discontinued LLVM Phabricator instance.

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

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

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

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

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

src/UnwindRegistersRestore.S
492

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

500

Prefix this with:

.set push
.set nomacro

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

539
.set pop
540

defined(mips) && _MIPS_SIM == _ABI64

550

See my comments on the o32 assembly.

src/UnwindRegistersSave.S
90

See my previous comments about this preprocessor macro.

101

Wrap this with

.set push
.set noat
.set nomacro
120

and:

.set pop
121

Again, see my previous comments on this preprocessor define.

132

Wrap this with

.set push
.set noat
.set nomacro
151
.set pop
src/libunwind.cpp
66

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

Needs checking for soft-float.

543

Needs checking for soft-float.

src/UnwindRegistersSave.S
90

Needs checking for soft-float.

125

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

Requires HI / LO registers.

src/Registers.hpp
2014

This appears to be missing the HI / LO registers.

2039

FIXME: Hard float, DSP accumulator registers, MSA registers

src/UnwindRegistersSave.S
120–122

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

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

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

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

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
63

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
2003

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.