[PATCH v2] kcov: add __no_sanitize_coverage to fix noinstr for all architectures

[PATCH v2] kcov: add __no_sanitize_coverage to fix noinstr for all architectures

[Marco Elver]

Until now no compiler supported an attribute to disable coverage
instrumentation as used by KCOV.

To work around this limitation on x86, noinstr functions have their
coverage instrumentation turned into nops by objtool. However, this
solution doesn't scale automatically to other architectures, such as
arm64, which are migrating to use the generic entry code.

Clang [1] and GCC [2] have added support for the attribute recently.
[1] https://github.com/llvm/llvm-project/commit/280333021e9550d80f5c1152a34e33e81df1e178
[2] https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=cec4d4a6782c9bd8d071839c50a239c49caca689
The changes will appear in Clang 13 and GCC 12.

Add __no_sanitize_coverage for both compilers, and add it to noinstr.

Note: In the Clang case, __has_feature(coverage_sanitizer) is only true
if the feature is enabled, and therefore we do not require an additional
defined(CONFIG_KCOV) (like in the GCC case where __has_attribute(..) is
always true) to avoid adding redundant attributes to functions if KCOV
is off. That being said, compilers that support the attribute will not
generate errors/warnings if the attribute is redundantly used; however,
where possible let's avoid it as it reduces preprocessed code size and
associated compile-time overheads.

Signed-off-by: Marco Elver <elver@google.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
v2:
* Implement __has_feature(coverage_sanitizer) in Clang
  (https://reviews.llvm.org/D103159) and use instead of version check.
* Add Peter's Ack.
---
 include/linux/compiler-clang.h | 11 +++++++++++
 include/linux/compiler-gcc.h   |  6 ++++++
 include/linux/compiler_types.h |  2 +-
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index adbe76b203e2..e15eebfa8e5d 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -45,6 +45,17 @@
 #define __no_sanitize_undefined
 #endif
 
+/*
+ * Support for __has_feature(coverage_sanitizer) was added in Clang 13 together
+ * with no_sanitize("coverage"). Prior versions of Clang support coverage
+ * instrumentation, but cannot be queried for support by the preprocessor.
+ */
+#if __has_feature(coverage_sanitizer)
+#define __no_sanitize_coverage __attribute__((no_sanitize("coverage")))
+#else
+#define __no_sanitize_coverage
+#endif
+
 /*
  * Not all versions of clang implement the type-generic versions
  * of the builtin overflow checkers. Fortunately, clang implements
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 5d97ef738a57..cb9217fc60af 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -122,6 +122,12 @@
 #define __no_sanitize_undefined
 #endif
 
+#if defined(CONFIG_KCOV) && __has_attribute(__no_sanitize_coverage__)
+#define __no_sanitize_coverage __attribute__((no_sanitize_coverage))
+#else
+#define __no_sanitize_coverage
+#endif
+
 #if GCC_VERSION >= 50100
 #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
 #endif
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index d29bda7f6ebd..cc2bee7f0977 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -210,7 +210,7 @@ struct ftrace_likely_data {
 /* Section for code which can't be instrumented at all */
 #define noinstr								\
 	noinline notrace __attribute((__section__(".noinstr.text")))	\
-	__no_kcsan __no_sanitize_address
+	__no_kcsan __no_sanitize_address __no_sanitize_coverage
 
 #endif /* __KERNEL__ */
 
-- 
2.31.1.818.g46aad6cb9e-goog

Re: [PATCH v2] kcov: add __no_sanitize_coverage to fix noinstr for all architectures

[Miguel Ojeda]

On Thu, May 27, 2021 at 6:27 PM 'Marco Elver' via Clang Built Linux
<clang-built-linux@googlegroups.com> wrote:
>
> Note: In the Clang case, __has_feature(coverage_sanitizer) is only true
> if the feature is enabled, and therefore we do not require an additional
> defined(CONFIG_KCOV) (like in the GCC case where __has_attribute(..) is

I would put this explanation as a comment.

Other than that:

    Reviewed-by: Miguel Ojeda <ojeda@kernel.org>

Thanks!

Cheers,
Miguel

Re: [PATCH v2] kcov: add __no_sanitize_coverage to fix noinstr for all architectures

[Nick Desaulniers]

On Thu, May 27, 2021 at 9:27 AM Marco Elver <elver@google.com> wrote:
>
> Until now no compiler supported an attribute to disable coverage
> instrumentation as used by KCOV.
>
> To work around this limitation on x86, noinstr functions have their
> coverage instrumentation turned into nops by objtool. However, this
> solution doesn't scale automatically to other architectures, such as
> arm64, which are migrating to use the generic entry code.
>
> Clang [1] and GCC [2] have added support for the attribute recently.
> [1] https://github.com/llvm/llvm-project/commit/280333021e9550d80f5c1152a34e33e81df1e178
> [2] https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=cec4d4a6782c9bd8d071839c50a239c49caca689
> The changes will appear in Clang 13 and GCC 12.
>
> Add __no_sanitize_coverage for both compilers, and add it to noinstr.
>
> Note: In the Clang case, __has_feature(coverage_sanitizer) is only true
> if the feature is enabled, and therefore we do not require an additional
> defined(CONFIG_KCOV) (like in the GCC case where __has_attribute(..) is
> always true) to avoid adding redundant attributes to functions if KCOV
> is off. That being said, compilers that support the attribute will not
> generate errors/warnings if the attribute is redundantly used; however,
> where possible let's avoid it as it reduces preprocessed code size and
> associated compile-time overheads.
>
> Signed-off-by: Marco Elver <elver@google.com>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> v2:
> * Implement __has_feature(coverage_sanitizer) in Clang
>   (https://reviews.llvm.org/D103159) and use instead of version check.
> * Add Peter's Ack.
> ---
>  include/linux/compiler-clang.h | 11 +++++++++++
>  include/linux/compiler-gcc.h   |  6 ++++++
>  include/linux/compiler_types.h |  2 +-
>  3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index adbe76b203e2..e15eebfa8e5d 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -45,6 +45,17 @@
>  #define __no_sanitize_undefined
>  #endif
>
> +/*
> + * Support for __has_feature(coverage_sanitizer) was added in Clang 13 together
> + * with no_sanitize("coverage"). Prior versions of Clang support coverage
> + * instrumentation, but cannot be queried for support by the preprocessor.

I'm not against a version check for supporting older releases (in
addition to the cleaner feature check, since the feature check was
non-existent); we can clean it up someday when clang-13 is the
minimally supported version.  Would having an additional version check
help support existing/older releases here?

> + */
> +#if __has_feature(coverage_sanitizer)
> +#define __no_sanitize_coverage __attribute__((no_sanitize("coverage")))
> +#else
> +#define __no_sanitize_coverage
> +#endif
> +
>  /*
>   * Not all versions of clang implement the type-generic versions
>   * of the builtin overflow checkers. Fortunately, clang implements
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index 5d97ef738a57..cb9217fc60af 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -122,6 +122,12 @@
>  #define __no_sanitize_undefined
>  #endif
>
> +#if defined(CONFIG_KCOV) && __has_attribute(__no_sanitize_coverage__)
> +#define __no_sanitize_coverage __attribute__((no_sanitize_coverage))
> +#else
> +#define __no_sanitize_coverage
> +#endif
> +
>  #if GCC_VERSION >= 50100
>  #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
>  #endif
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index d29bda7f6ebd..cc2bee7f0977 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -210,7 +210,7 @@ struct ftrace_likely_data {
>  /* Section for code which can't be instrumented at all */
>  #define noinstr                                                                \
>         noinline notrace __attribute((__section__(".noinstr.text")))    \
> -       __no_kcsan __no_sanitize_address
> +       __no_kcsan __no_sanitize_address __no_sanitize_coverage
>
>  #endif /* __KERNEL__ */
>
> --
> 2.31.1.818.g46aad6cb9e-goog
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] kcov: add __no_sanitize_coverage to fix noinstr for all architectures

[Marco Elver]

On Tue, 1 Jun 2021 at 19:42, Nick Desaulniers <ndesaulniers@google.com> wrote:
> On Thu, May 27, 2021 at 9:27 AM Marco Elver <elver@google.com> wrote:
> >
> > Until now no compiler supported an attribute to disable coverage
> > instrumentation as used by KCOV.
> >
> > To work around this limitation on x86, noinstr functions have their
> > coverage instrumentation turned into nops by objtool. However, this
> > solution doesn't scale automatically to other architectures, such as
> > arm64, which are migrating to use the generic entry code.
> >
> > Clang [1] and GCC [2] have added support for the attribute recently.
> > [1] https://github.com/llvm/llvm-project/commit/280333021e9550d80f5c1152a34e33e81df1e178
> > [2] https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=cec4d4a6782c9bd8d071839c50a239c49caca689
> > The changes will appear in Clang 13 and GCC 12.
> >
> > Add __no_sanitize_coverage for both compilers, and add it to noinstr.
> >
> > Note: In the Clang case, __has_feature(coverage_sanitizer) is only true
> > if the feature is enabled, and therefore we do not require an additional
> > defined(CONFIG_KCOV) (like in the GCC case where __has_attribute(..) is
> > always true) to avoid adding redundant attributes to functions if KCOV
> > is off. That being said, compilers that support the attribute will not
> > generate errors/warnings if the attribute is redundantly used; however,
> > where possible let's avoid it as it reduces preprocessed code size and
> > associated compile-time overheads.
> >
> > Signed-off-by: Marco Elver <elver@google.com>
> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> > v2:
> > * Implement __has_feature(coverage_sanitizer) in Clang
> >   (https://reviews.llvm.org/D103159) and use instead of version check.
> > * Add Peter's Ack.
> > ---
> >  include/linux/compiler-clang.h | 11 +++++++++++
> >  include/linux/compiler-gcc.h   |  6 ++++++
> >  include/linux/compiler_types.h |  2 +-
> >  3 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> > index adbe76b203e2..e15eebfa8e5d 100644
> > --- a/include/linux/compiler-clang.h
> > +++ b/include/linux/compiler-clang.h
> > @@ -45,6 +45,17 @@
> >  #define __no_sanitize_undefined
> >  #endif
> >
> > +/*
> > + * Support for __has_feature(coverage_sanitizer) was added in Clang 13 together
> > + * with no_sanitize("coverage"). Prior versions of Clang support coverage
> > + * instrumentation, but cannot be queried for support by the preprocessor.
>
> I'm not against a version check for supporting older releases (in
> addition to the cleaner feature check, since the feature check was
> non-existent); we can clean it up someday when clang-13 is the
> minimally supported version.  Would having an additional version check
> help support existing/older releases here?

The feature check will just return 0 on older releases, since the
feature does not exist there. Therefore, no additional code is
required to support older releases and a version check would be
redundant.

> > + */
> > +#if __has_feature(coverage_sanitizer)
> > +#define __no_sanitize_coverage __attribute__((no_sanitize("coverage")))
> > +#else
> > +#define __no_sanitize_coverage
> > +#endif
> > +

Thanks,
-- Marco

Re: [PATCH v2] kcov: add __no_sanitize_coverage to fix noinstr for all architectures

[Marco Elver]

On Tue, 1 Jun 2021 at 19:46, Marco Elver <elver@google.com> wrote:
>
> On Tue, 1 Jun 2021 at 19:42, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > On Thu, May 27, 2021 at 9:27 AM Marco Elver <elver@google.com> wrote:
> > >
> > > Until now no compiler supported an attribute to disable coverage
> > > instrumentation as used by KCOV.
> > >
> > > To work around this limitation on x86, noinstr functions have their
> > > coverage instrumentation turned into nops by objtool. However, this
> > > solution doesn't scale automatically to other architectures, such as
> > > arm64, which are migrating to use the generic entry code.
> > >
> > > Clang [1] and GCC [2] have added support for the attribute recently.
> > > [1] https://github.com/llvm/llvm-project/commit/280333021e9550d80f5c1152a34e33e81df1e178
> > > [2] https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=cec4d4a6782c9bd8d071839c50a239c49caca689
> > > The changes will appear in Clang 13 and GCC 12.
> > >
> > > Add __no_sanitize_coverage for both compilers, and add it to noinstr.
> > >
> > > Note: In the Clang case, __has_feature(coverage_sanitizer) is only true
> > > if the feature is enabled, and therefore we do not require an additional
> > > defined(CONFIG_KCOV) (like in the GCC case where __has_attribute(..) is
> > > always true) to avoid adding redundant attributes to functions if KCOV
> > > is off. That being said, compilers that support the attribute will not
> > > generate errors/warnings if the attribute is redundantly used; however,
> > > where possible let's avoid it as it reduces preprocessed code size and
> > > associated compile-time overheads.
> > >
> > > Signed-off-by: Marco Elver <elver@google.com>
> > > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > ---
> > > v2:
> > > * Implement __has_feature(coverage_sanitizer) in Clang
> > >   (https://reviews.llvm.org/D103159) and use instead of version check.
> > > * Add Peter's Ack.
> > > ---
> > >  include/linux/compiler-clang.h | 11 +++++++++++
> > >  include/linux/compiler-gcc.h   |  6 ++++++
> > >  include/linux/compiler_types.h |  2 +-
> > >  3 files changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> > > index adbe76b203e2..e15eebfa8e5d 100644
> > > --- a/include/linux/compiler-clang.h
> > > +++ b/include/linux/compiler-clang.h
> > > @@ -45,6 +45,17 @@
> > >  #define __no_sanitize_undefined
> > >  #endif
> > >
> > > +/*
> > > + * Support for __has_feature(coverage_sanitizer) was added in Clang 13 together
> > > + * with no_sanitize("coverage"). Prior versions of Clang support coverage
> > > + * instrumentation, but cannot be queried for support by the preprocessor.
> >
> > I'm not against a version check for supporting older releases (in
> > addition to the cleaner feature check, since the feature check was
> > non-existent); we can clean it up someday when clang-13 is the
> > minimally supported version.  Would having an additional version check
> > help support existing/older releases here?
>
> The feature check will just return 0 on older releases, since the
> feature does not exist there. Therefore, no additional code is
> required to support older releases and a version check would be
> redundant.

And to avoid further confusion: -fsanitize-coverage exists, but the
feature "coverage_sanitizer" queryable by __has_feature() does not
exist. The confusion is the price we pay for this technical debt --
but I'd rather not write an essay about this in the comments. Most of
it is in the commit message, and if people are still confused I hope
they find this thread.

There was also a v3 explaining this more in the comments, too:
https://lkml.kernel.org/r/20210527194448.3470080-1-elver@google.com

Hopefully that is all enough.

> > > + */
> > > +#if __has_feature(coverage_sanitizer)
> > > +#define __no_sanitize_coverage __attribute__((no_sanitize("coverage")))
> > > +#else
> > > +#define __no_sanitize_coverage
> > > +#endif
> > > +
>
> Thanks,
> -- Marco