[PATCH] D64317: [Driver] Add float-divide-by-zero back to supported sanitizers after D63793/rC365272

[Fangrui Song via Phabricator via llvm-commits]

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