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

[Mehdi Amini via cfe-dev]

> On Sep 27, 2016, at 5:48 PM, Mehdi Amini via cfe-dev <cfe-dev at lists.llvm.org> wrote:
> 
>> 
>> 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.

To illustrate what I mean, see TargetLowering::LowerOperation for instance.

— 
Mehdi


> 
>> 
>>> 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
> 
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160927/db6e410c/attachment.html>