[Clang] Enable __has_feature(coverage_sanitizer)

[melver]

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

[melver]

Documentation wording.

[aaron.ballman]

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]

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]

s/Since/Because/

[ojeda]

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]

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]

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]

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]

In D103159#2784845, @melver wrote:

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.

[melver]

In D103159#2784926, @aaron.ballman wrote:

In D103159#2784845, @melver wrote:

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/[e-mail]

clang/docs/SanitizerCoverage.rst

.. code-block:: c++
   // Called before a GetElemementPtr (GEP) instruction
   // for every non-constant array index.
   void __sanitizer_cov_trace_gep(uintptr_t Idx);
 
 Disabling instrumentation with ``__attribute__((no_sanitize("coverage")))``
 ===========================================================================
 
 It is possible to disable coverage instrumentation for select functions via the
-function attribute ``__attribute__((no_sanitize("coverage")))``.
+function attribute ``__attribute__((no_sanitize("coverage")))``. Because this
+attribute may not be supported by other compilers, it is recommended to use it
+together with ``__has_feature(coverage_sanitizer)``.
 
 Disabling instrumentation without source modification
 =====================================================
 
 It is sometimes useful to tell SanitizerCoverage to instrument only a subset of the
 functions in your target without modifying source files.
 With ``-fsanitize-coverage-allowlist=allowlist.txt``
 and ``-fsanitize-coverage-blocklist=blocklist.txt``,

clang/include/clang/Basic/Features.def

FEATURE(leak_sanitizer,
         LangOpts.Sanitize.has(SanitizerKind::Leak))
 FEATURE(hwaddress_sanitizer,
         LangOpts.Sanitize.hasOneOf(SanitizerKind::HWAddress |
                                    SanitizerKind::KernelHWAddress))
 FEATURE(memtag_sanitizer, LangOpts.Sanitize.has(SanitizerKind::MemTag))
 FEATURE(xray_instrument, LangOpts.XRayInstrument)
 FEATURE(undefined_behavior_sanitizer,
         LangOpts.Sanitize.hasOneOf(SanitizerKind::Undefined))
+FEATURE(coverage_sanitizer, LangOpts.SanitizeCoverage)
 FEATURE(assume_nonnull, true)
 FEATURE(attribute_analyzer_noreturn, true)
 FEATURE(attribute_availability, true)
 FEATURE(attribute_availability_with_message, true)
 FEATURE(attribute_availability_app_extension, true)
 FEATURE(attribute_availability_with_version_underscores, true)
 FEATURE(attribute_availability_tvos, true)
 FEATURE(attribute_availability_watchos, true)

clang/include/clang/Basic/LangOptions.h

public:
   };
 
 public:
   /// The used language standard.
   LangStandard::Kind LangStd;
 
   /// Set of enabled sanitizers.
   SanitizerSet Sanitize;
+  /// Is at least one coverage instrumentation type enabled.
+  bool SanitizeCoverage = false;
 
   /// Paths to files specifying which objects
   /// (files, functions, variables) should not be instrumented.
   std::vector<std::string> NoSanitizeFiles;
 
   /// Paths to the XRay "always instrument" files specifying which
   /// objects (files, functions, variables) should be imbued with the XRay
   /// "always instrument" attribute.

clang/lib/Frontend/CompilerInvocation.cpp

static bool FixupInvocation(CompilerInvocation &Invocation,
   CodeGenOptions &CodeGenOpts = Invocation.getCodeGenOpts();
   TargetOptions &TargetOpts = Invocation.getTargetOpts();
   FrontendOptions &FrontendOpts = Invocation.getFrontendOpts();
   CodeGenOpts.XRayInstrumentFunctions = LangOpts.XRayInstrument;
   CodeGenOpts.XRayAlwaysEmitCustomEvents = LangOpts.XRayAlwaysEmitCustomEvents;
   CodeGenOpts.XRayAlwaysEmitTypedEvents = LangOpts.XRayAlwaysEmitTypedEvents;
   CodeGenOpts.DisableFree = FrontendOpts.DisableFree;
   FrontendOpts.GenerateGlobalModuleIndex = FrontendOpts.UseGlobalModuleIndex;
-
+  LangOpts.SanitizeCoverage = CodeGenOpts.hasSanitizeCoverage();
   LangOpts.ForceEmitVTables = CodeGenOpts.ForceEmitVTables;
   LangOpts.SpeculativeLoadHardening = CodeGenOpts.SpeculativeLoadHardening;
   LangOpts.CurrentModule = LangOpts.ModuleName;
 
   llvm::Triple T(TargetOpts.Triple);
   llvm::Triple::ArchType Arch = T.getArch();
 
   CodeGenOpts.CodeModel = TargetOpts.CodeModel;

clang/test/Lexer/has_feature_coverage_sanitizer.cpp

+// RUN: %clang -E -fsanitize-coverage=indirect-calls %s -o - | FileCheck --check-prefix=CHECK-SANCOV %s
+// RUN: %clang -E -fsanitize-coverage=inline-8bit-counters %s -o - | FileCheck --check-prefix=CHECK-SANCOV %s
+// RUN: %clang -E -fsanitize-coverage=trace-cmp %s -o - | FileCheck --check-prefix=CHECK-SANCOV %s
+// RUN: %clang -E -fsanitize-coverage=trace-pc %s -o - | FileCheck --check-prefix=CHECK-SANCOV %s
+// RUN: %clang -E -fsanitize-coverage=trace-pc-guard %s -o - | FileCheck --check-prefix=CHECK-SANCOV %s
+// RUN: %clang -E  %s -o - | FileCheck --check-prefix=CHECK-NO-SANCOV %s
+
+#if __has_feature(coverage_sanitizer)
+int SancovEnabled();
+#else
+int SancovDisabled();
+#endif
+
+// CHECK-SANCOV: SancovEnabled
+// CHECK-NO-SANCOV: SancovDisabled