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

[Vedant Kumar via cfe-dev]

> On Sep 27, 2016, at 3:06 PM, Justin Bogner <mail at justinbogner.com> 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.

I think not having coverage for functions marked "used" is a little iffy since
users are free to apply that attribute to interesting functions. But, you raise
a great point about the unused attribute -- we should do something with it.


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

Sorry, that was a thinko. I meant to write 'derived'. Example:

  struct Base {
    virtual int foo() { return 0; }
  };
  
  struct Derived : public Base {
    virtual int foo() = delete;          //< Illegal.
    virtual int foo() { unreachable(); } //< OK, but uncovered.
  };

The derived version of foo() should just be marked unused.


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

We could bump up the function coverage of a lot of the files in llvm by
suppressing coverage reporting at the function level. Though, admittedly, not
by much, and a lot of the candidate functions *are* already marked 'used'.


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

Yeah, anything marked noreturn. We don't need a new attribute to do this.


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

Agreed. I'm taking this to mean that there isn't a lot of interest in
suppressing coverage mapping generation for specific macros.


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

Hm, do we really need a CFG?

I was thinking we could add an 'Unreachable' bit to SourceMappingRegion. Then
when we're emitting source regions, treat unreachable regions like skipped
ones (i.e makeSkipped(), not makeRegion()):

  void VisitCallExpr(const CallExpr *E) {
    if (E is not marked 'noreturn')
      just visit its children and return, as we normally would

    terminateRegion(E); //< Push a new region.
    getRegion().setUnreachable(true);
  }

** hand-waving intensifies **

vedant