This is an archive of the discontinued LLVM Phabricator instance.

More OpenBSD fixes
ClosedPublic

Authored by devnexen on Mar 20 2018, 3:46 PM.

Details

Summary
  • Use internal_syscall_ptr in internal_readlink
  • use sigcontext on OpenBSD

Patch by David CARLIER

Diff Detail

Event Timeline

devnexen created this revision.Mar 20 2018, 3:46 PM
Herald added subscribers: Restricted Project, llvm-commits, kubamracek. · View Herald TranscriptMar 20 2018, 3:46 PM
devnexen edited the summary of this revision. (Show Details)Mar 20 2018, 3:49 PM
vitalybuka added inline comments.Mar 21 2018, 12:53 AM
lib/sanitizer_common/sanitizer_linux.cc
72

this should not be needed after r328077

339

What about this?

1757

Why zeroes? Can you get real values?

devnexen added inline comments.Mar 21 2018, 1:48 AM
lib/sanitizer_common/sanitizer_linux.cc
72

I commented this commit here

339

I thought I did this one ...

1757

ucontext not supported here.

devnexen added inline comments.Mar 21 2018, 1:52 AM
lib/sanitizer_common/sanitizer_linux.cc
72

Nice works better under FreeBSD should do the job under OpenBSD.

devnexen updated this revision to Diff 139262.Mar 21 2018, 1:57 AM
vitalybuka added inline comments.Mar 21 2018, 11:02 AM
lib/sanitizer_common/sanitizer_linux.cc
72

Sorry, that was a typo.

312–313

newfstatat should return int, so no _ptr

318

same for stat

339

Sorry, but I only asked, because I am not sure why you are making this change.
According sanitizer_common/sanitizer_syscall_generic.inc

internal_syscall_ptr for pointer and (s)size_t

Looking on documentation all of them return int and internal_syscall supposed to be used. What am I missing?

408

same for unlinkat, it returns int, on both

1757

https://man.openbsd.org/sigaction.2

On OpenBSD, ucontext_t is an alias for the sigcontext structure defined in <signal.h>.

devnexen added inline comments.Mar 21 2018, 11:39 AM
lib/sanitizer_common/sanitizer_linux.cc
408

Seems under Linux it s the same in fact https://github.com/llvm-mirror/compiler-rt/blob/b4e600115ad4c50192095c225aaed3ae9ca99ec1/lib/sanitizer_common/sanitizer_syscall_generic.inc#L53 but I personally do not mind reverting them I think it was kamel who asked me to update in the first place.

vitalybuka added inline comments.Mar 21 2018, 12:36 PM
lib/sanitizer_common/sanitizer_linux.cc
408

it's the same, but if we introduced internal_syscall_ptr and we specified that it's for pointers and size, we should follow that convention.

devnexen added inline comments.Mar 21 2018, 12:42 PM
lib/sanitizer_common/sanitizer_linux.cc
408

Good enough for me.

devnexen updated this revision to Diff 139354.Mar 21 2018, 12:47 PM
vitalybuka requested changes to this revision.Mar 21 2018, 1:23 PM
vitalybuka added inline comments.
lib/sanitizer_common/sanitizer_linux.cc
326

I think it should be internal_syscall() as it's int everywhere, why do you need ptr?
And this is not OpenBSD branch at all

395–400

On linux readlinkat is int, on OpenBSD it's size. So it should be two different branches

416–417
1757

Could you please comment about your plans here?

This revision now requires changes to proceed.Mar 21 2018, 1:23 PM
devnexen added inline comments.Mar 21 2018, 1:38 PM
lib/sanitizer_common/sanitizer_linux.cc
1757

All good points ... Did not know about sigcontext

devnexen updated this revision to Diff 139366.Mar 21 2018, 1:54 PM

Adding sigcontext support

vitalybuka added inline comments.Mar 22 2018, 10:47 AM
lib/sanitizer_common/sanitizer_linux.cc
416–417

it's int on OpenBSD as well
https://github.com/openbsd/src/blob/master/sys/sys/syscall.h

What do you think about this one?

devnexen added inline comments.Mar 22 2018, 10:53 AM
lib/sanitizer_common/sanitizer_linux.cc
416–417

Sorry I missed this one good point.

devnexen updated this revision to Diff 139477.Mar 22 2018, 11:00 AM
vitalybuka accepted this revision.Mar 22 2018, 11:20 AM
This revision is now accepted and ready to land.Mar 22 2018, 11:20 AM
vitalybuka retitled this revision from Unsubmitted part of D44599 to More OpenBSD fixes.Mar 22 2018, 11:21 AM
vitalybuka edited the summary of this revision. (Show Details)
vitalybuka edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.