Recognize all NetBSD architectures in UBSan

[krytarowski]

Summary

Use uniform accessors for Program Pointer,
Stack Pointer and Frame Pointer.

Remove CPU check in UBSan supported platforms
and rely only on the OS type.

This adds NetBSD support in GetPcSpBp() for:

• ARM
• ARM64
• HPPA
• PowerPC/PowerPC64
• SPARC/SPARC64
• MIPS
• DEC Alpha AXP
• DEC VAX
• M68K and M68010
• SH3
• IA64
• OR1K
• RISCV

Sponsored by <The NetBSD Foundation>

[ro]

I'm not sure I like it this way: the patch seems extremely repetive to me. Couldn't this just (with the exception of sparc/sparcv9) define a common macro
for the bp register number and then do

          *pc = _UC_MACHINE_PC(ucontext);
	​  *sp = _UC_MACHINE_SP(ucontext);
	​  *bp = ucontext->uc_mcontext.__gregs[_REG_BP];

Maybe even factor out

ucontext_t *ucontext = (ucontext_t*)context;

in all cases?

Another nit (while you are fixing inconsistencies): we have both `(ucontext_t*)` and `(ucontext_t *)` (i.e. with and without a space) in casts.

[krytarowski]

There is an option to to attempt to push _UC_MACHINE_BP to the sources of NetBSD for all ports.

So all NetBSD ports will use _UC_MACHINE_().

What do you think?

[ro]

That's even better, of course. Way more readable.

[krytarowski]

How about the CAN_SANITIZER_UB part?

[ro]

TBH, I've always disliked it that way: my preference would be to go for the SANITIZE_<PLATFORM> forms as far as possible.

[krytarowski]

I've proposed this patch in the NetBSD source code:

http://netbsd.org/~kamil/patch-00044-_UC_MACHINE_BP.txt

Thread: http://mail-index.netbsd.org/tech-userlevel/2018/02/07/msg011106.html

The UBSan ifdef could be changed to check only the kernel and skipping CPU.

[krytarowski]

• use _UC_MACHINE_FP() for all NetBSD architectures
• cover all NetBSD architectures with the same code in GetPcSpBp()
• remove check for CPU types in lib/ubsan/ubsan_platform.h

[vitalybuka]

lib/ubsan/ubsan_platform.h

16–17

Why did you remove CPU check?
How can we be sure that e.g. Linux on any CPU is going to work?

[krytarowski]

lib/ubsan/ubsan_platform.h

16–17

We already maintain it with CMake rules.

As I understand this was proposed by @ro to just maintain the list of OSes here. I prefer this over the existing situation.

If we want to control CPU types here, I would prefer to split this per OS and check CPU. LINUX || FREEBSD || APPLE || NETBSD working on the same set of CPUs is wrong.

lib/sanitizer_common/sanitizer_linux.cc

 #endif
 }
 
 void SignalContext::DumpAllRegisters(void *context) {
   // FIXME: Implement this.
 }
 
 static void GetPcSpBp(void *context, uptr *pc, uptr *sp, uptr *bp) {
-#if defined(__arm__)
+#if SANITIZER_NETBSD
+  // This covers all NetBSD architectures
+  ucontext_t *ucontext = (ucontext_t *)context;
+  *pc = _UC_MACHINE_PC(ucontext);
+  *bp = _UC_MACHINE_FP(ucontext);
+  *sp = _UC_MACHINE_SP(ucontext);
+#elif defined(__arm__)
   ucontext_t *ucontext = (ucontext_t*)context;
   *pc = ucontext->uc_mcontext.arm_pc;
   *bp = ucontext->uc_mcontext.arm_fp;
   *sp = ucontext->uc_mcontext.arm_sp;
 #elif defined(__aarch64__)
   ucontext_t *ucontext = (ucontext_t*)context;
   *pc = ucontext->uc_mcontext.pc;
   *bp = ucontext->uc_mcontext.regs[29];
   *sp = ucontext->uc_mcontext.sp;
 #elif defined(__hppa__)
   ucontext_t *ucontext = (ucontext_t*)context;
   *pc = ucontext->uc_mcontext.sc_iaoq[0];
   /* GCC uses %r3 whenever a frame pointer is needed.  */
   *bp = ucontext->uc_mcontext.sc_gr[3];
   *sp = ucontext->uc_mcontext.sc_gr[30];
 #elif defined(__x86_64__)
 # if SANITIZER_FREEBSD
   ucontext_t *ucontext = (ucontext_t*)context;
   *pc = ucontext->uc_mcontext.mc_rip;
   *bp = ucontext->uc_mcontext.mc_rbp;
   *sp = ucontext->uc_mcontext.mc_rsp;
-#elif SANITIZER_NETBSD
-  ucontext_t *ucontext = (ucontext_t *)context;
-  *pc = ucontext->uc_mcontext.__gregs[_REG_RIP];
-  *bp = ucontext->uc_mcontext.__gregs[_REG_RBP];
-  *sp = ucontext->uc_mcontext.__gregs[_REG_RSP];
 # else
   ucontext_t *ucontext = (ucontext_t*)context;
   *pc = ucontext->uc_mcontext.gregs[REG_RIP];
   *bp = ucontext->uc_mcontext.gregs[REG_RBP];
   *sp = ucontext->uc_mcontext.gregs[REG_RSP];
 # endif
 #elif defined(__i386__)
 # if SANITIZER_FREEBSD
   ucontext_t *ucontext = (ucontext_t*)context;
   *pc = ucontext->uc_mcontext.mc_eip;
   *bp = ucontext->uc_mcontext.mc_ebp;
   *sp = ucontext->uc_mcontext.mc_esp;
-#elif SANITIZER_NETBSD
-  ucontext_t *ucontext = (ucontext_t *)context;
-  *pc = ucontext->uc_mcontext.__gregs[_REG_EIP];
-  *bp = ucontext->uc_mcontext.__gregs[_REG_EBP];
-  *sp = ucontext->uc_mcontext.__gregs[_REG_ESP];
 # else
   ucontext_t *ucontext = (ucontext_t*)context;
 # if SANITIZER_SOLARIS
   /* Use the numeric values: the symbolic ones are undefined by llvm
      include/llvm/Support/Solaris.h.  */
 # ifndef REG_EIP
 #  define REG_EIP 14 // REG_PC
 # endif

lib/ubsan/ubsan_platform.h

 //===-- ubsan_platform.h ----------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // Defines the platforms which UBSan is supported at.
 //
 //===----------------------------------------------------------------------===//
 #ifndef UBSAN_PLATFORM_H
 #define UBSAN_PLATFORM_H
 
 // Other platforms should be easy to add, and probably work as-is.
-#if (defined(__linux__) || defined(__FreeBSD__) || defined(__APPLE__) ||       \
-     defined(__NetBSD__)) &&                                                   \
-    (defined(__x86_64__) || defined(__i386__) || defined(__arm__) ||           \
-     defined(__aarch64__) || defined(__mips__) || defined(__powerpc64__) ||    \
-     defined(__s390__)) || (defined(__sun__) && defined(__svr4__))
-# define CAN_SANITIZE_UB 1
-#elif defined(_WIN32) || defined(__Fuchsia__)
+#if defined(__linux__) || defined(__FreeBSD__) || defined(__APPLE__) ||        \
+    defined(__NetBSD__) || (defined(__sun__) && defined(__svr4__)) ||          \
+    defined(_WIN32) || defined(__Fuchsia__)
 # define CAN_SANITIZE_UB 1
 #else
 # define CAN_SANITIZE_UB 0
 #endif
 
 #endif