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

[Kamil Rytarowski via Phabricator via llvm-commits]

krytarowski added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_platform_limits_posix.h:32
+#define GET_LINK_MAP_BY_DLOPEN_HANDLE(handle) \
+  ((link_map *)((handle) == nullptr ? nullptr : ((char *)(handle) + 324)))
+#else
----------------
joerg wrote:
> dlinfo with RTLD_DI_LINKMAP?
I've followed: reviews.llvm.org/D7233


================
Comment at: lib/sanitizer_common/sanitizer_platform_limits_posix.h:236
+    u32 newp;
+    u32 newlen;
+#else
----------------
filcab wrote:
> 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.
I will try to merge it. Right now we support only regular ABIs.


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


================
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;
----------------
filcab wrote:
> 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.
NetBSD always has long long type of time_t regardless of ABI.


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


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

-- `sys/sigtypes.h`

Right now I can merge it with FreeBSD.


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


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

ubsan/asan can work without it as of now.


Repository:
  rL LLVM

https://reviews.llvm.org/D35554