This is an archive of the discontinued LLVM Phabricator instance.

OpenBSD UBsan support / common
ClosedPublic

Authored by devnexen on Feb 28 2018, 2:12 PM.

Details

Summary

Sanitizer common, enable OpenBSD platform.

  • Enable common interceptors as possible and create few distinct ones.
  • Create necessary sanitizer_struct types.

Diff Detail

Repository
rL LLVM

Event Timeline

devnexen created this revision.Feb 28 2018, 2:12 PM
krytarowski added inline comments.Feb 28 2018, 10:40 PM
cmake/base-config-ix.cmake
142 ↗(On Diff #136395)

Rationale?

lib/sanitizer_common/sanitizer_common_interceptors.inc
4410 ↗(On Diff #136395)

Same here as bellow regarding INIT_PTHREAD_ATTR_GET

4442 ↗(On Diff #136395)

Split this INIT_PTHREAD_ATTR_GET into two versions, one with shared interceptors, the other with interceptors for !OPENBSD.

lib/sanitizer_common/sanitizer_interface_internal.h
72 ↗(On Diff #136395)

Rationale?

lib/sanitizer_common/sanitizer_internal_defs.h
104 ↗(On Diff #136395)

Extract FREEBSD and OPENBSD from this #if and follow SOLARIS adding a dedicated entry with a comment.

lib/sanitizer_common/sanitizer_linux.cc
316 ↗(On Diff #136395)

internal_syscall_ptr

401 ↗(On Diff #136395)

internal_syscall_ptr

410 ↗(On Diff #136395)

internal_syscall_ptr

418 ↗(On Diff #136395)

internal_syscall_ptr

467 ↗(On Diff #136395)

This is wrong for FreeBSD and OpenBSD.

thr_self() for FreeBSD
syscall(SYS_getthrid) for OpenBSD

486 ↗(On Diff #136395)

Please remove all unrelated changes for this file.

1732 ↗(On Diff #136395)

Use #else at the end.

1780 ↗(On Diff #136395)

#elif SANITIZER_NETBSD

lib/sanitizer_common/sanitizer_platform_interceptors.h
460 ↗(On Diff #136395)

Please use braces in this file, like: (SI_NETBSD || SI_OPENBSD).

lib/sanitizer_common/sanitizer_procmaps.h
99 ↗(On Diff #136395)

missing OPENBSD

lib/sanitizer_common/sanitizer_stacktrace.h
26 ↗(On Diff #136395)

I would merge it with SANITIZER_WINDOWS.

devnexen added inline comments.Feb 28 2018, 10:51 PM
cmake/base-config-ix.cmake
142 ↗(On Diff #136395)

No multi arch support.

Assuming that OpenBSD is capable to run only a subset of functionality of UBSan (no unwinder, no 32-bit on 64-bit kernel etc), do we need all the code like sanitizer_platform_limits_openbsd?

devnexen added a comment.EditedFeb 28 2018, 11:44 PM

Assuming that OpenBSD is capable to run only a subset of functionality of UBSan (no unwinder, no 32-bit on 64-bit kernel etc), do we need all the code like sanitizer_platform_limits_openbsd?

lib/sanitizer_common/sanitizer_interface_internal.h
72 ↗(On Diff #136395)

Probably old changes forgot to remove same as interception I believe.

devnexen updated this revision to Diff 136494.Mar 1 2018, 3:56 AM

simplify openbsd parts.

krytarowski added inline comments.Mar 1 2018, 4:39 AM
lib/sanitizer_common/sanitizer_linux.cc
428 ↗(On Diff #136494)

Linux should switch to internal_syscall_ptr as well in all these mentioned branches.

476 ↗(On Diff #136494)

This is still wrong. thr_self returns 0 on success, -1 on error.

Cast to (uptr) in both cases is wrong.

Also tid_t shall be corrected and marked u64 for everybody.

devnexen added inline comments.Mar 1 2018, 4:45 AM
lib/sanitizer_common/sanitizer_linux.cc
476 ↗(On Diff #136494)

On Solaris ? Because on FreeBSD it s thread_t type.

krytarowski added inline comments.Mar 1 2018, 4:47 AM
lib/sanitizer_common/sanitizer_linux.cc
476 ↗(On Diff #136494)

FreeBSD 11.1 notes:

SYNOPSIS
     #include <sys/thr.h>

     int
     thr_self(long *id);
devnexen added inline comments.Mar 1 2018, 4:54 AM
lib/sanitizer_common/sanitizer_linux.cc
476 ↗(On Diff #136494)

You got a point I looked at the wrong man page :-)
https://www.freebsd.org/cgi/man.cgi?query=thr_self&manpath=SunOS+5.10

devnexen added inline comments.Mar 1 2018, 5:00 AM
lib/sanitizer_common/sanitizer_linux.cc
476 ↗(On Diff #136494)

I m less keen for the last bit though since not all oses are into 64 bits types I d say keep upturn for everybody until the fact change.

krytarowski added inline comments.Mar 1 2018, 5:02 AM
lib/sanitizer_common/sanitizer_linux.cc
476 ↗(On Diff #136494)

uptr is wrong for everybody
pthread_t is a pointer on FreeBSD and it was used wrongly as pid_t

devnexen added inline comments.Mar 1 2018, 5:07 AM
lib/sanitizer_common/sanitizer_linux.cc
428 ↗(On Diff #136494)

A bit unrelated change. Hope that won t break anything can t test under Linux at the moment.

krytarowski added inline comments.Mar 1 2018, 5:08 AM
lib/sanitizer_common/sanitizer_linux.cc
428 ↗(On Diff #136494)

internal_syscall_ptr=internal_syscall on Linux

devnexen updated this revision to Diff 136499.Mar 1 2018, 5:18 AM

Changing GetTid signature
Converting few internal_syscall to internal_syscall_ptr equivalent

krytarowski added inline comments.Mar 1 2018, 5:27 AM
lib/sanitizer_common/sanitizer_linux.cc
476 ↗(On Diff #136494)

I had in mind to change typedef tid_t to u64... instead of uptr.

devnexen updated this revision to Diff 136502.Mar 1 2018, 5:35 AM
kettenis added inline comments.Mar 1 2018, 5:48 AM
cmake/base-config-ix.cmake
142 ↗(On Diff #136395)

Correct; OpenBSD (deliberately) does not support running 32-bit code on a 64-bit kernel.

devnexen retitled this revision from [3/3] OpenBSD UBsan support / common to OpenBSD UBsan support / common.Mar 1 2018, 11:56 AM
devnexen edited the summary of this revision. (Show Details)

Please try to partition this into 200-300 line patches.

This patch is 2271 lines long.

Please try to remove all unrelated style changes.

GetTid() fix for FreeBSD (and abandoning uptr concept) deserves a separate commit.

devnexen updated this revision to Diff 136756.Mar 2 2018, 8:15 AM
vitalybuka requested changes to this revision.Mar 2 2018, 10:57 AM

Please send for review as a set of small patches.

This revision now requires changes to proceed.Mar 2 2018, 10:57 AM
devnexen updated this revision to Diff 136812.Mar 2 2018, 11:15 AM

Reducing the first diff to the pure openbsd part

vitalybuka accepted this revision.Mar 2 2018, 1:28 PM

LGTM, but maybe you want to wait for others input.

"git clang-format -f --style=file HEAD^" may fix couple minor issues

This revision is now accepted and ready to land.Mar 2 2018, 1:28 PM
devnexen updated this revision to Diff 136843.Mar 2 2018, 1:45 PM
devnexen updated this revision to Diff 136854.Mar 2 2018, 2:30 PM
devnexen updated this revision to Diff 136855.
devnexen updated this revision to Diff 136869.Mar 2 2018, 3:39 PM
krytarowski added inline comments.Mar 3 2018, 4:05 AM
lib/sanitizer_common/sanitizer_openbsd.cc
45 ↗(On Diff #136869)

I think you need to use syscall(), however this is restricted for UBSan-only so we might bear with it.

devnexen added inline comments.Mar 3 2018, 4:07 AM
lib/sanitizer_common/sanitizer_openbsd.cc
45 ↗(On Diff #136869)

Originally I used syscall but it fails (e.g. cannot map).

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.