Skip to content

Fix coverage reports for ext_proc tests#39041

Merged
botengyao merged 2 commits intoenvoyproxy:mainfrom
krinkinmu:fix-ext_proc-coverage
Apr 8, 2025
Merged

Fix coverage reports for ext_proc tests#39041
botengyao merged 2 commits intoenvoyproxy:mainfrom
krinkinmu:fix-ext_proc-coverage

Conversation

@krinkinmu
Copy link
Copy Markdown
Contributor

Commit Message:

In #38898 @botengyao added an exception for the ext_proc tests for coverage. That's because clang source based coverage reported missing coverage for the comments (the desirable behavior here is that comments are not counted in the coverage reports).

Digging through it, what I realized is that for only some tests report counters for the comments. And all the tests that report thos bogus counters don't actually exercise the affected code.

I still don't fully understand what triggers the issue here, but that observation suggests a workaround that this PR implements. I basically remove dependency on the grpc_client_impl.h from the tests that don't need it.

I started a thread on LLVM discussions (https://discourse.llvm.org/t/llvm-source-based-coverage-sometimes-generates-counter-for-comments/85730), but given that I couldn't come up with a small enough reproducer, I don't think that we can resolve it quickly.

That being said, removing unnecessary includes and moving functions from headers to cc files is in general a good strategy as it reduces depndencies between translation units and reduces the amount of work compiler has to do, so I think we can still push this workaround upstream, even though I don't have a root cause for the problem.

Additional Description:

It was reported in #37911, but other than that I don't think it's relevant.

Risk Level: n/a
Testing: regular CI tests, but with coverage targets changed to remove the exception for the ext_proc.
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

In envoyproxy#38898 @botengyao added an exception for the ext_proc tests for
coverage. That's because clang source based coverage reported missing
coverage for the comments (the desirable behavior here is that comments
are not counted in the coverage reports).

Digging through it, what I realized is that for only some tests report
counters for the comments. And all the tests that report thos bogus
counters don't actually exercise the affected code.

I still don't fully understand what triggers the issue here, but that
observation suggests a workaround that this PR implements. I basically
remove dependency on the grpc_client_impl.h from the tests that don't
need it.

I started a thread on LLVM discussions, but given that I couldn't come
up with a small enough reproducer, I don't think that we can resolve it
quickly.

That being said, removing unnecessary includes and moving functions from
headers to cc files is in general a good strategy as it reduces
depndencies between translation units and reduces the amount of work
compiler has to do, so I think we can still push this workaround
upstream, even though I don't have a root cause for the problem.

Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>
@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #39041 was opened by krinkinmu.

see: more, trace.

@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/coverage-shephards: FYI only for changes made to (test/per_file_coverage.sh).
envoyproxy/coverage-shephards assignee is @RyanTheOptimist

🐱

Caused by: #39041 was opened by krinkinmu.

see: more, trace.

Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>
@yanjunxiang-google
Copy link
Copy Markdown
Contributor

/assign @yanjunxiang-google

@krinkinmu krinkinmu marked this pull request as ready for review April 8, 2025 17:44
Copy link
Copy Markdown
Member

@botengyao botengyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Mike for the fix!

@yanjunxiang-google
Copy link
Copy Markdown
Contributor

LGTM the change. It's a mystery how this change brought back the coverage number though.

@botengyao botengyao merged commit 4409064 into envoyproxy:main Apr 8, 2025
24 checks passed
@krinkinmu
Copy link
Copy Markdown
Contributor Author

@yanjunxiang-google @botengyao FWIW, I debugged it a bit more and I believe it's a bug in clang (or at least weird interaction of source-based coverage with template instantiation). I was able to create a reasonably small reproducer and filed a bug in llvm-project for that: llvm/llvm-project#135049. I think we have a fair chance that somebody will look at it and maybe fix it.

That being said, the sad reality seem to be that clang source-based code coverage (that's what we currently use) is riddles with weird corner cases and obscure bugs (some bugs that has been there for years and are not getting fixed). So it should be used very carefully to produce good results.

Here are some tips that I fould working well in the past and working around the issues in source based covearage:

  1. Don't use dynamic linking for coverage tests (Envoy currently does use dynamic linking and I'd like to change that)
  2. Don't put implementations in the header files unless absolutely necessary
  3. If you have to put implementations in header files mark those with used attribute to make compiler actually emit the code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants