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

[Mehdi Amini via cfe-dev]

> On Sep 27, 2016, at 3:06 PM, Justin Bogner via cfe-dev <cfe-dev at lists.llvm.org> wrote:
> 
> 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.


Isn’t happening when you have an "optional feature”, like if XX is enabled then this method has to be overridden/implemented, otherwise it is not expected to be called.

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

__attribute__((__used__)) is intended to be added on method called from inline assembly.
It is not clear why we should key them off?


> 
> 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?

I agree, not creating coverage for unreachable blocks would be nice as well!
Not clear if a CFG is needed though, I expect that most of the time the frontend can only see a single unreachable block that should be removed from the coverage map.


— 
Mehdi