This is an archive of the discontinued LLVM Phabricator instance.

[AddressSanitizer] Add AddressSanitizer support for VxWorks OS
Needs RevisionPublic

Authored by wenbin0301 on Jan 20 2022, 8:11 AM.

Details

Reviewers
phosek
kcc
samsonov
vitalybuka
Group Reviewers
Restricted Project
Summary

This changeset adds AddressSanitizer support for VxWorks on X86-64/AARCH64/RISCV64. All VxWorks specific code changes are controlled by the macro SANITIZER_VXWORKS and so will not impact other OS and common implementation. There are some differences between VxWorks and other Operating Systems. The most significant one is the VxWorks doesn't support on-demanding page. That means we can't mmap too big memory at one time because of physical memory limitation and hence it also requires the change in VxWorks kernel to support the shadow memory and allocator memory management. All the changes have been proved to work and will be published in VxWorks next release 22.03.

Diff Detail

Event Timeline

wenbin0301 created this revision.Jan 20 2022, 8:11 AM
wenbin0301 requested review of this revision.Jan 20 2022, 8:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2022, 8:11 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Apply clang-format to some new changes

Are we going to have a buildbot?
Who is going to maintain this code?
I past we had to spent some time reviewing, maintaining on guess, and then finally revering code for abandoned platforms.

If usage is limited, then keeping it downstream fork is a reasonable approach.

MaskRay added a reviewer: Restricted Project.Jan 20 2022, 7:06 PM

Same thought. RTEMS and Myriad were previously removed ports. OpenBSD shifted to their downstream port.

Thank you for raising the concern.

The AddressSantizer feature will be officially published to all Windriver Customers worldwild in next VxWorks release and the usage is widespread. The LLVM tool is heavily used in Windriver and has been updated it regularly. We will benifit a lot
if the changeset can be upstreamed.

An internal test platform (something like buildbot) is being setup to reguarly test the AddressSanitizer based on the latest main branch code. In case any something broken, we will be able to quickly fix it. That can prevent the AddressSanitizer on VxWorks from being broken by
new changes. And also, since all VxWorks specific changes are guarded by macro SANITIZER_VXORKS, it will not break others.

I will maintain the AddressSanitizer for VxWorks on behalf of WindRiver.

Hello all reviewers,

May I know your opinion on this review?

Thanks,
Wenbin

MaskRay added a comment.EditedMar 15 2022, 9:53 PM

This is a major target to support. It will add considerable maintenance costs to sanitizer developers.
There are 80 occurrences of SANITIZER_VXWORKS. Consider that when a maintainer refactors some interfaces, if there is a vxworks special case, they may need to stop and think about it, and probably remember to inform the vxworks maintainer(s).

Historically such a major addition needs an RFC on the llvm-dev mailing list.
Now that it has been migrated to discourse (https://discourse.llvm.org/t/post-discourse-migration-information/59719), I'd suggest that you start a post on https://discourse.llvm.org/c/runtimes/sanitizers/12 and tag eugenis, kcc, vitalybuka in the post.

The main consideration for accepting a target is (in asb's words) "benefits to end users, the cost of having the functionality upstream, and if there are people willing to support it"
These bullet points https://clang.llvm.org/get_involved.html#criteria may be useful as a start point.

Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2022, 9:53 PM
Herald added a subscriber: arichardson. · View Herald Transcript
MaskRay added inline comments.Mar 15 2022, 10:01 PM
compiler-rt/lib/interception/interception.h
277–279

Just stick with the local formatting that uses one-column for indentation. Ignore clang-format lints. This applies to other places in this patch.

compiler-rt/lib/interception/interception_linux.h
22–23

Ignore clang-format lints. Use the local convention.

MaskRay added inline comments.Mar 15 2022, 10:07 PM
compiler-rt/lib/sanitizer_common/sanitizer_common.h
953

I believe this is not needed after D118783

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
54–56

use local convention. don't add indentation

compiler-rt/lib/sanitizer_common/sanitizer_posix.cpp
30

Is the comment stale?

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp
54

This seems surprising to me. Why isn't pthread_self available?

126

Prefer #if XXX #else #endif to #if !XXX xxx #else #endif if XXX has 0/1 value

Why is SANITIZER_VXWORKS special?

wenbin0301 added inline comments.Mar 16 2022, 8:16 PM
compiler-rt/lib/interception/interception.h
277–279

The pre-merge check always failed if not follow the clang-format result. Does that matter?

compiler-rt/lib/sanitizer_common/sanitizer_common.h
953

That is right. Thank you for your info.

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp
54

It is actually available on VxWorks. We are using a vxWorks native method to implement GetThreadSelf. The reason is we want to avoid linking PTHREAD library only due to the reference to pthread_self on vxWorks if application code doesn't use other pthread_* functions.

I can delete this change if it is not appropriate from upstream point of view.

126

VxWorks doesn't support RLIMIT_AS and RLIMIT_CORE in kernel. I can delete these changes and keep them in private patch if it is not appropriate for upstreaming.

wenbin0301 added inline comments.Mar 16 2022, 8:27 PM
compiler-rt/lib/sanitizer_common/sanitizer_posix.cpp
30

MAP_NORESERVE is not supported on VxWorks either. Yeah, this information could be added in the comments.

vitalybuka requested changes to this revision.Dec 7 2022, 1:27 PM
This revision now requires changes to proceed.Dec 7 2022, 1:27 PM