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

Source-based code coverage incorrectly handles if constexpr block #54419

Closed
chfast opened this issue Mar 17, 2022 · 6 comments
Closed

Source-based code coverage incorrectly handles if constexpr block #54419

chfast opened this issue Mar 17, 2022 · 6 comments
Labels
clang:codegen clang:frontend Language frontend issues, e.g. anything involving "Sema" coverage

Comments

@chfast
Copy link
Member

chfast commented Mar 17, 2022

When a if constexpr has a block {} and the condition is false, the source-based code coverage always marks the opening { as not-executed region (see line 4).

Tested with clang 14.

    1|       |template <int C>
    2|      1|int test() {
    3|      1|    if constexpr (C == 0)
  ------------------
  |  Branch (3:19): [Folded - Ignored]
  ------------------
    4|      0|    {
    5|      1|        return 1;
    6|      1|    }
    7|      1|    return 0;
    8|      1|}
    9|       |
   10|       |int main()
   11|      1|{
   12|      1|    return test<1>();
   13|      1|}

@chfast chfast added clang Clang issues not falling into any other category coverage labels Mar 17, 2022
@chfast
Copy link
Member Author

chfast commented Mar 20, 2022

When viewing the AST of this program, the body of if constexpr is replaced with NullStmt. https://godbolt.org/z/x1zosvM43

|     |-IfStmt 0x557733b75ae0 <line:3:5, line:4:5> constexpr
|     | |-ConstantExpr 0x557733b75ab8 <line:3:19, col:24> 'bool'
|     | | |-value: Int 0
|     | | `-BinaryOperator 0x557733b75a98 <col:19, col:24> 'bool' '=='
|     | |   |-SubstNonTypeTemplateParmExpr 0x557733b75a78 <col:19> 'int'
|     | |   | |-NonTypeTemplateParmDecl 0x557733b752e8 <line:1:11, col:15> col:15 referenced 'int' depth 0 index 0 C
|     | |   | `-IntegerLiteral 0x557733b75a58 <line:3:19> 'int' 1
|     | |   `-IntegerLiteral 0x557733b75588 <col:24> 'int' 0
|     | `-NullStmt 0x557733b75ad8 <line:4:5>

@cor3ntin
Copy link
Contributor

if consteval is also not handled correctly. see #57377

cor3ntin added a commit that referenced this issue Aug 26, 2022
Clang crashes when encountering an `if consteval` statement.
This is the minimum fix not to crash.
The fix is consistent with the current behavior of if constexpr,
which does generate coverage data for the discarded branches.
This is of course not correct and a better solution is
needed for both if constexpr and if consteval.
See #54419.

Fixes #57377

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D132723
cor3ntin added a commit to cor3ntin/llvm-project that referenced this issue Sep 8, 2022
Clang crashes when encountering an `if consteval` statement.
This is the minimum fix not to crash.
The fix is consistent with the current behavior of if constexpr,
which does generate coverage data for the discarded branches.
This is of course not correct and a better solution is
needed for both if constexpr and if consteval.
See llvm#54419.

Fixes llvm#57377

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D132723
tru pushed a commit to llvm/llvm-project-release-prs that referenced this issue Sep 12, 2022
Clang crashes when encountering an `if consteval` statement.
This is the minimum fix not to crash.
The fix is consistent with the current behavior of if constexpr,
which does generate coverage data for the discarded branches.
This is of course not correct and a better solution is
needed for both if constexpr and if consteval.
See llvm/llvm-project#54419.

Fixes #57377

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D132723
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
Clang crashes when encountering an `if consteval` statement.
This is the minimum fix not to crash.
The fix is consistent with the current behavior of if constexpr,
which does generate coverage data for the discarded branches.
This is of course not correct and a better solution is
needed for both if constexpr and if consteval.
See llvm/llvm-project#54419.

Fixes #57377

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D132723
@hanickadot
Copy link
Contributor

I have a fix in mentioned PR. There was incorrectly marked one character after if constexpr (condition) as uncovered. Now whole region of valid THEN or ELSE branch is marked as uncovered. Also there was a similar problem with if consteval where always first branch wasn't covered. This is first part of fix. In subsequent PR I will make the code as skipped so it won't glow red.

@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen and removed clang Clang issues not falling into any other category labels Jan 10, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 10, 2024

@llvm/issue-subscribers-clang-frontend

Author: Paweł Bylica (chfast)

When a `if constexpr` has a block `{}` and the condition is false, the source-based code coverage always marks the opening `{` as not-executed region (see line 4).

Tested with clang 14.

    1|       |template &lt;int C&gt;
    2|      1|int test() {
    3|      1|    if constexpr (C == 0)
  ------------------
  |  Branch (3:19): [Folded - Ignored]
  ------------------
    4|      0|    {
    5|      1|        return 1;
    6|      1|    }
    7|      1|    return 0;
    8|      1|}
    9|       |
   10|       |int main()
   11|      1|{
   12|      1|    return test&lt;1&gt;();
   13|      1|}

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 10, 2024

@llvm/issue-subscribers-clang-codegen

Author: Paweł Bylica (chfast)

When a `if constexpr` has a block `{}` and the condition is false, the source-based code coverage always marks the opening `{` as not-executed region (see line 4).

Tested with clang 14.

    1|       |template &lt;int C&gt;
    2|      1|int test() {
    3|      1|    if constexpr (C == 0)
  ------------------
  |  Branch (3:19): [Folded - Ignored]
  ------------------
    4|      0|    {
    5|      1|        return 1;
    6|      1|    }
    7|      1|    return 0;
    8|      1|}
    9|       |
   10|       |int main()
   11|      1|{
   12|      1|    return test&lt;1&gt;();
   13|      1|}

@hanickadot
Copy link
Contributor

And I'm working on continuation of previous PR for if constexpr and if consteval to be properly skipped...
#78033

justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this issue Jan 28, 2024
…rt (llvm#77214)

Replace the discarded statement by an empty compound statement so we can keep track of the
whole source range we need to skip in coverage

Fixes llvm#54419
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang:frontend Language frontend issues, e.g. anything involving "Sema" coverage
Projects
None yet
Development

No branches or pull requests

5 participants