Page MenuHomePhabricator

[Clang] Enable __has_feature(coverage_sanitizer)
ClosedPublic

Authored by melver on May 26 2021, 5:31 AM.

Details

Summary

Like other sanitizers, enable __has_feature(coverage_sanitizer) if clang
has enabled at least one SanitizerCoverage instrumentation type.

Because coverage instrumentation selection is not handled via normal
-fsanitize= (and thus not in SanitizeSet), passing this information
through to LangOptions required propagating the already parsed
-fsanitize-coverage= options from CodeGenOptions through to LangOptions
in FixupInvocation().

Diff Detail

Event Timeline

melver created this revision.May 26 2021, 5:31 AM
melver requested review of this revision.May 26 2021, 5:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2021, 5:31 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
melver updated this revision to Diff 347936.May 26 2021, 5:33 AM

Documentation wording.

aaron.ballman added inline comments.
clang/docs/SanitizerCoverage.rst
319–321
clang/include/clang/Basic/Features.def
52

I think this is likely fine, but wanted to call it out explicitly in case others had opinions.

FEATURE is supposed to be used for standard features and EXTENSION used for Clang extensions. This is an extension, not a standard feature, so it's wrong in that way. However, it's following the same pattern as the other sanitizers which is consistent. I think consistently wrong is better than inconsistently right for this case, but I have no idea how others feel.

melver added inline comments.May 26 2021, 10:55 AM
clang/include/clang/Basic/Features.def
52

Yes, you are correct of course, and I was pondering the same thing.

In the end I'd like all sanitizers be queryable via __has_feature() and not have this be the odd one out requiring __has_extension() as that's also going to lead to confusion/errors on the user side.

melver updated this revision to Diff 348021.May 26 2021, 10:59 AM
melver marked an inline comment as done.

s/Since/Because/

ojeda added a subscriber: ojeda.May 26 2021, 11:07 AM
ojeda added inline comments.May 26 2021, 11:12 AM
clang/include/clang/Basic/Features.def
52

Perhaps add both, deprecate __has_feature() for non-standard features like these ones, and remove them after a couple releases? :)

Regardless of the way, any is better than a version check, so thanks!

melver added inline comments.May 26 2021, 11:19 AM
clang/include/clang/Basic/Features.def
52

I think realistically we have to pick one, and that's the one we keep for all eternity. :-)

Because if we deprecate/remove something, some codebases would require version checks, which is a non-starter again. Not every project is on top of what their compilers deprecates/removes. (And, unlike the Linux kernel, some codebases just never upgrade their compiler requirements, but still expect newer compilers to work.)

So if we want consistency with other sanitizers, it has to be __has_feature().

ojeda added inline comments.May 26 2021, 11:43 AM
clang/include/clang/Basic/Features.def
52

Agreed, any friction on updates is bad for users and will annoy someone somewhere.

Having said that, any serious project migrating to a new toolchain needs to revalidate regardless. And, after all, these are non-standard features. So in practice I do not think it would matter too much if the deprecation notice is long enough (several years).

But I may be saying something completely stupid, since I do not even know if Clang promises forever-stability for options, like rustc does.

melver marked 3 inline comments as done.May 27 2021, 8:09 AM

Ping.

To reviewers: Do note the feature vs. extension discussion.
Summary: We think to be consistent with other sanitizers and avoid confusion, we must make this a feature, too.

Thanks.

aaron.ballman accepted this revision.May 27 2021, 8:45 AM

Ping.

FWIW, the usual practice is to ping after no activity on the review for about a week.

That said, LGTM!

clang/include/clang/Basic/Features.def
52

I agree with @melver that it'd be worse to deprecate the feature testing macro. Then you have to use compiler version checks to decide which way to spell the feature testing macro, which largely defeats the entire purpose of having feature testing macros in the first place. I think we're basically stuck with a policy that all the sanitizers can be tested as features rather than extensions.

This revision is now accepted and ready to land.May 27 2021, 8:45 AM
This revision was landed with ongoing or failed builds.May 27 2021, 9:25 AM
This revision was automatically updated to reflect the committed changes.

Ping.

FWIW, the usual practice is to ping after no activity on the review for about a week.

That said, LGTM!

Thanks! And sorry for the early Ping.
We had an earlier version of a patch using the no_sanitize("coverage") attribute in Linux -next and it was causing build robots to complain because they somehow want to use a pre-release of Clang 13 (they probably shouldn't, and also probably not a big deal about them suddenly crashing as they should just upgrade).
In any case, I thought we should get this fixed sooner than later -- and I just sent this: https://lkml.kernel.org/r/20210527162655.3246381-1-elver@google.com