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

[Justin Bogner via cfe-dev]

Mehdi Amini <mehdi.amini at apple.com> writes:
>> 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.

Fair enough, I don't really like that pattern but it does happen.

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

The place where I see __attribute__((__used__)) come up pretty often is
in methods that aren't used but you want to emit in the final binary so
that people can call them in a debugger. An example of this is
LLVM_DUMP_METHOD. For that case, the programmer promises "hey this
really is used", but it isn't actually covered since its never called.

OTOH, coverage for things called by inline assembly is definitely
interesting, so it probably isn't safe to suppress coverage for all
__used__ functions.

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