Page MenuHomePhabricator

[libunwind][MIPS] Support MIPS floating-point registers for hard-float ABIs.
ClosedPublic

Authored by bsdjhb on Jan 11 2018, 3:17 PM.

Details

Summary

For MIPS ABIs with 64-bit floating point registers including newabi
and O32 with 64-bit floating point registers, just save and restore the
32 floating-point registers as doubles.

For O32 MIPS with 32-bit floating-point registers, save and restore the
individual floating-point registers as "plain" registers. These registers
are encoded as floats rather than doubles, but the DWARF unwinder
assumes that floating-point registers are stored as doubles when reading
them from memory (via AddressSpace::getDouble()). Treating the
registers as "normal" registers instead causes the DWARF unwinder to
fetch them from memory as a 32-bit register. This does mean that for
O32 with 32-bit floating-point registers unw_get_fpreg() and
unw_set_fpreg() do not work. One would have to use unw_get_reg()
and unw_set_reg() instead. However, DWARF unwinding works
correctly as the DWARF CFI emits records for individual 32-bit
floating-point registers even when they are treated as doubles stored
in paired registers. If the lack of unw_get/set_fpreg() becomes a pressing
need in the future for O32 MIPS we could add in special handling to
make it work.

Diff Detail

Repository
rL LLVM

Event Timeline

bsdjhb created this revision.Jan 11 2018, 3:17 PM
bsdjhb added inline comments.Jan 11 2018, 3:20 PM
src/Registers.hpp
2659 ↗(On Diff #129538)

I chose to always use double here to avoid having different context sizes for the 32-bit vs 64-bit FPR cases.

I am not sure what the answer is. There's a slightly different issue in that returning the contents of a floating point register on MIPS and expecting it to be a double is not necessarily correct. For a 64bit FPU, the result of lwc1/mtc1 leaves the upper 32bits of an floating point register as UNPREDICTABLE by the architecture definition. It's entirely possible for the application being unwound to be operating on single precision values, but the contents of the register in it's entirety are a signalling nan.

I think the least worst option for registers is to always present the data as double values and to only permit access to the sets of double registers for O32, and leave the API user to determine how to proceed.

Unfortunately this complicates unwinding in the case of R6 in strict fp32 mode as that doesn't have double precision at all. But there's more work to be done for R6 support anyway, so it can be deferred. Some nits inlined. Also, watch out for ABI guards such as (defined(_ABIN32) || defined(_ABI64), these can be replaced with __mips64.

src/Registers.hpp
2659 ↗(On Diff #129538)

Add a comment highlighting that design choice.

src/UnwindRegistersRestore.S
647 ↗(On Diff #129538)

Use ldc1 instead of l.d . l.d is an alias of ldc1.

755 ↗(On Diff #129538)

ldc1 here too.

src/UnwindRegistersSave.S
119 ↗(On Diff #129538)

&& _MIPS_SIM == _ABIO32

172 ↗(On Diff #129538)

Use sdc1 rather than s.d.

228 ↗(On Diff #129538)

defined(__mips64) rather than the checking if the ABI names are defined.

280 ↗(On Diff #129538)

sdc1 rather s.d

src/libunwind.cpp
64 ↗(On Diff #129538)

Check for that _MIPS_SIM == _ABIO32 as well.

66 ↗(On Diff #129538)

Check for __mips64 rather than defined(_ABIXXX).

After thinking about this some more, I need to rework this a bit. The choice of how to expose the floating point registers via getFloatingPointRegister / setFloatingPointRegister only affects consumers of the libunwind unw_get_fpreg/unw_set_fpreg. I think a bigger factor is that we need to honor unwind rules in frames that describe the saved location of FP registers on the stack (which this patchset doesn't do). I think that's fairly trivial for the case where an FP register is the same size as a GP register, and it probably argues for storing 32-bit FP registers as individual 'float' if that is how the unwind information is encoded. I haven't yet looked to see what the implications would be for O32 MIPS with 64-bit FP registers. It may be that I punt on that configuration for now.

Hmmm, so I was somewhat mistaken as DwarfInstructions.hpp::stepWithDwarf() does use the Register class's setFloatRegister(), however, it assumes that the floating point register is always a double (DwarfInstructions.hpp::getSavedFloatRegister() uses AddressSpace::getDouble() to read a double from memory). So I think that means that the NEWABI cases work as-is. O32 with 64-bit FP registers should also work as-is. However, O32 with 32-bit FP registers will not quite work. The DWARF info for O32 with 32-bit registers does save/restore individual 32-bit registers:

000006dc 00000034 000006e0 FDE cie=00000000 pc=0000d384..0000d46c
  DW_CFA_advance_loc4: 16 to 0000d394
  DW_CFA_def_cfa_offset: 56
  DW_CFA_advance_loc4: 20 to 0000d3a8
  DW_CFA_offset: r31 at cfa-12
  DW_CFA_offset: r17 at cfa-16
  DW_CFA_offset: r16 at cfa-20
  DW_CFA_offset: r52 at cfa-4
  DW_CFA_offset: r53 at cfa-8
...

If the compiler happens to always save and restore them in pairs then the current approach will work. If we don't want to assume that they are saved and restored in pairs, then we could instead perhaps claim that 32-bit FP registers aren't really floating point but are just plain 32-bit registers. This would make unwinding work (I believe), but would mean that one would need to use unw_get_reg() instead of unw_get_fpreg() to fetch individual 32-bit FP registers.

bsdjhb updated this revision to Diff 136164.Feb 27 2018, 2:45 PM
  • Rebase after N32 commit.
  • Use ldc1/sdc1 rather than l.d and s.d.
bsdjhb marked 8 inline comments as done.Feb 27 2018, 2:48 PM
bsdjhb updated this revision to Diff 137085.Mar 5 2018, 2:45 PM
  • Add a comment about using a single FP layout for O32.
  • Treat 32-bit floating point registers on O32 as plain registers.
bsdjhb marked 2 inline comments as done.Mar 5 2018, 2:50 PM

This version follows the suggestion I made earlier of treating 32-bit floating point registers as a "plain" register for O32 rather than a floating-point register. It seems to work for me though for O32 I've only tested with 32-bit floating point registers. If we stick with this approach for 32-bit floating point it might be simpler and/or more readable to make _floats[] an array of uint32_t for that case and just use different context sizes for the O32 with 32-bit fp vs O32 with 64-bit fp. This version works in my testing on FreeBSD for O32 with 32-bit FPR, N32, and N64.

@sdardis ping. I think the approach I've used for O32 is probably the right one in that it matches what DWARF expects (DWARF doesn't treat the 32-bit floating point registers as pairs but as individual registers). I think the question is if I O32 with 32-bit FP registers should use an array of float[]s instead of an array of double[]s though (which adds an extra #ifdef for context size) vs keeping the current layout.

bsdjhb edited the summary of this revision. (Show Details)Mar 20 2018, 8:38 AM
sdardis accepted this revision.May 3 2018, 5:03 AM

Sorry for the delay.

LGTM.

This revision is now accepted and ready to land.May 3 2018, 5:03 AM
This revision was automatically updated to reflect the committed changes.