Skip to content

Don't link dynamically when running coverage#38447

Closed
krinkinmu wants to merge 9 commits into
envoyproxy:mainfrom
krinkinmu:coverage
Closed

Don't link dynamically when running coverage#38447
krinkinmu wants to merge 9 commits into
envoyproxy:mainfrom
krinkinmu:coverage

Conversation

@krinkinmu
Copy link
Copy Markdown
Contributor

Commit Message:

The long story short, source-based coverage in clang does not seem to play well with dynamic linking when there are functions defined in header files included in multiple places (inline functions and class members).

The exact details are way above my head, but the visible effect is that some functions, that are in fact exercised during tests will be reported as not covered, because LLVM toolchain may in some circumstances fail to merge information about multiple versions of the same function.

You can refer to
https://discourse.llvm.org/t/llvm-cov-hash-mismatches-originating-from-class-methods-implemented-in-header-files/79832 for more information.

There are few ways to deal with this problem with different tradeoffs:

  1. [this change] stop using dynamic linking and switch to static linking, this addresses the problem, since there will be no multiple versions of the same function, but we started using dynamic linking for coverage because we had issues with storage/caching in CI a while back, so it might potentially cause issues there
  2. -femit-all-decls compile flag, can be used to work around the issue, it's good option because it could be applied to the whole repository via .bazelrc, but on the flip side, the compiler will have to do more work with this flag in place, increasing resource usage/reducing speed of the CI
  3. We can move individual affected function definitions from the header files to cc files, that addresses the problem for the affected function and reduces the compilation time somewhat, but it will have to be done on a function-by-function basis and in some cases it's not possible to do at all.
  4. Annotate affected function with used annotation, this will address the problem for that function and it's always possible, but just like in the option above it has to be done on function-by-function basis, and it will increase the work the compiler needs to do, because it's essentially an equivalent of -femit-all-decls, but applied to a single function and not the whole translation unit.

None of the options is great, and unfortunately even if the issue will get fixed upstream, it's not at all clear when the fix will be available in clang 18 if it will be available there at all.

Given that, I want to try the option 1, as "the most correct and future proof" option and see if the issues we tried to address by switching to dynamic linking for coverage tests are still affecting us.

Additional Description:

Related to #37911 and fixes one of the issues in #38093

Risk Level: Low
Testing: regular test runs
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

The long story short, source-based coverage in clang does not seem to
play well with dynamic linking when there are functions defined in
header files included in multiple places (inline functions and class
members).

The exact details are way above my head, but the visible effect is that
some functions, that are in fact exercised during tests will be reported
as not covered, because LLVM toolchain may in some circumstances fail to
merge information about multiple versions of the same function.

You can refer to
https://discourse.llvm.org/t/llvm-cov-hash-mismatches-originating-from-class-methods-implemented-in-header-files/79832
for more information.

There are few ways to deal with this problem with different tradeoffs:

1. [this change] stop using dynamic linking and switch to static
   linking, this addresses the problem, since there will be no multiple
   versions of the same function, but we started using dynamic linking
   for coverage because we had issues with storage/caching in CI a while
   back, so it might potentially cause issues there
2. -femit-all-decls compile flag, can be used to work around the issue,
   it's good option because it could be applied to the whole repository
   via .bazelrc, but on the flip side, the compiler will have to do more
   work with this flag in place, increasing resource usage/reducing
   speed of the CI
3. We can move individual affected function definitions from the header
   files to cc files, that addresses the problem for the affected
   function and reduces the compilation time somewhat, but it will have
   to be done on a function-by-function basis and in some cases it's not
   possible to do at all.
4. Annotate affected function with `used` annotation, this will address
   the problem for that function and it's always possible, but just like
   in the option above it has to be done on function-by-function basis,
   and it will increase the work the compiler needs to do, because it's
   essentially an equivalent of -femit-all-decls, but applied to a
   single function and not the whole translation unit.

None of the options is great, and unfortunately even if the issue will
get fixed upstream, it's not at all clear when the fix will be available
in clang 18 if it will be available there at all.

Given that, I want to try the option 1, as "the most correct and future
proof" option and see if the issues we tried to address by switching to
dynamic linking for coverage tests are still affecting us.

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: #38447 was opened by krinkinmu.

see: more, trace.

Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>
Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>
Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>
Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>
Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>
Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>
Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>
Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>
@krinkinmu krinkinmu closed this Feb 17, 2025
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.

1 participant