MaskRay marked 2 inline comments as done.
MaskRay added inline comments.
================
Comment at: lib/Driver/SanitizerArgs.cpp:27-30
static const SanitizerMask NeedsUbsanRt =
SanitizerKind::Undefined | SanitizerKind::Integer |
SanitizerKind::ImplicitConversion | SanitizerKind::Nullability |
+ SanitizerKind::CFI | SanitizerKind::FloatDivideByZero;
----------------
rsmith wrote:
> Duplicating this list of "all sanitizers covered by the ubsan runtime" in
> many places is leading to lots of bugs.
>
> This change misses `getPPTransparentSanitizers()` in
> `include/clang/Basic/Sanitizers.h`. Previous changes forgot to add
> `UnsignedIntegerOverflow` and `LocalBounds` to that function and also here
> and in `SupportsCoverage` and `RecoverableByDefault` below (but did update
> `TrappingSupported` and `getSupportedSanitizers`).
>
> A better approach would be to change `Sanitizers.def` to specify the relevant
> properties of the sanitizer (whether it's in the ubsan runtime, whether it's
> recoverable, whether it supports coverage, whether it supports trapping,
> whether it affects preprocessing) along with its definition.
> This change misses getPPTransparentSanitizers() in
> include/clang/Basic/Sanitizers.h
Thanks for pointing this out! It also missed ubsan_blacklist.txt (I think it
still makes sense to reuse the ubsan_blacklist.txt file, as "Integer" does that
too)
It is not in `static const SanitizerMask LegacyFsanitizeRecoverMask =
SanitizerKind::Undefined | SanitizerKind::Integer;` but since this mask is
legacy, I guess we do not have to touch it. AFAICT, all places have been fixed.
> A better approach would be to change Sanitizers.def to specify the relevant
> properties of the sanitizer
I'm strongly in favor of doing this, as I learned from the process how brittle
it is to let these masks scatter over all places.. But, can we (1) bring back
float-divide-by-zero first, and then (2) refactor tests and make them
target-independent, (3) fix Sanitizers.def?
================
Comment at: test/Driver/fsanitize.c:844
+
+// RUN: %clang -target x86_64-linux -fsanitize=float-divide-by-zero %s -###
2>&1 | FileCheck %s --check-prefixes=CHECK-DIVBYZERO,CHECK-DIVBYZERO-RECOVER
+// RUN: %clang -target x86_64-linux -fsanitize=float-divide-by-zero %s
-fno-sanitize-recover=float-divide-by-zero -### 2>&1 | FileCheck %s
--check-prefixes=CHECK-DIVBYZERO,CHECK-DIVBYZERO-NORECOVER
----------------
rsmith wrote:
> I think these tests should be target-independent. We should support this
> sanitizer (and indeed almost all of ubsan) regardless of the target.
> (Currently this is true on all targets except Myriad, where the latter is
> disallowed only due to a bug in r281071.)
Changed `-target x86_64-linux` to `-target %itanium_abi_triple`.
It seems Myriad also works - I have tried `-target sparcel-myriad`
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64317/new/
https://reviews.llvm.org/D64317
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
