[PATCH] D41968: [libunwind][MIPS] Support MIPS floating-point registers for hard-float ABIs.

[Simon Dardis via Phabricator via cfe-commits]

sdardis added a reviewer: compnerd.
sdardis added a comment.

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 https://reviews.llvm.org/source/compiler-rt/ in strict fp32 mode as that doesn't have double precision at all. But there's more work to be done for https://reviews.llvm.org/source/compiler-rt/ 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.



================
Comment at: src/Registers.hpp:2659
+  uint32_t _padding;
+  double _floats[32];
+#endif
----------------
bsdjhb wrote:
> I chose to always use double here to avoid having different context sizes for the 32-bit vs 64-bit FPR cases.
Add a comment highlighting that design choice.


================
Comment at: src/UnwindRegistersRestore.S:647
+#if __mips_fpr == 32
+  l.d   $f0, (4 * 36 + 8 * 0)($4)
+  l.d   $f2, (4 * 36 + 8 * 2)($4)
----------------
Use ldc1 instead of l.d . l.d is an alias of ldc1.


================
Comment at: src/UnwindRegistersRestore.S:755
+#ifdef __mips_hard_float
+  l.d   $f0, (8 * 35)($4)
+  l.d   $f1, (8 * 36)($4)
----------------
ldc1 here too.


================
Comment at: src/UnwindRegistersSave.S:119
 
-#elif defined(__mips__) && defined(_ABIO32) && defined(__mips_soft_float)
+#elif defined(__mips__) && defined(_ABIO32)
 
----------------
&& _MIPS_SIM == _ABIO32


================
Comment at: src/UnwindRegistersSave.S:172
+#if __mips_fpr == 32
+  s.d   $f0, (4 * 36 + 8 * 0)($4)
+  s.d   $f2, (4 * 36 + 8 * 2)($4)
----------------
Use sdc1 rather than s.d.


================
Comment at: src/UnwindRegistersSave.S:228
 
-#elif defined(__mips__) && (defined(_ABI64) || defined(_ABIN32)) &&            \
-    defined(__mips_soft_float)
+#elif defined(__mips__) && (defined(_ABI64) || defined(_ABIN32))
 
----------------
defined(__mips64) rather than the checking if the ABI names are defined.


================
Comment at: src/UnwindRegistersSave.S:280
+#ifdef __mips_hard_float
+  s.d   $f0, (8 * 35)($4)
+  s.d   $f1, (8 * 36)($4)
----------------
sdc1 rather s.d


================
Comment at: src/libunwind.cpp:64
 # define REGISTER_KIND Registers_or1k
-#elif defined(__mips__) && defined(_ABIO32) && defined(__mips_soft_float)
+#elif defined(__mips__) && defined(_ABIO32)
 # define REGISTER_KIND Registers_mips_o32
----------------
Check for that _MIPS_SIM == _ABIO32 as well.


================
Comment at: src/libunwind.cpp:66
 # define REGISTER_KIND Registers_mips_o32
-#elif defined(__mips__) && (defined(_ABIN32) || defined(_ABI64)) &&            \
-    defined(__mips_soft_float)
+#elif defined(__mips__) && (defined(_ABIN32) || defined(_ABI64))
 # define REGISTER_KIND Registers_mips_newabi
----------------
Check for __mips64 rather than defined(_ABIXXX).


https://reviews.llvm.org/D41968