Fix a bug where instrumentation_filter doesn't control what files to …

[yliu98]

trafficstars

…be instrumente. link to the github issue: https://github.com/bazelbuild/bazel/issues/15627

[yliu98]

The reason is that at line 206 of CcModule.java, this method is called where the ruleContext could be null, the change here is just to protect against that and maintain the same behavior as before if ruleContext is indeed null. Please let me know if better fix is available. Thanks.

On Fri, Jun 10, 2022 at 2:07 AM Charles Mita @.***> wrote:

@.**** commented on this pull request.

In src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java https://github.com/bazelbuild/bazel/pull/15634#discussion_r894319762:

@@ -1036,7 +1038,9 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException( } }

• allFeatures.addAll(getCoverageFeatures(cppConfiguration));

• if (ruleContext == null || CcCompilationHelper.isCodeCoverageEnabled(ruleContext)) {

Does it make sense to permit ruleContext to be null? The semantics in this case are not intuitive to me. Is there even a context where this gets called from that doesn't have access to a rule context?

— Reply to this email directly, view it on GitHub https://github.com/bazelbuild/bazel/pull/15634#pullrequestreview-1002479682, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHC5ALPTXB7S7F4QLAUP6ULVOMAV3ANCNFSM5YDW7FWQ . You are receiving this because you authored the thread.Message ID: @.***>

[comius]

ping @c-mita

[c-mita]

The reason is that at line 206 of CcModule.java, this method is called where the ruleContext could be null, the change here is just to protect against that and maintain the same behavior as before if ruleContext is indeed null. Please let me know if better fix is available. Thanks. … On Fri, Jun 10, 2022 at 2:07 AM Charles Mita @.> wrote: @.* commented on this pull request. ------------------------------ In src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java <#15634 (comment)>: > @@ -1036,7 +1038,9 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException( } } - allFeatures.addAll(getCoverageFeatures(cppConfiguration)); + if (ruleContext == null || CcCompilationHelper.isCodeCoverageEnabled(ruleContext)) { Does it make sense to permit ruleContext to be null? The semantics in this case are not intuitive to me. Is there even a context where this gets called from that doesn't have access to a rule context? — Reply to this email directly, view it on GitHub <#15634 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHC5ALPTXB7S7F4QLAUP6ULVOMAV3ANCNFSM5YDW7FWQ . You are receiving this because you authored the thread.Message ID: @.***>

It can adjusted later if needed. LGTM

[fmeum]

@bazel-io flag

[sgowroji]

@bazel-io fork 5.3.0

[ShreeM01]

Hey @yliu98, Can you please Resolve conflicts? Thank you!

[c-mita]

This breaks coverage integration tests for some of our rules internally; I'm not entirely sure why and unfortunately I'm not able to do much debugging at this time.

[ShreeM01]

Hi @yliu98, can you please resolve the conflict if you want this changes to add in our 5.3.0 release? As we are close to our release date ,we will stop accepting new PR. Thanks!

[ShreeM01]

Hi @yliu98, Is there any update for this? Otherwise we will be moving this particular PR to our next release. Thanks!

[ShreeM01]

Hi @yliu98! We are marking the above PR as stale because it has not had any recent interaction from many days. It will be closed in 7 days if there is no further activity occurs. Thank you!

[yliu98]

Hi @c-mita, can you please share some details about the integration test failures you mentioned earlier? We will still need this change, so I'd like to know what had been broken by this cl. Thanks.

[c-mita]

The error in question is a link-time error, where coverage symbols referenced by a C++ source are not available. e.g.

ld: error: undefined symbol: llvm_gcda_emit_function
>>> referenced by foo.cc
>>>               [...]/foo.pic.o:(__llvm_gcov_writeout)

I believe what's happening is the following:

Suppose we have three targets (and two custom rules for language "x") all in the same package:

bar = cc_library(
  srcs = ["bar.cc"],
)

foo = x_library(
  srcs = ["foo.x"],
  cdeps = [":bar"],
)

foo_test = x_test(
  srcs = ["foo_test.x"],
  deps = [":foo"],
)

Where x_test is responsible for linking a final executable target.

With a default --instrumentation_filter, bar and foo will be instrumented and foo_test will not be. This means bar will be compiled with coverage enabled, but with this change, it means the coverage feature will be disabled by the time it gets to linking.

CcCompilationHelper::isCoverageEnabled mitigates this in the common case by checking deps to see if anything there is instrumented, but that won't help in the example above because the C++ dependency is coming from a different attribute.

x_test doesn't really have a way of handling this at the moment; there would need to be API changes to C++ feature configuration (enabling deciding on coverage configuration to occur outside of that call).

[fmeum]

@c-mita We are also running into this so I would be interested in moving it forward in some way. Do you think this could be worked around by having separate features for linking and compiling with coverage instrumentation? The latter could be safely disabled based on the filter, the former could remain enabled as long as --collect_code_coverage has been requested.

[c-mita]

Because more work is required to solve this, I want to move this discussion off this PR and onto the issue. I'm closing this PR because it isn't sufficient on its own and have described the problem encountered on #15627.