[PATCH] D76016: [ProfileData] Add option to ignore invalid regions

Thu Mar 19 18:07:36 PDT 2020

vsk added a comment.

In D76016#1932039 <https://reviews.llvm.org/D76016#1932039>, @modocache wrote:

> I see. No, I think whenever we have malformed coverage data, it points to an underlying bug in the compiler. Still, I think `llvm-cov`'s treatment of invalid data is too severe. Is there a good reason it should bail entirely? If it's able to process all but a single coverage region, it seems like it ought to report what it was able to process, no? I think that at least having the option for `llvm-cov` to behave this way is helpful, although I can understand if others disagree.
>
> For the time being, I'm working to fix the coverage data flaw in Clang's support of C++20 coroutines. But just to reiterate: it's not just coroutines and new language features that have caused invalid regions to appear in coverage data, there's also been cases such as D65428 <https://reviews.llvm.org/D65428>, which as far as I can tell is a bug in a long-existing language feature. In a sense, it's nice that `llvm-cov` "fails fast" and reports invalid data, but pragmatically it's also nice to have the capability to make it resilient to frontend bugs. At least, that's what I think -- let me know if you disagree :)

I definitely see your point of view. Actually I had a similar perspective when I first started working on this feature. The "fail hard, early" approach created.. quite a bit of up-front work. But the upshot was that we had more confidence in the coverage reports. And we didn't have to add tricky workarounds to hide partially incorrect data (incidentally, re: this patch, if a CounterMappingRegion got messed up, could it be that other CMRs in the function are not quite right?). If compiler bugs which break the coverage workflow are relatively rare (and afaict they now are), istm it's feasible/preferable to fix them early, instead of fleshing out and maintaining a separate alpha-quality coverage reporting mode.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76016/new/

https://reviews.llvm.org/D76016