[Coverage] Do not emit coverage mappings for functions marked 'unused'

[vsk]

Summary

This lets us suppress coverage reporting for ~30 functions across llvm and clang.

I passed the test case through llvm-cov and verified that 'unused' functions are skipped, e.g:

3|       |void __attribute__((unused)) f1() {}

[arphaman]

Thanks for working on this, I have a couple of comments/questions:

• The current logic doesn't work correctly for a test case that's shown below:

class C2 {
  void __attribute__((unused)) m4() { }
  void __attribute__((unused)) m5();
};

The method 'm4' actually gets covered with a zero region. I suspect you might have to perform the check for the attribute in the 'EmptyCoverageMappingBuilder' class as well.

• I understand the value in ignoring coverage for unused functions which aren't actually used anywhere. But people can still use them freely, so do you think that it would be a good idea to provide coverage for those functions when they are actually used?

[vsk]

In D25040#556109, @arphaman wrote:

Thanks for working on this, I have a couple of comments/questions:

• The current logic doesn't work correctly for a test case that's shown below:

class C2 {
  void __attribute__((unused)) m4() { }
  void __attribute__((unused)) m5();
};

The method 'm4' actually gets covered with a zero region. I suspect you might have to perform the check for the attribute in the 'EmptyCoverageMappingBuilder' class as well.

Thanks, I'll take a look at that.

• I understand the value in ignoring coverage for unused functions which aren't actually used anywhere. But people can still use them freely, so do you think that it would be a good idea to provide coverage for those functions when they are actually used?

Yeah. Things like readCpuInfo() (lib/Support/Host.cpp) are marked 'unused', but we shouldn't suppress coverage for them.

Based on a quick skim [1], it seems like most functions marked 'unused' aren't called normally. Maybe that's all the more reason to provide coverage for them: if they *are* executed, it would be helpful to know. But the common case seems to be 'dump' or 'verify' methods that are called from lldb/gtest, and I don't think it's useful to have coverage for these.

I'm not sure how we'd determine that the function is actually used. Do you have an approach in mind? IIRC libclang has an API which allows you to count the users of a USR, but this isn't helpful without a view of all translation units.

[1] $ grep -rnE -A10 "LLVM_ATTRIBUTE_UNUSED( |$)" lib/

[arphaman]

In D25040#556165, @vsk wrote:

In D25040#556109, @arphaman wrote:

Thanks for working on this, I have a couple of comments/questions:

• The current logic doesn't work correctly for a test case that's shown below:

class C2 {
  void __attribute__((unused)) m4() { }
  void __attribute__((unused)) m5();
};

The method 'm4' actually gets covered with a zero region. I suspect you might have to perform the check for the attribute in the 'EmptyCoverageMappingBuilder' class as well.

Thanks, I'll take a look at that.

• I understand the value in ignoring coverage for unused functions which aren't actually used anywhere. But people can still use them freely, so do you think that it would be a good idea to provide coverage for those functions when they are actually used?

Yeah. Things like readCpuInfo() (lib/Support/Host.cpp) are marked 'unused', but we shouldn't suppress coverage for them.

Based on a quick skim [1], it seems like most functions marked 'unused' aren't called normally. Maybe that's all the more reason to provide coverage for them: if they *are* executed, it would be helpful to know. But the common case seems to be 'dump' or 'verify' methods that are called from lldb/gtest, and I don't think it's useful to have coverage for these.

I'm not sure how we'd determine that the function is actually used. Do you have an approach in mind? IIRC libclang has an API which allows you to count the users of a USR, but this isn't helpful without a view of all translation units.

We could create a special 'unused' region type which would cover unused functions. If a function is marked as unused, its first region will be the special one, and the normal regions will follow the special one. llvm-cov would then be able to ignore coverage for functions with the 'unused' first region if the function counter is zero, but it will still show it when unused functions have been actually executed. In terms of serialization the unused regions can be handled similarly to the 'SkippedRegion' type, so they won't increase the size of coverage mapping for functions that don't use the unused attribute.

[1] $ grep -rnE -A10 "LLVM_ATTRIBUTE_UNUSED( |$)" lib/

[vsk]

In D25040#556209, @arphaman wrote:

In D25040#556165, @vsk wrote:

In D25040#556109, @arphaman wrote:

Thanks for working on this, I have a couple of comments/questions:

• The current logic doesn't work correctly for a test case that's shown below:

class C2 {
  void __attribute__((unused)) m4() { }
  void __attribute__((unused)) m5();
};

The method 'm4' actually gets covered with a zero region. I suspect you might have to perform the check for the attribute in the 'EmptyCoverageMappingBuilder' class as well.

Thanks, I'll take a look at that.

• I understand the value in ignoring coverage for unused functions which aren't actually used anywhere. But people can still use them freely, so do you think that it would be a good idea to provide coverage for those functions when they are actually used?

Yeah. Things like readCpuInfo() (lib/Support/Host.cpp) are marked 'unused', but we shouldn't suppress coverage for them.

Based on a quick skim [1], it seems like most functions marked 'unused' aren't called normally. Maybe that's all the more reason to provide coverage for them: if they *are* executed, it would be helpful to know. But the common case seems to be 'dump' or 'verify' methods that are called from lldb/gtest, and I don't think it's useful to have coverage for these.

I'm not sure how we'd determine that the function is actually used. Do you have an approach in mind? IIRC libclang has an API which allows you to count the users of a USR, but this isn't helpful without a view of all translation units.

We could create a special 'unused' region type which would cover unused functions. If a function is marked as unused, its first region will be the special one, and the normal regions will follow the special one. llvm-cov would then be able to ignore coverage for functions with the 'unused' first region if the function counter is zero, but it will still show it when unused functions have been actually executed. In terms of serialization the unused regions can be handled similarly to the 'SkippedRegion' type, so they won't increase the size of coverage mapping for functions that don't use the unused attribute.

I think this is the best approach, but afaict it would require updating the coverage mapping format (http://llvm.org/docs/CoverageMappingFormat.html#pseudo-counter). That seems like a lot of work for not very much gain.. I might sit on this patch for a while until/unless I can think of something simpler.

lib/CodeGen/CoverageMappingGen.cpp

       extendRegion(S);
     for (const Stmt *Child : S->children())
       if (Child)
         this->Visit(Child);
     handleFileExit(getEnd(S));
   }
 
   void VisitDecl(const Decl *D) {
+    // Do not visit unused functions.
+    if (const auto *FD = dyn_cast<FunctionDecl>(D))
+      if (FD->hasAttr<UnusedAttr>())
+        return;
+
     Stmt *Body = D->getBody();
 
     // Do not propagate region counts into system headers.
     if (Body && SM.isInSystemHeader(SM.getSpellingLoc(getStart(Body))))
       return;
 
     propagateCounts(getRegionCounter(Body), Body);
   }

test/CoverageMapping/skip_unused_function.cpp

+// RUN: %clang_cc1 -std=c++11 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %s | count 0
+
+void __attribute__((unused)) f1() {}
+
+template<typename T>
+class C1 {
+public:
+  void __attribute__((unused)) m1() {}
+
+  static void __attribute__((unused)) m2(T x) {}
+
+  void __attribute__((unused)) m3();
+};
+
+template <typename T>
+void C1<T>::m3() {
+  C1<int> v1;
+  v1.m1();
+  C1<float>::m2(0.0);
+}