[cfe-dev] RFC: Attribute to suppress coverage mapping for functions

[Justin Bogner via cfe-dev]

Vedant Kumar via cfe-dev <cfe-dev at lists.llvm.org> writes:
> Hi,
>
> I'd like to add a new attribute which can be used to suppress code coverage
> reporting on the function level. This is meant to work with clang's
> frontend-based coverage implementation [1]. Here are some potential users of
> the new attribute:
>
>   * llvm_unreachable_internal.

Sure, but see below.

>   * The various dump() functions for llvm Values, AST nodes, etc.

For things like dump() you can probably just key on used/unused
attributes. We statically know at compile time it isn't actually used,
so we also know that coverage isn't interesting.

>   * Methods in a base class which just call llvm_unreachable.

These are usually indicative of bad design no? Why aren't they deleted
or abstract instead? I'm not sure how valuable suppressing them from
coverage is.

> These functions are usually not covered. It's not practical to write "death
> tests" for all of them, and it's also unhelpful to report missing coverage for
> them.
>
> I'd like to be able to write this in C:
>
>   void foo() __attribute__((nocoverage)) { llvm_unreachable("boom!"); }
>
> And this in C++11:
>
>   void foo() [[clang::nocoverage]] { llvm_unreachable("boom!"); }

I don't think this really goes far enough to be worthwhile. The number
of functions that are intentionally completely uncovered is minuscule
(or have things like __attribute__((__used__)) that we should be able to
key off of already).

I guess what you're really trying to do longer term is gain the ability
to recognize blocks in the caller that aren't supposed to be reached
(like a __builtin_unreachable after a fully covered switch) and suppress
coverage for them. Admittedly these annotations would make that easier,
but then we need to propagate them up through the call graph anyway like
your suggestion below, so recognizing __builtin_unreachable and abort is
probably enough.

> Here are some alternatives and why I think they're not as good:
>
>   * Define a preprocessor macro when -fcoverage-mapping is enabled.
>
>     Conditionally compiling code based on whether code coverage is enabled
>     sounds scary. We shouldn't make it easy (or possible?) to change the
>     meaning of a program by enabling coverage.

Agreed, I don't like this either.

>   * Pass a function blacklist to llvm-cov.
>
>     The blacklist would have to live separately from the source code, and may
>     get out of sync. We also would go through the trouble of emitting coverage
>     mappings for functions even though they aren't needed.
>
>   * Add a pair of pragmas to arbitrarily stop/resume coverage mapping.
>
>     We'd need some extra diagnostics to catch abuse of the pragmas. It also
>     requires more typing in the common case (disabling coverage at the function
>     level).

This seems quite a bit more awkward, so I think it wouldn't be useable
enough to be worth it, but it is strictly more flexible if you ended up
wanting to annotate unreachable blocks directly.

>   * Look at the function CFG. If all paths through the function can be shown to
>     reach __builtin_unreachable(), don't create coverage mappings for the
>     function.
>
>     I'm worried this might be complicated and inflexible. It wouldn't let us
>     mark dump() functions as uncovered.

Like I said above, we basically need something like this anyway if we
want to do anything more interesting with the attribute, no?

> Wdyt?
>
> vedant
>
> [1] http://clang.llvm.org/docs/SourceBasedCodeCoverage.html
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev