[PATCH] D44036: OpenBSD UBsan support / common part 3

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added a comment.

Title of the patch is not useful, but it hard to make it as this patch is still too large to my taste.
E.g. There is no reason to have ReadProcMaps and GetTls changes in a single patch.

Ideally:
One patch for trivial "SANITIZER_OPENBSD ||"
For the rest. One patch per fixed function.



================
Comment at: lib/sanitizer_common/sanitizer_internal_defs.h:184
 #if SANITIZER_MAC
-// On Darwin, thread IDs are 64-bit even on 32-bit systems.
+// On Darwin, thread IDs are 64-bit even on 32 bits systems.
 typedef u64 tid_t;
----------------
why this change?


================
Comment at: lib/sanitizer_common/sanitizer_libignore.cc:17
 #include "sanitizer_flags.h"
+
 #include "sanitizer_posix.h"
----------------
remove empty line


================
Comment at: lib/sanitizer_common/sanitizer_linux.cc:91
 }
+#include <sys/thr.h>
 extern char **environ;  // provided by crt1
----------------
why do you need sys/thr.h here?


================
Comment at: lib/sanitizer_common/sanitizer_linux.cc:104
+#if SANITIZER_OPENBSD
+#include <limits.h>
+#include <sys/sysctl.h>
----------------
I suspect you can move following into common includes?
#include <sys/sysctl.h>
#include <limits.h>


================
Comment at: lib/sanitizer_common/sanitizer_linux.cc:106
+#include <sys/sysctl.h>
+extern char **environ;
+#endif
----------------
Why do you need this extern?
Is any header for this?


================
Comment at: lib/sanitizer_common/sanitizer_linux.cc:178
                               (long)0, offset);
-#elif SANITIZER_FREEBSD || SANITIZER_LINUX_USES_64BIT_SYSCALLS
+#elif SANITIZER_FREEBSD || \
+    SANITIZER_LINUX_USES_64BIT_SYSCALLS
----------------
make elif one line


================
Comment at: lib/sanitizer_common/sanitizer_linux.cc:319
 #elif SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
-  return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path,
+  return internal_syscall_ptr(SYSCALL(newfstatat), AT_FDCWD, (uptr)path,
                           (uptr)buf, 0);
----------------
why _ptr was added here and to other non OPENBSD parts?


================
Comment at: lib/sanitizer_common/sanitizer_linux.cc:601
+  if (sysctl(envmib, 4, NULL, &nenv, NULL, 0) == -1) {
+    Printf("sysctl KERN_PROC_NENV failed\n");
+    Die();
----------------
Can you add own GetArgv or GetArgsAndEnv to sanitizer_openbsd.cc ?


================
Comment at: lib/sanitizer_common/sanitizer_linux.cc:1077
 
 uptr ReadBinaryName(/*out*/char *buf, uptr buf_len) {
 #if SANITIZER_SOLARIS
----------------
ReadBinaryName in sanitizer_openbsd.cc ?


================
Comment at: lib/sanitizer_common/sanitizer_platform_interceptors.h:231
+#define SANITIZER_INTERCEPT_ACCEPT4 SI_LINUX_NOT_ANDROID || \
+  SI_NETBSD || SI_OPENBSD
 #define SANITIZER_INTERCEPT_PACCEPT SI_NETBSD
----------------
```
#define SANITIZER_INTERCEPT_ACCEPT4 (SI_LINUX_NOT_ANDROID || \
  SI_NETBSD || SI_OPENBSD)
```


================
Comment at: lib/sanitizer_common/sanitizer_platform_interceptors.h:310
 #define SANITIZER_INTERCEPT_ETHER_R (SI_FREEBSD || SI_LINUX_NOT_ANDROID)
 #define SANITIZER_INTERCEPT_SHMCTL                       \
+  (SI_NETBSD || SI_OPENBSD || SI_SOLARIS || \
----------------
clang-format?


================
Comment at: lib/sanitizer_common/sanitizer_platform_interceptors.h:447
+  !SI_NETBSD && !SI_OPENBSD && SI_NOT_FUCHSIA)
+#define SANITIZER_INTERCEPT_MEMALIGN (!SI_FREEBSD && !SI_MAC && \
+  !SI_NETBSD && !SI_OPENBSD)
----------------
clang-format it please.


================
Comment at: lib/sanitizer_common/sanitizer_platform_interceptors.h:460
 
-#define SANITIZER_INTERCEPT_ACCT SI_NETBSD
+#define SANITIZER_INTERCEPT_ACCT SI_NETBSD || SI_OPENBSD
 #define SANITIZER_INTERCEPT_USER_FROM_UID SI_NETBSD
----------------
(SI_NETBSD || SI_OPENBSD)


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D44036