This supports the soft-float ABI only and has been tested with both clang
and gcc on FreeBSD.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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).
+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! |
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.
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?
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. |
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. |
- 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.
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. |
src/UnwindRegistersSave.S | ||
---|---|---|
100 ↗ | (On Diff #118960) | I was about to ask if that would be better as well. |
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. |
- Rebase on more MAX_REGISTER changes.
- Use macro for lastDwarfRegisterNumber.
- Move MIPS ABI constants under a single #ifdef mips.
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.