Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Coverage][NFC] Avoid visiting non-unique OpaqueValueExpr #88910

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

bolshakov-a
Copy link
Contributor

Only unique OpaqueValueExprs should be handled in the mapping builder, as discussed in #85837. However, getCond() returns non-unique OpaqueValueExpr for BinaryConditionalOperator (because it is also used as the "true" branch expression). Use getCommon() instead so as to bypass the OpaqueValueExpr.

@efriedma-quic

Only unique `OpaqueValueExpr`s should be handled in the mapping
builder, as discussed in llvm#85837. However, `getCond()` returns
non-unique `OpaqueValueExpr` for `BinaryConditionalOperator` (because
it is also used as the "true" branch expression). Use `getCommon()`
instead so as to bypass the `OpaqueValueExpr`.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen labels Apr 16, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 16, 2024

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Andrey Ali Khan Bolshakov (bolshakov-a)

Changes

Only unique OpaqueValueExprs should be handled in the mapping builder, as discussed in #85837. However, getCond() returns non-unique OpaqueValueExpr for BinaryConditionalOperator (because it is also used as the "true" branch expression). Use getCommon() instead so as to bypass the OpaqueValueExpr.

@efriedma-quic


Full diff: https://github.com/llvm/llvm-project/pull/88910.diff

1 Files Affected:

  • (modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+5-5)
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 71215da362d3d0..64c39c5de351c7 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -2011,11 +2011,13 @@ struct CounterCoverageMappingBuilder
     Counter TrueCount = llvm::EnableSingleByteCoverage
                             ? getRegionCounter(E->getTrueExpr())
                             : getRegionCounter(E);
-
-    propagateCounts(ParentCount, E->getCond());
     Counter OutCount;
 
-    if (!isa<BinaryConditionalOperator>(E)) {
+    if (const auto *BCO = dyn_cast<BinaryConditionalOperator>(E)) {
+      propagateCounts(ParentCount, BCO->getCommon());
+      OutCount = TrueCount;
+    } else {
+      propagateCounts(ParentCount, E->getCond());
       // The 'then' count applies to the area immediately after the condition.
       auto Gap =
           findGapAreaBetween(E->getQuestionLoc(), getStart(E->getTrueExpr()));
@@ -2024,8 +2026,6 @@ struct CounterCoverageMappingBuilder
 
       extendRegion(E->getTrueExpr());
       OutCount = propagateCounts(TrueCount, E->getTrueExpr());
-    } else {
-      OutCount = TrueCount;
     }
 
     extendRegion(E->getFalseExpr());

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

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

LGTM

@bolshakov-a
Copy link
Contributor Author

Could you merge it please?

@efriedma-quic
Copy link
Collaborator

Looks like automation didn't trigger for some reason... but quoting the automated message that's supposed to trigger:

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

@bolshakov-a
Copy link
Contributor Author

Please turn off Keep my email addresses private setting in your account.

Done.

@efriedma-quic efriedma-quic merged commit 949e66b into llvm:main Apr 19, 2024
7 checks passed
@bolshakov-a bolshakov-a deleted the cond_op_coverage_refactor branch May 16, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants