This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Fix build on FreeBSD RISC-V
ClosedPublic

Authored by arichardson on Jun 8 2021, 4:09 AM.

Details

Summary

We have to avoid calling renameat2 and clone on FreeBSD.
Additionally, the mcontext structure has different members.

Diff Detail

Event Timeline

arichardson created this revision.Jun 8 2021, 4:09 AM
arichardson requested review of this revision.Jun 8 2021, 4:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2021, 4:09 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
jrtc27 added inline comments.Jun 8 2021, 4:19 AM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
1319

x86_64, i386 and arm currently have this, but aarch64, mips and powerpc64 do not. Should we not just hoist an #if SANITIZER_LINUX out around the whole thing so we don't need N copies of the same thing?

1883

This whole code is overly complex on FreeBSD. On Linux's it's needed because (for whatever reason, probably because it copied MIPS) the exception info isn't available. On FreeBSD you can just inspect gp_sstatus and see if it's a load or store/amo page/access fault, like is done for Arm and AArch64.

jrtc27 added inline comments.Jun 8 2021, 4:43 AM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
1883

... never mind, that's sstatus, not scause. How sad, and what a weird field to expose, there's nothing you can usefully do with it from userspace. Maybe we should break ABI and replace it with gp_scause... nobody should be using it at the moment.

arichardson added inline comments.Jun 8 2021, 5:10 AM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
1319

I considered adding it around the whole block, but then decided to keep the diff as small as possible instead. Happy to change that though

lenary resigned from this revision.Jun 8 2021, 8:00 AM
luismarques accepted this revision.Jun 9 2021, 2:07 PM

LGTM.

compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
1319

@jrtc27's suggestion makes sense. Please commit an updated patch.

1883

... never mind, that's sstatus, not scause. How sad, and what a weird field to expose, there's nothing you can usefully do with it from userspace. Maybe we should break ABI and replace it with gp_scause... nobody should be using it at the moment.

That would be nice. The current approach is always going to be brittle. Plus, IIRC it's not even being directly unit-tested. Was your suggestion to change the ABI only for FreeBSD, though?

This revision is now accepted and ready to land.Jun 9 2021, 2:07 PM
jrtc27 added inline comments.Jun 9 2021, 2:15 PM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
1883

It should be doable on Linux too in the same way that it's done for AArch64 I would imagine, i.e. tacking an extra list of variable length extension structs on the end. They probably won't be so willing to break ABI :) (in FreeBSD breaking ABI is ok if you have good justification and it's not a Tier 1 architecture)

Use a top-level ifdef

jrtc27 accepted this revision.Aug 26 2021, 2:51 AM
This revision was landed with ongoing or failed builds.Aug 26 2021, 4:07 AM
This revision was automatically updated to reflect the committed changes.