This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by MaskRay on Jul 8 2019, 3:01 AM.

Details

Summary

D63793 removed float-divide-by-zero from the "undefined" set but it
failed to add it to getSupportedSanitizers(), thus the sanitizer is
rejected by the driver:

clang-9: error: unsupported option '-fsanitize=float-divide-by-zero' for target 'x86_64-unknown-linux-gnu'

Also, add SanitizerMask::FloatDivideByZero to a few other masks to make -fsanitize-trap, -fsanitize-recover, -fsanitize-minimal-runtime and -fsanitize-coverage work.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Jul 8 2019, 3:01 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 8 2019, 3:01 AM
Herald added subscribers: llvm-commits, Restricted Project, delcypher, kubamracek. · View Herald Transcript
MaskRay abandoned this revision.Jul 8 2019, 4:15 AM

This is incorrect. Should fix -fsanitize=float-divide-by-zero instead.

MaskRay updated this revision to Diff 208378.Jul 8 2019, 5:54 AM
MaskRay retitled this revision from [ubsan] Delete the FloatDivideByZero ErrorType to [Driver] Add float-divide-by-zero back to supported sanitizers after D63793/rC365272.
MaskRay edited the summary of this revision. (Show Details)

Fix -fsanitize=float-divide-by-zero

Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2019, 5:54 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MaskRay updated this revision to Diff 208382.Jul 8 2019, 6:03 AM

Remove a stray :

MaskRay updated this revision to Diff 208395.Jul 8 2019, 7:06 AM
MaskRay edited the summary of this revision. (Show Details)

make various sanitizer features work

MaskRay updated this revision to Diff 208410.Jul 8 2019, 7:36 AM

Fix sanitizer-ld.c test

is the patch description still correct?

is the patch description still correct?

Yes.

Also, add SanitizerMask::FloatDivideByZero to a few other masks to make -fsanitize-trap, -fsanitize-recover, -fsanitize-minimal-runtime and -fsanitize-coverage work.

-fsanitize=float-divide-by-zero was accidentally removed by D63793. This patch adds it back.

rsmith added inline comments.Jul 8 2019, 12:32 PM
lib/Driver/SanitizerArgs.cpp
27–30 ↗(On Diff #208410)

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.

test/Driver/fsanitize.c
844 ↗(On Diff #208410)

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.)

MaskRay updated this revision to Diff 208565.Jul 8 2019, 6:13 PM

Fix getPPTransparentSanitizers (test/Modules/check-for-sanitizer-feature.cpp) & ubsan_blacklist.txt

MaskRay marked 2 inline comments as done.Jul 8 2019, 6:24 PM
MaskRay added inline comments.
lib/Driver/SanitizerArgs.cpp
27–30 ↗(On Diff #208410)

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?

test/Driver/fsanitize.c
844 ↗(On Diff #208410)

Changed -target x86_64-linux to -target %itanium_abi_triple.

It seems Myriad also works - I have tried -target sparcel-myriad

MaskRay marked an inline comment as done.Jul 8 2019, 6:26 PM
MaskRay added inline comments.
test/Driver/fsanitize-blacklist.c
32 ↗(On Diff #208565)

These tests can probably be changed to use -target %itanium_abi_triple, too. I can do that if -target %itanium_abi_triple -fsanitize=float-divide-by-zero is proven to work...

rsmith accepted this revision.Jul 9 2019, 12:40 PM

LGTM, thanks!

This revision is now accepted and ready to land.Jul 9 2019, 12:40 PM
vitalybuka accepted this revision.Jul 9 2019, 12:47 PM
This revision was automatically updated to reflect the committed changes.