This is an archive of the discontinued LLVM Phabricator instance.

Add ubsan/NetBSD/amd64 support
AbandonedPublic

Authored by krytarowski on Jul 7 2017, 1:20 AM.

Details

Summary

This includes common_sanitizer pieces.

$ cat ub.c
int main(int argc, char **argv) {

int k = 0x7fffffff;
k += argc;
return 0;

}
$ /usr/pkg/bin/clang -fsanitize=undefined ./ub.c
$ ./a.out
ub.c:3:5: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'

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

Sponsored by <The NetBSD Foundation>

Diff Detail

Repository
rL LLVM

Event Timeline

krytarowski created this revision.Jul 7 2017, 1:20 AM
filcab added a subscriber: filcab.Jul 7 2017, 4:34 AM

Can you split interception and sanitizer_common from ubsan patches?
I'd also rather have the sanitizer_common stuff + ninja check-sanitizer working before enabling UBSan, since it'd be easy to split those changes.
I'd also split the interception patches from sanitizer_common unless there's a problem running the tests with only one of those.
When you enable UBSan you might also be able to enable CXX_ABI on NetBSD, assuming it uses the usual Itanium C++abi.

lib/sanitizer_common/sanitizer_linux.cc
589

Seems fragile. :-(
One thing that might happen (I don't know the details of your scheduler) is that a thread hitting this case might spin forever and take most of the CPU time, since it might end up with its priority (might be a differently named concept on your scheduler), due to always relinquishing the CPU very quickly (I've seen this kind of thing happen). This might end up being a problem when threads with the same CPU affinity are waiting for each other.

685

I wonder if we should just make the count argument a uptr, and use the same as Linux.

lib/sanitizer_common/sanitizer_platform_limits_posix.h
1113

What?

1462

Stray comment

lib/sanitizer_common/sanitizer_syscall_generic.inc
25

Nit: Newline above this

33

Why do we care about byte order, here? Everything should always be in host order and we have no bswaps.
Why can't you always use the 64 bit version on 64 bit platforms? Are the return values expected to be changed inside the ASan runtime?

krytarowski planned changes to this revision.Jul 7 2017, 5:01 AM

I will hold on with this large patch and prepare a smaller chunk with only sanitizer_common stuff + functional ninja check-sanitizer.

lib/sanitizer_common/sanitizer_platform_limits_posix.h
1113

I overlooked it.

lib/sanitizer_common/sanitizer_syscall_generic.inc
33

Byte order is a bad wording, argument+return value length.

I will check if we can always use __syscall(2).

krytarowski added inline comments.Jul 7 2017, 5:09 AM
lib/sanitizer_common/sanitizer_syscall_generic.inc
33

According to my understanding we need these three types of syscall functions on NetBSD.

Since architectures return 32 bit and 64 bit results in different
registers, it may be impossible to portably convert the result of
__syscall() to a 32bit value.  For instance sparc returns 32 bit values
in %o0 and 64 bit values in %o0:%o1 (with %o0 containing the most
significant part) so a 32 bit right shift of the result is needed to get
a correct 32 bit result.
kcc edited edge metadata.Jul 10 2017, 12:18 PM

Please reduce the amount of ifdefs, probably by moving some of the code to new separate files (e.g. *_netbsd.cc)

In D35111#804082, @kcc wrote:

Please reduce the amount of ifdefs, probably by moving some of the code to new separate files (e.g. *_netbsd.cc)

Please point what files in particular should be moved to *_netbsd.cc.

krytarowski abandoned this revision.Jul 16 2017, 5:06 PM

This will be resend with smaller chunks.