This is an archive of the discontinued LLVM Phabricator instance.

Add NetBSD support in sanitizer_platform_limits_posix.*
AbandonedPublic

Authored by krytarowski on Jul 18 2017, 5:37 AM.

Details

Summary

NetBSD is a BSD Operating System, close to FreeBSD, however with distinct interfaces that
were diverged over 20 years ago.

Part of the code inspired by the original work on libsanitizer in GCC 5.4 by Christos Zoulas.

Sponsored by <The NetBSD Foundation>

Diff Detail

Repository
rL LLVM

Event Timeline

krytarowski created this revision.Jul 18 2017, 5:37 AM
joerg added inline comments.Jul 18 2017, 6:06 AM
lib/sanitizer_common/sanitizer_platform_limits_posix.h
33

dlinfo with RTLD_DI_LINKMAP?

232

Why is this duplicating random sets of system headers? Just for using syscall(2) directly? Ugly.

filcab added inline comments.Jul 18 2017, 5:31 PM
lib/sanitizer_common/sanitizer_platform_limits_posix.h
228

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 #ifdefs 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.

232

They can be used in syscall interceptors.

235

Maybe uptr data[5]? That way you won't need to have two defs.

458

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.

472

Why are you changing non NetBSD code? It was already int pw_uid.

695

Seems like it could be shared with FreeBSD, assuming FreeBSD sanitizers only support LP64.
Is the NetBSD definition using u32 on its system headers?

751

Please don't do simple formatting changes in this patch.

973

FreeBSD doesn't seem to need this. Does NetBSD need it?

krytarowski added inline comments.Jul 19 2017, 8:48 AM
lib/sanitizer_common/sanitizer_platform_limits_posix.h
33

I've followed: reviews.llvm.org/D7233

228

I will try to merge it. Right now we support only regular ABIs.

235

OK.

458

NetBSD always has long long type of time_t regardless of ABI.

472

I will recheck it.

695
typedef struct {
        __uint32_t      __bits[4];
} sigset_t;
  • sys/sigtypes.h

Right now I can merge it with FreeBSD.

751

OK

973

FreeBSD has ported it.

ubsan/asan can work without it as of now.

Apply changes from review.

filcab added inline comments.Jul 19 2017, 9:34 PM
lib/sanitizer_common/sanitizer_platform_limits_posix.cc
296

Setting these to -1 looks weird. Can you at least add a small comment? I'm assuming these simply don't exist on NetBSD.

lib/sanitizer_common/sanitizer_platform_limits_posix.h
124

Stray change.

185

Maybe use uptr (Or char*) for aio_buf and long for aio_nbytes and others below?
It seems you can merge these two platforms.

194

There's more than one of these. Right now you're just dealing with x86 and x86_64, so why not just have one #error at the start of the file if you need to, and then not have these cases?

973

If this is set to 0, can't you just use the #else case, which defined __sanitizer_FILE to void?

krytarowski added inline comments.Jul 20 2017, 6:44 AM
lib/sanitizer_common/sanitizer_platform_limits_posix.cc
296

These interfaces are absent on NetBSD.

lib/sanitizer_common/sanitizer_platform_limits_posix.h
124

I will fix it.

185

I will go for it.

194

It will be gone with the above change.

973

I don't recall right now if I needed to have __sanitizer_FILE != void to make it buildable/functional... but for now I will go for it now.

It will be ported in future.

Apply changes from review.

glider added a subscriber: glider.Aug 7 2017, 5:45 AM

A couple of drive-by comments.

lib/sanitizer_common/sanitizer_platform_limits_posix.h
30

Can you please wrap this line into a macro that takes handle and offset and returns the link_map* ?

54

I suggest we always keep SANITIZER_FREEBSD and SANITIZER_NETBSD together in the ifdefs:
!SANITIZER_FREEBSD && !SANITIZER_NETBSD && !SANITIZER_IOS

By the way, do we need a special SANITIZER_BSD for the common BSD-specific cases?
I guess in many cases SANITIZER_FREEBSD || SANITIZER_NETBSD also means "or other BSD system"

79

Please change this to "#if !SANITIZER_ANDROID && !SANITIZER_NETBSD"

609–610

Please fix the comment.

724–733

Please fix the comment.

krytarowski added inline comments.Aug 7 2017, 10:06 AM
lib/sanitizer_common/sanitizer_platform_limits_posix.h
54

I think that we should keep alphabetical order, both systems diverged over 20 years ago and besides common ancestors decades ago, they are different. Linux adopted BSD interfaces too (like ptrace(2)).

SANITIZER_BSD has no point in my opinion. Pretending that something is BSD-flavor and defining it as a standard is a losing battle, we all are struggling to be compatible with POSIX. Old interfaces over they years were refactored, deprecated, replaced, abandoned, reinvented etc. We will end up with SANITIZER_BSD && !SANITIZER_OPENBSD and similar.

Apply changes from review.

krytarowski marked 23 inline comments as done.Aug 8 2017, 6:56 PM
krytarowski marked 9 inline comments as done.
krytarowski marked 2 inline comments as done.

This is the last patch needed for common-sanitizer on NetBSD.

I've closed existing comments to help to keep track on changes.

ASAN, UBSAN and SAFESTACK are already committed.

More to come soon. This diff blocks them all from building.

I plan to go on vacations during September, if there are no more comments I will commit it as it is in the coming week.

kcc edited edge metadata.Aug 26 2017, 10:56 AM

This patch has too many #ifdefs.
#ifdefs cause maintenance nightmare.
Instead of #ifdefs, can you move the code to separate *netbsd*.[h,cpp] files?

krytarowski abandoned this revision.Aug 27 2017, 5:39 AM
In D35554#853339, @kcc wrote:

This patch has too many #ifdefs.
#ifdefs cause maintenance nightmare.
Instead of #ifdefs, can you move the code to separate *netbsd*.[h,cpp] files?

I've submitted it as a new diff: D37193.