[PATCH] D35554: Add NetBSD support in sanitizer_platform_limits_posix.*

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_platform_limits_posix.h:236
+    u32 newp;
+    u32 newlen;
+#else
----------------
These seem to be roughly the same as the Linux/FreeBSD ones assuming the supported platforms use LP64 (not ILP64). Can't we just share the definition?

If you're planning on enabling the sanitizers on some non LP64 target on NetBSD, may I suggest delaying that architecture, and making sure we work properly on x86/x86-64/arm/arm64/some other platform already supported by Linux, and only then do we add `#ifdef`s for those "weirder" platforms.

Add something like this if you want a loud error:
```#if SANITIZER_NETBSD && !defined(_LP64)
#error Not supported yet!
#endif```

You might also be able to merge the `__sanitizer_iocb` definitions for NetBSD (but not with Linux) is you use fd_t, int, etc. But I'm not 100% sure on that one.


================
Comment at: lib/sanitizer_common/sanitizer_platform_limits_posix.h:240
+#endif
+#endif
   };
----------------
joerg wrote:
> Why is this duplicating random sets of system headers? Just for using syscall(2) directly? Ugly.
They can be used in syscall interceptors.


================
Comment at: lib/sanitizer_common/sanitizer_platform_limits_posix.h:261
+#elif defined(__i386__)
+    u8 data[20];
+#else
----------------
Maybe `uptr data[5]`? That way you won't need to have two defs.


================
Comment at: lib/sanitizer_common/sanitizer_platform_limits_posix.h:490
 
+#if SANITIZER_NETBSD || (defined(__x86_64__) && !defined(_LP64))
+  typedef long long __sanitizer_time_t;
----------------
I wonder if doing the same I suggested above (Doing `_LP64` platforms first) will be easier to get the patches reviewed+accepted, and then you'd need some smaller changes which will be easier to understand to make it support non LP64 platforms.


================
Comment at: lib/sanitizer_common/sanitizer_platform_limits_posix.h:505
+    u32 pw_gid;
+#endif
+#if SANITIZER_MAC || SANITIZER_FREEBSD || SANITIZER_NETBSD
----------------
Why are you changing non NetBSD code? It was already `int pw_uid`.


================
Comment at: lib/sanitizer_common/sanitizer_platform_limits_posix.h:733
+    u32 __bits[4];
+  };
 #endif
----------------
Seems like it could be shared with FreeBSD, assuming FreeBSD sanitizers only support LP64.
Is the NetBSD definition using `u32` on its system headers?


================
Comment at: lib/sanitizer_common/sanitizer_platform_limits_posix.h:793
 #endif
+
 #ifndef __mips__
----------------
Please don't do simple formatting changes in this patch.


================
Comment at: lib/sanitizer_common/sanitizer_platform_limits_posix.h:1016
+  };
+#define SANITIZER_HAS_STRUCT_FILE 0  // not ported
 #else
----------------
FreeBSD doesn't seem to need this. Does NetBSD need it?


Repository:
  rL LLVM

https://reviews.llvm.org/D35554