Re: macppc libunwind without altivec

2020-04-02 Thread Mark Kettenis
> Date: Thu, 2 Apr 2020 16:05:09 -0400
> From: George Koehler 
> 
> Hello tech,
> 
> powerpc libunwind is broken on machines without altivec.  It crashes
> SIGILL when code (built with base-clang++) throws a C++ exception,
> because libunwind always saves the altivec registers.  You don't have
> altivec if sysctl machdep.altivec is 0.  I believe that G3 cpus don't
> have altivec, and G4 and G5 do have it.
> 
> This diff defers saving the altivec registers until we need to access
> them.  I took the idea from arm, which defers saving VFP registers.
> The diff fixes a small C++ demo on my G3.  I didn't try other C++
> code; I was building ports with macppc base-clang but stopped at an
> error from lang/python/3.7.
> 
> Registers_arm has members like "bool _saved_vfp_d0_d15;" to know if
> VFP got saved.  I can't add a "bool _saved_vrs" to Registers_ppc,
> because some assertions would fail, because unw_context_t would be
> too small.  I don't enlarge unw_context_t (an opaque struct of 117 * 8
> bytes) because it is in /usr/include/c++/v1/libunwind.h and I don't
> want to change the ABI.  I instead check if vrsave != 0; vrsave is a
> bitset of altivec registers in use (Altivec Programming Environments
> Manual, ALTIVECPEM.pdf, section 2.3.3).
> 
> libunwind operates on a saved context, not real registers.  If a
> compiler exists that would tell libunwind to set v31 when vrsave == 0,
> this diff would break it.  I have no bool to check if I have already
> saved the vrs but vrsave == 0.
> 
> I also stop using the red zone, because it doesn't exist: a signal
> handler may clobber anything below the stack pointer.
> 
> Is the diff OK to commit?  It applies to src/lib/libunwind but you
> build it in src/lib/libcxxabi
> 
> $ cat thrown.cpp
> #include 
> #include 
> 
> using std::cout;
> using std::runtime_error;
> 
> int
> main()
> {
>   try {
>   throw runtime_error("ouch");
>   } catch(runtime_error e) {
>   cout << "caught " << e.what() << "\n";
>   }
> }
> $ clang++ -o thrown thrown.cpp
> $ ./thrown  # without the fix
> Illegal instruction (core dumped) 
> $ ./thrown  # with the fixed libc++abi.so.2.0
> caught ouch

ok kettenis@

> Index: src/Registers.hpp
> ===
> RCS file: /cvs/src/lib/libunwind/src/Registers.hpp,v
> retrieving revision 1.8
> diff -u -p -r1.8 Registers.hpp
> --- src/Registers.hpp 17 Jun 2019 22:28:51 -  1.8
> +++ src/Registers.hpp 2 Apr 2020 16:40:32 -
> @@ -630,7 +630,7 @@ private:
>  unsigned int __lr; /* Link register */
>  unsigned int __ctr;/* Count register */
>  unsigned int __mq; /* MQ register (601 only) */
> -unsigned int __vrsave; /* Vector Save Register */
> +mutable unsigned int __vrsave; /* Vector Save Register */
>};
>  
>struct ppc_float_state_t {
> @@ -640,9 +640,11 @@ private:
>  unsigned int __fpscr; /* floating point status register */
>};
>  
> +  void saveVectorRegisters() const;
> +
>ppc_thread_state_t _registers;
>ppc_float_state_t  _floatRegisters;
> -  v128   _vectorRegisters[32]; // offset 424
> +  mutable v128   _vectorRegisters[32]; // offset 424
>  };
>  
>  inline Registers_ppc::Registers_ppc(const void *registers) {
> @@ -657,10 +659,8 @@ inline Registers_ppc::Registers_ppc(cons
>   sizeof(_floatRegisters));
>static_assert(sizeof(ppc_thread_state_t) + sizeof(ppc_float_state_t) == 
> 424,
>  "expected vector register offset to be 424 bytes");
> -  memcpy(_vectorRegisters,
> - static_cast(registers) + 
> sizeof(ppc_thread_state_t) +
> - sizeof(ppc_float_state_t),
> - sizeof(_vectorRegisters));
> +  // no values until saveVectorRegisters()
> +  memset(&_vectorRegisters, 0, sizeof(_vectorRegisters));
>  }
>  
>  inline Registers_ppc::Registers_ppc() {
> @@ -780,6 +780,7 @@ inline uint32_t Registers_ppc::getRegist
>case UNW_PPC_CR7:
>  return (_registers.__cr & 0x000F);
>case UNW_PPC_VRSAVE:
> +saveVectorRegisters();
>  return _registers.__vrsave;
>}
>_LIBUNWIND_ABORT("unsupported ppc register");
> @@ -932,6 +933,7 @@ inline void Registers_ppc::setRegister(i
>  _registers.__cr |= (value & 0x000F);
>  return;
>case UNW_PPC_VRSAVE:
> +saveVectorRegisters();
>  _registers.__vrsave = value;
>  return;
>  // not saved
> @@ -976,12 +978,14 @@ inline bool Registers_ppc::validVectorRe
>  
>  inline v128 Registers_ppc::getVectorRegister(int regNum) const {
>assert(validVectorRegister(regNum));
> +  saveVectorRegisters();
>v128 result = _vectorRegisters[regNum - UNW_PPC_V0];
>return result;
>  }
>  
>  inline void Registers_ppc::setVectorRegister(int regNum, v128 value) {
>assert(validVectorRegister(regNum));
> +  saveVectorRegisters();
>_vectorRegisters[regNum - UNW_PPC_V0] = value;
>  }
>  
> Index: src/UnwindRegistersRestore.S
> 

macppc libunwind without altivec

2020-04-02 Thread George Koehler
Hello tech,

powerpc libunwind is broken on machines without altivec.  It crashes
SIGILL when code (built with base-clang++) throws a C++ exception,
because libunwind always saves the altivec registers.  You don't have
altivec if sysctl machdep.altivec is 0.  I believe that G3 cpus don't
have altivec, and G4 and G5 do have it.

This diff defers saving the altivec registers until we need to access
them.  I took the idea from arm, which defers saving VFP registers.
The diff fixes a small C++ demo on my G3.  I didn't try other C++
code; I was building ports with macppc base-clang but stopped at an
error from lang/python/3.7.

Registers_arm has members like "bool _saved_vfp_d0_d15;" to know if
VFP got saved.  I can't add a "bool _saved_vrs" to Registers_ppc,
because some assertions would fail, because unw_context_t would be
too small.  I don't enlarge unw_context_t (an opaque struct of 117 * 8
bytes) because it is in /usr/include/c++/v1/libunwind.h and I don't
want to change the ABI.  I instead check if vrsave != 0; vrsave is a
bitset of altivec registers in use (Altivec Programming Environments
Manual, ALTIVECPEM.pdf, section 2.3.3).

libunwind operates on a saved context, not real registers.  If a
compiler exists that would tell libunwind to set v31 when vrsave == 0,
this diff would break it.  I have no bool to check if I have already
saved the vrs but vrsave == 0.

I also stop using the red zone, because it doesn't exist: a signal
handler may clobber anything below the stack pointer.

Is the diff OK to commit?  It applies to src/lib/libunwind but you
build it in src/lib/libcxxabi

$ cat thrown.cpp
#include 
#include 

using std::cout;
using std::runtime_error;

int
main()
{
try {
throw runtime_error("ouch");
} catch(runtime_error e) {
cout << "caught " << e.what() << "\n";
}
}
$ clang++ -o thrown thrown.cpp
$ ./thrown  # without the fix
Illegal instruction (core dumped) 
$ ./thrown  # with the fixed libc++abi.so.2.0
caught ouch

Index: src/Registers.hpp
===
RCS file: /cvs/src/lib/libunwind/src/Registers.hpp,v
retrieving revision 1.8
diff -u -p -r1.8 Registers.hpp
--- src/Registers.hpp   17 Jun 2019 22:28:51 -  1.8
+++ src/Registers.hpp   2 Apr 2020 16:40:32 -
@@ -630,7 +630,7 @@ private:
 unsigned int __lr; /* Link register */
 unsigned int __ctr;/* Count register */
 unsigned int __mq; /* MQ register (601 only) */
-unsigned int __vrsave; /* Vector Save Register */
+mutable unsigned int __vrsave; /* Vector Save Register */
   };
 
   struct ppc_float_state_t {
@@ -640,9 +640,11 @@ private:
 unsigned int __fpscr; /* floating point status register */
   };
 
+  void saveVectorRegisters() const;
+
   ppc_thread_state_t _registers;
   ppc_float_state_t  _floatRegisters;
-  v128   _vectorRegisters[32]; // offset 424
+  mutable v128   _vectorRegisters[32]; // offset 424
 };
 
 inline Registers_ppc::Registers_ppc(const void *registers) {
@@ -657,10 +659,8 @@ inline Registers_ppc::Registers_ppc(cons
  sizeof(_floatRegisters));
   static_assert(sizeof(ppc_thread_state_t) + sizeof(ppc_float_state_t) == 424,
 "expected vector register offset to be 424 bytes");
-  memcpy(_vectorRegisters,
- static_cast(registers) + sizeof(ppc_thread_state_t) +
- sizeof(ppc_float_state_t),
- sizeof(_vectorRegisters));
+  // no values until saveVectorRegisters()
+  memset(&_vectorRegisters, 0, sizeof(_vectorRegisters));
 }
 
 inline Registers_ppc::Registers_ppc() {
@@ -780,6 +780,7 @@ inline uint32_t Registers_ppc::getRegist
   case UNW_PPC_CR7:
 return (_registers.__cr & 0x000F);
   case UNW_PPC_VRSAVE:
+saveVectorRegisters();
 return _registers.__vrsave;
   }
   _LIBUNWIND_ABORT("unsupported ppc register");
@@ -932,6 +933,7 @@ inline void Registers_ppc::setRegister(i
 _registers.__cr |= (value & 0x000F);
 return;
   case UNW_PPC_VRSAVE:
+saveVectorRegisters();
 _registers.__vrsave = value;
 return;
 // not saved
@@ -976,12 +978,14 @@ inline bool Registers_ppc::validVectorRe
 
 inline v128 Registers_ppc::getVectorRegister(int regNum) const {
   assert(validVectorRegister(regNum));
+  saveVectorRegisters();
   v128 result = _vectorRegisters[regNum - UNW_PPC_V0];
   return result;
 }
 
 inline void Registers_ppc::setVectorRegister(int regNum, v128 value) {
   assert(validVectorRegister(regNum));
+  saveVectorRegisters();
   _vectorRegisters[regNum - UNW_PPC_V0] = value;
 }
 
Index: src/UnwindRegistersRestore.S
===
RCS file: /cvs/src/lib/libunwind/src/UnwindRegistersRestore.S,v
retrieving revision 1.8
diff -u -p -r1.8 UnwindRegistersRestore.S
--- src/UnwindRegistersRestore.S17 Jun 2019 22:28:51 -  1.8
+++ src/UnwindRegistersRestore.S2 Apr 2020 16:40:33 -
@@ -476,11