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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to