On Tue, May 25, 2021 at 7:59 PM Marco Elver <[email protected]> wrote:
>
> +#if defined(CONFIG_KCOV) && CONFIG_CLANG_VERSION >= 130000
Is there any reason why Clang does not implement
`__has_attribute(__no_sanitize_coverage__)` like GCC? That way we can
merge both (perhaps even in `compiler_attributes.h`).
Cheers,
Miguel
On Tue, 25 May 2021 at 20:25, Miguel Ojeda
<[email protected]> wrote:
> On Tue, May 25, 2021 at 7:59 PM Marco Elver <[email protected]> wrote:
> >
> > +#if defined(CONFIG_KCOV) && CONFIG_CLANG_VERSION >= 130000
>
> Is there any reason why Clang does not implement
> `__has_attribute(__no_sanitize_coverage__)` like GCC? That way we can
> merge both (perhaps even in `compiler_attributes.h`).
It's complicated. Clang implements all no_sanitize options via
no_sanitize(<string_literal>), except for 3 which are there for
backwards-compatibility reasons (no_sanitize_{address,memory,thread}).
But otherwise, no_sanitize_sanitizer is deprecated in Clang in favor
of no_sanitize("sanitizer") per comment at
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Attr.td#L2907.
(That being said, there's already inconsistency due to coverage
instrumentation requiring "-fsanitize-coverage=..." and not
"-fsanitize=coverage-...". The implementation vs other no_sanitize is
also a bit special, see LLVM commit.)
This means we only have __has_attribute(no_sanitize). Which is also
the reason why the other __no_sanitize_* defines in compiler-clang.h
first check the feature, as feature existence implies attribute
existence. But, sadly, this is not the case for coverage
instrumentation (where in fact, __has_feature(coverage_sanitizer)
doesn't work either...)
From a UX perspective, having Clang only give us no_sanitize("...")
without the corresponding __has_attribute() support is not great, but
passable due to __has_feature() working for other sanitizers. From
Clang's perspective, it kept things simpler because we've gotten quite
a number of sanitizers recently. The big ones are manageable [1], but
UBSan is just too much [2].
[1] https://clang.llvm.org/docs/UsersManual.html#controlling-code-generation
[2] https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#ubsan-checks
This is probably a longer answer to your question, but is a summary of
the frustrations I encountered as I looked deeper into Clang's
no_sanitize attribute.
Long story short: this is not fixable without more Clang changes. The
only way to do it without a version check would be to introduce
no_sanitize_coverage attr to Clang, which we probably shouldn't do,
and I didn't want to fight it. ;-)
Thanks,
-- Marco
On Tue, May 25, 2021 at 9:13 PM Marco Elver <[email protected]> wrote:
>
> Long story short: this is not fixable without more Clang changes. The
> only way to do it without a version check would be to introduce
> no_sanitize_coverage attr to Clang, which we probably shouldn't do,
> and I didn't want to fight it. ;-)
I am not sure I followed why you would not want to support querying
for the attributes (if they are intended to be used separately).
But regardless of that, why not the feature flag at least then, to be
consistent with the others?
Going back to version checks seems bad -- they should be reserved for
e.g. known broken versions and things like that. New compiler features
should come with new feature flags...
In fact, for Clang, I do not see any version checks in code at the
moment, so this would be the first :(
Cheers,
Miguel
On Wed, 26 May 2021 at 03:54, Miguel Ojeda
<[email protected]> wrote:
> On Tue, May 25, 2021 at 9:13 PM Marco Elver <[email protected]> wrote:
> >
> > Long story short: this is not fixable without more Clang changes. The
> > only way to do it without a version check would be to introduce
> > no_sanitize_coverage attr to Clang, which we probably shouldn't do,
> > and I didn't want to fight it. ;-)
>
> I am not sure I followed why you would not want to support querying
> for the attributes (if they are intended to be used separately).
Not my decision, but some historical decision in Clang. Somebody
thought "no_sanitize(<string_literal>)" simplifies things. Hence,
Clang only knows about the no_sanitize attribute but not its
"subattributes".
> But regardless of that, why not the feature flag at least then, to be
> consistent with the others?
__has_feature(coverage_sanitizer) does not work either (yet).
> Going back to version checks seems bad -- they should be reserved for
> e.g. known broken versions and things like that. New compiler features
> should come with new feature flags...
>
> In fact, for Clang, I do not see any version checks in code at the
> moment, so this would be the first :(
In this instance it's absolutely required (for now). But if you don't
like it I'll go back to trying to fix Clang more. I'll check with
Clang folks which one we can implement, the feature check or the
attribute check.
> Cheers,
> Miguel
On Wed, 26 May 2021 at 08:25, Marco Elver <[email protected]> wrote:
> On Wed, 26 May 2021 at 03:54, Miguel Ojeda
> <[email protected]> wrote:
> > On Tue, May 25, 2021 at 9:13 PM Marco Elver <[email protected]> wrote:
> > >
> > > Long story short: this is not fixable without more Clang changes. The
> > > only way to do it without a version check would be to introduce
> > > no_sanitize_coverage attr to Clang, which we probably shouldn't do,
> > > and I didn't want to fight it. ;-)
> >
> > I am not sure I followed why you would not want to support querying
> > for the attributes (if they are intended to be used separately).
>
> Not my decision, but some historical decision in Clang. Somebody
> thought "no_sanitize(<string_literal>)" simplifies things. Hence,
> Clang only knows about the no_sanitize attribute but not its
> "subattributes".
>
> > But regardless of that, why not the feature flag at least then, to be
> > consistent with the others?
>
> __has_feature(coverage_sanitizer) does not work either (yet).
>
> > Going back to version checks seems bad -- they should be reserved for
> > e.g. known broken versions and things like that. New compiler features
> > should come with new feature flags...
> >
> > In fact, for Clang, I do not see any version checks in code at the
> > moment, so this would be the first :(
>
> In this instance it's absolutely required (for now). But if you don't
> like it I'll go back to trying to fix Clang more. I'll check with
> Clang folks which one we can implement, the feature check or the
> attribute check.
Ok, let's wait for response to: https://reviews.llvm.org/D103159
If that lands in the LLVM repo I'll change to use
__has_feature(coverage_sanitizer), and send a v2. That __has_feature()
is a bit of a lie though, because fsanitize-coverage has long been
supported, but it just so happens that if we get it, then its
availability implies availability of the no_sanitize("coverage")
attribute.
Thanks,
-- Marco
On Wed, May 26, 2021 at 2:38 PM Marco Elver <[email protected]> wrote:
>
> Ok, let's wait for response to: https://reviews.llvm.org/D103159
> If that lands in the LLVM repo I'll change to use
> __has_feature(coverage_sanitizer), and send a v2. That __has_feature()
> is a bit of a lie though, because fsanitize-coverage has long been
> supported, but it just so happens that if we get it, then its
> availability implies availability of the no_sanitize("coverage")
> attribute.
Thanks a lot for that! Appreciated :)
Cheers,
Miguel