Page MenuHomePhabricator

[llvm-cov] reset executation count to 0 after wrapped segment
ClosedPublic

Authored by zequanwu on Jul 31 2020, 10:25 AM.

Diff Detail

Unit TestsFailed

TimeTest
30 mslinux > LLVM.tools/llvm-cov::cov-comdat.test
Script: -- : 'RUN: at line 8'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-cov show /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-cov/Inputs/binary-formats.v1.linux64l -instr-profile /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-cov/Inputs/elf_binary_comdat.profdata -path-equivalence=/tmp,/mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-cov/Inputs /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-cov/Inputs/instrprof-comdat.h -dump 2> /mnt/disks/ssd0/agent/llvm-project/build/test/tools/llvm-cov/Output/cov-comdat.test.tmp.err | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck --check-prefix=HEADER /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-cov/Inputs/instrprof-comdat.h
40 mslinux > LLVM.tools/llvm-cov::ignore-filename-regex.test
Script: -- : 'RUN: at line 5'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-cov report -instr-profile /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-cov/Inputs/sources_specified/main.profdata -path-equivalence=/tmp,/mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-cov/Inputs -ignore-filename-regex='.*\.h$' /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-cov/Inputs/sources_specified/main.covmapping | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck -check-prefix=REPORT_IGNORE_HEADERS /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-cov/ignore-filename-regex.test
0 mslinux > lld.ELF::whole-archive-name.s
Exception during script execution: OSError: [Errno 28] No space left on device
60 mswindows > Clang.OpenMP::declare_variant_device_isa_codegen_1.c
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w64\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -verify -fopenmp -x c -triple x86_64-pc-windows-gnu -emit-llvm C:\ws\w64\llvm-project\premerge-checks\clang\test\OpenMP\declare_variant_device_isa_codegen_1.c -o - -fopenmp-version=50 | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\clang\test\OpenMP\declare_variant_device_isa_codegen_1.c --check-prefix=GENERIC
50 mswindows > LLVM.tools/llvm-cov::cov-comdat.test
Script: -- : 'RUN: at line 8'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llvm-cov.exe show C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-cov/Inputs/binary-formats.v1.linux64l -instr-profile C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-cov/Inputs/elf_binary_comdat.profdata -path-equivalence=/tmp,C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-cov/Inputs C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-cov/Inputs/instrprof-comdat.h -dump 2> C:\ws\w64\llvm-project\premerge-checks\build\test\tools\llvm-cov\Output\cov-comdat.test.tmp.err | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe --check-prefix=HEADER C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-cov/Inputs/instrprof-comdat.h
View Full Test Results (6 Failed)

Event Timeline

zequanwu created this revision.Jul 31 2020, 10:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2020, 10:25 AM
zequanwu requested review of this revision.Jul 31 2020, 10:25 AM
zequanwu updated this revision to Diff 282324.Jul 31 2020, 3:00 PM

Update coverage unit test.
line 5 to line 9 should have executation count of 10.

zequanwu edited the summary of this revision. (Show Details)Jul 31 2020, 3:40 PM
vsk accepted this revision.Aug 4 2020, 4:42 PM

LGTM, thanks. The count from the wrapped segment shouldn't be preferred over the count from a region-entry segment that begins on some line.

This revision is now accepted and ready to land.Aug 4 2020, 4:42 PM
vsk added a comment.Aug 4 2020, 4:44 PM

@zequanwu it looks like the llvm-cov tests need to be updated to match this change.

zequanwu updated this revision to Diff 283069.Aug 4 2020, 5:20 PM

Update llvm-cov tests.

This revision was landed with ongoing or failed builds.Aug 4 2020, 6:39 PM
This revision was automatically updated to reflect the committed changes.
vsk added a comment.Aug 17 2020, 5:11 PM

Hi @zequanwu, we've started seeing a regression due to this change in swift (https://ci.swift.org/view/swift-master-next/job/oss-swift-incremental-RA-osx-master-next/7818/consoleText, see the bit about 'coverage_smoke.swift'). Here's the failure we see:

           165:  165| 2| throw CustomError.Err // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
check:167'0            X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
check:167'1                                                                          with "@LINE" equal to "167"
check:167'2                                                                   ?      possible intended match
           166:  166| 2| } catch {
check:167'0     ~~~~~~~~~~~~~~~~~~
           167:  167| 1| if b { // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
check:167'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           168:  168| 1| return 1 // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}1
check:167'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           169:  169| 1| }

The issue is that line 167 ("if b {") has an execution count of 1, even though it's actually reached twice. Before this change, the wrapped count from the catch block starting on line 166 would apply on line 167, all the way through the IfStmt condition (giving an execution count of 2). But with this change, seeing as the IfStmt condition doesn't get its own region, the line execution count gets picked up from the "Then" side of the IfStmt.

I'm concerned that this nested statements generated in clang as well as swift. Do you have some idea about how to address this?

I wonder whether we can make use of gap regions to fix the clang bugs you were looking at. I noticed that this fixed an issue in instrprof-comdat.h, but it's possible that that was caused by a bug in clang that has since been fixed (per the note).

In D85036#2222873, @vsk wrote:

Hi @zequanwu, we've started seeing a regression due to this change in swift (https://ci.swift.org/view/swift-master-next/job/oss-swift-incremental-RA-osx-master-next/7818/consoleText, see the bit about 'coverage_smoke.swift'). Here's the failure we see:

           165:  165| 2| throw CustomError.Err // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
check:167'0            X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
check:167'1                                                                          with "@LINE" equal to "167"
check:167'2                                                                   ?      possible intended match
           166:  166| 2| } catch {
check:167'0     ~~~~~~~~~~~~~~~~~~
           167:  167| 1| if b { // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
check:167'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           168:  168| 1| return 1 // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}1
check:167'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           169:  169| 1| }

The issue is that line 167 ("if b {") has an execution count of 1, even though it's actually reached twice. Before this change, the wrapped count from the catch block starting on line 166 would apply on line 167, all the way through the IfStmt condition (giving an execution count of 2). But with this change, seeing as the IfStmt condition doesn't get its own region, the line execution count gets picked up from the "Then" side of the IfStmt.

I'm concerned that this nested statements generated in clang as well as swift. Do you have some idea about how to address this?

I wonder whether we can make use of gap regions to fix the clang bugs you were looking at. I noticed that this fixed an issue in instrprof-comdat.h, but it's possible that that was caused by a bug in clang that has since been fixed (per the note).

Hi, I don't know how swift generate the regions. Shouldn't IfStmt gets its own regions covering from if keyword to thenStmt? Is that intended not to do so in swift?
My original thought about this is for one line if there is any new line segment which is start of region, we should ignore the wrapped line segment, and the new line segment count should be the accurate one. Do you have dumping result of segments (-debug-only=coverage-mapping) so we can take a deeper look?

vsk added a comment.Aug 18 2020, 10:32 AM
In D85036#2222873, @vsk wrote:

Hi @zequanwu, we've started seeing a regression due to this change in swift (https://ci.swift.org/view/swift-master-next/job/oss-swift-incremental-RA-osx-master-next/7818/consoleText, see the bit about 'coverage_smoke.swift'). Here's the failure we see:

           165:  165| 2| throw CustomError.Err // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
check:167'0            X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
check:167'1                                                                          with "@LINE" equal to "167"
check:167'2                                                                   ?      possible intended match
           166:  166| 2| } catch {
check:167'0     ~~~~~~~~~~~~~~~~~~
           167:  167| 1| if b { // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
check:167'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           168:  168| 1| return 1 // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}1
check:167'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           169:  169| 1| }

The issue is that line 167 ("if b {") has an execution count of 1, even though it's actually reached twice. Before this change, the wrapped count from the catch block starting on line 166 would apply on line 167, all the way through the IfStmt condition (giving an execution count of 2). But with this change, seeing as the IfStmt condition doesn't get its own region, the line execution count gets picked up from the "Then" side of the IfStmt.

I'm concerned that this nested statements generated in clang as well as swift. Do you have some idea about how to address this?

I wonder whether we can make use of gap regions to fix the clang bugs you were looking at. I noticed that this fixed an issue in instrprof-comdat.h, but it's possible that that was caused by a bug in clang that has since been fixed (per the note).

Hi, I don't know how swift generate the regions. Shouldn't IfStmt gets its own regions covering from if keyword to thenStmt? Is that intended not to do so in swift?

Thanks for taking a look. Clang and Swift both handle IfStmt in the same way, i.e. they both extend the parent region of the IfStmt through the condition. In clang, this is:

void VisitIfStmt(const IfStmt *S) {
  extendRegion(S);
  if (S->getInit())
    Visit(S->getInit());

  // Extend into the condition before we propagate through it below - this is
  // needed to handle macros that generate the "if" but not the condition.
  extendRegion(S->getCond());
  ...

I think this behavior is reasonable (it makes sense to say that the condition of an IfStmt is executed exactly as many times as the parent region containing the IfStmt).

My original thought about this is for one line if there is any new line segment which is start of region, we should ignore the wrapped line segment, and the new line segment count should be the accurate one. Do you have dumping result of segments (-debug-only=coverage-mapping) so we can take a deeper look?

Right, I thought I was on the same page as you about this. After paging a bit more of this in, I think a problem with ignoring the wrapped line segment is that it must have originated from a region that hasn't actually ended. If a line contains a new start-of-region line segment, it may well begin very late (e.g. at the end of the line), in which case it seems appropriate to consider using the count from the parent region (the wrapped count).

Here is the swift example in more detail:

func catchError2(_ b: Bool) -> Int {
  do {
    throw CustomError.Err // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
  } catch { // <<< Line 165 >>>
    if b {                // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
      return 1            // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}1
    }
  }
  return 0                // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}1
}
let _ = catchError2(true)
let _ = catchError2(false)
...
Counter in file 0 165:11 -> 169:4, #1
Counter in file 0 166:10 -> 168:6, #2
Counter in file 0 168:6 -> 169:4, (#1 - #2)
...
  163:6 -> 165:4 (count=2)
  165:11 -> 169:4 (count=2)
  166:10 -> 168:6 (count=1)
  168:6 -> 169:4 (count=1)

So, the "catch" on line 165 gets count=2. We then go to line 166 with wrapped-count=2. But we end up applying the start-of-region count on line 166, 166:10 -> 168:6 (count=1), which seems off.

In D85036#2224279, @vsk wrote:
In D85036#2222873, @vsk wrote:

Hi @zequanwu, we've started seeing a regression due to this change in swift (https://ci.swift.org/view/swift-master-next/job/oss-swift-incremental-RA-osx-master-next/7818/consoleText, see the bit about 'coverage_smoke.swift'). Here's the failure we see:

           165:  165| 2| throw CustomError.Err // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
check:167'0            X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
check:167'1                                                                          with "@LINE" equal to "167"
check:167'2                                                                   ?      possible intended match
           166:  166| 2| } catch {
check:167'0     ~~~~~~~~~~~~~~~~~~
           167:  167| 1| if b { // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
check:167'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           168:  168| 1| return 1 // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}1
check:167'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           169:  169| 1| }

The issue is that line 167 ("if b {") has an execution count of 1, even though it's actually reached twice. Before this change, the wrapped count from the catch block starting on line 166 would apply on line 167, all the way through the IfStmt condition (giving an execution count of 2). But with this change, seeing as the IfStmt condition doesn't get its own region, the line execution count gets picked up from the "Then" side of the IfStmt.

I'm concerned that this nested statements generated in clang as well as swift. Do you have some idea about how to address this?

I wonder whether we can make use of gap regions to fix the clang bugs you were looking at. I noticed that this fixed an issue in instrprof-comdat.h, but it's possible that that was caused by a bug in clang that has since been fixed (per the note).

Hi, I don't know how swift generate the regions. Shouldn't IfStmt gets its own regions covering from if keyword to thenStmt? Is that intended not to do so in swift?

Thanks for taking a look. Clang and Swift both handle IfStmt in the same way, i.e. they both extend the parent region of the IfStmt through the condition. In clang, this is:

void VisitIfStmt(const IfStmt *S) {
  extendRegion(S);
  if (S->getInit())
    Visit(S->getInit());

  // Extend into the condition before we propagate through it below - this is
  // needed to handle macros that generate the "if" but not the condition.
  extendRegion(S->getCond());
  ...

I think this behavior is reasonable (it makes sense to say that the condition of an IfStmt is executed exactly as many times as the parent region containing the IfStmt).

My original thought about this is for one line if there is any new line segment which is start of region, we should ignore the wrapped line segment, and the new line segment count should be the accurate one. Do you have dumping result of segments (-debug-only=coverage-mapping) so we can take a deeper look?

Right, I thought I was on the same page as you about this. After paging a bit more of this in, I think a problem with ignoring the wrapped line segment is that it must have originated from a region that hasn't actually ended. If a line contains a new start-of-region line segment, it may well begin very late (e.g. at the end of the line), in which case it seems appropriate to consider using the count from the parent region (the wrapped count).

Here is the swift example in more detail:

func catchError2(_ b: Bool) -> Int {
  do {
    throw CustomError.Err // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
  } catch { // <<< Line 165 >>>
    if b {                // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
      return 1            // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}1
    }
  }
  return 0                // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}1
}
let _ = catchError2(true)
let _ = catchError2(false)
...
Counter in file 0 165:11 -> 169:4, #1
Counter in file 0 166:10 -> 168:6, #2
Counter in file 0 168:6 -> 169:4, (#1 - #2)
...
  163:6 -> 165:4 (count=2)
  165:11 -> 169:4 (count=2)
  166:10 -> 168:6 (count=1)
  168:6 -> 169:4 (count=1)

So, the "catch" on line 165 gets count=2. We then go to line 166 with wrapped-count=2. But we end up applying the start-of-region count on line 166, 166:10 -> 168:6 (count=1), which seems off.

Is it because it's missing the code region for condition b? In clang, the condition will have a region with count 2. In the dumping result you given, I don't see that.

vsk added a comment.Aug 18 2020, 11:43 AM
In D85036#2224279, @vsk wrote:
In D85036#2222873, @vsk wrote:

Hi @zequanwu, we've started seeing a regression due to this change in swift (https://ci.swift.org/view/swift-master-next/job/oss-swift-incremental-RA-osx-master-next/7818/consoleText, see the bit about 'coverage_smoke.swift'). Here's the failure we see:

           165:  165| 2| throw CustomError.Err // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
check:167'0            X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
check:167'1                                                                          with "@LINE" equal to "167"
check:167'2                                                                   ?      possible intended match
           166:  166| 2| } catch {
check:167'0     ~~~~~~~~~~~~~~~~~~
           167:  167| 1| if b { // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
check:167'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           168:  168| 1| return 1 // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}1
check:167'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           169:  169| 1| }

The issue is that line 167 ("if b {") has an execution count of 1, even though it's actually reached twice. Before this change, the wrapped count from the catch block starting on line 166 would apply on line 167, all the way through the IfStmt condition (giving an execution count of 2). But with this change, seeing as the IfStmt condition doesn't get its own region, the line execution count gets picked up from the "Then" side of the IfStmt.

I'm concerned that this nested statements generated in clang as well as swift. Do you have some idea about how to address this?

I wonder whether we can make use of gap regions to fix the clang bugs you were looking at. I noticed that this fixed an issue in instrprof-comdat.h, but it's possible that that was caused by a bug in clang that has since been fixed (per the note).

Hi, I don't know how swift generate the regions. Shouldn't IfStmt gets its own regions covering from if keyword to thenStmt? Is that intended not to do so in swift?

Thanks for taking a look. Clang and Swift both handle IfStmt in the same way, i.e. they both extend the parent region of the IfStmt through the condition. In clang, this is:

void VisitIfStmt(const IfStmt *S) {
  extendRegion(S);
  if (S->getInit())
    Visit(S->getInit());

  // Extend into the condition before we propagate through it below - this is
  // needed to handle macros that generate the "if" but not the condition.
  extendRegion(S->getCond());
  ...

I think this behavior is reasonable (it makes sense to say that the condition of an IfStmt is executed exactly as many times as the parent region containing the IfStmt).

My original thought about this is for one line if there is any new line segment which is start of region, we should ignore the wrapped line segment, and the new line segment count should be the accurate one. Do you have dumping result of segments (-debug-only=coverage-mapping) so we can take a deeper look?

Right, I thought I was on the same page as you about this. After paging a bit more of this in, I think a problem with ignoring the wrapped line segment is that it must have originated from a region that hasn't actually ended. If a line contains a new start-of-region line segment, it may well begin very late (e.g. at the end of the line), in which case it seems appropriate to consider using the count from the parent region (the wrapped count).

Here is the swift example in more detail:

func catchError2(_ b: Bool) -> Int {
  do {
    throw CustomError.Err // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
  } catch { // <<< Line 165 >>>
    if b {                // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
      return 1            // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}1
    }
  }
  return 0                // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}1
}
let _ = catchError2(true)
let _ = catchError2(false)
...
Counter in file 0 165:11 -> 169:4, #1
Counter in file 0 166:10 -> 168:6, #2
Counter in file 0 168:6 -> 169:4, (#1 - #2)
...
  163:6 -> 165:4 (count=2)
  165:11 -> 169:4 (count=2)
  166:10 -> 168:6 (count=1)
  168:6 -> 169:4 (count=1)

So, the "catch" on line 165 gets count=2. We then go to line 166 with wrapped-count=2. But we end up applying the start-of-region count on line 166, 166:10 -> 168:6 (count=1), which seems off.

Is it because it's missing the code region for condition b? In clang, the condition will have a region with count 2. In the dumping result you given, I don't see that.

I think that explains the difference. Thanks for pointing that out!

zequanwu added a comment.EditedFeb 8 2021, 12:13 PM

This fix to the three bugs is not correct. It's a workaround on llvm-cov side to accommodate clang frontend bugs. It results unexpected effect discovered by Rust coverage (example: https://reviews.llvm.org/D95987#2541091). We need to fix those bugs on clang coverage side rather than llvm-cov.

Should I revert it now?

vsk added a comment.Feb 8 2021, 4:13 PM

IIUC, this change makes it so that the wrapped segment count isn't used on a line that starts a new region. I suppose we might prefer the wrapped count in some cases, e.g. in:

1| if (..) {
2|   a ? b : c; // "b" is a new region, but the wrapped count from line 1 is better for line 2 than the count from the "b" region.

In my comment from Aug. 2020, I think I mistakenly assumed that there'd be some region-entry segment on a line matching the wrapped count.

I think it makes sense to revert, assuming the clang frontend bugs are fixed? It may be necessary to regenerate the .covmapping file for the instrprof-comdat.h test.

I think it makes sense to revert, assuming the clang frontend bugs are fixed? It may be necessary to regenerate the .covmapping file for the instrprof-comdat.h test.

No fixes on clang coverage frontend yet :(. Reverting this will reintroduce the three bugs, and that's what I am concerned. Or revert it only after fixing it on clang coverage?

BTW, what's your opinion on the coverage bug (introduced by this change) on the description of https://reviews.llvm.org/D95987. Should line 5 be marked as executed in line coverage? I think it should.

vsk added a comment.Feb 9 2021, 8:32 AM

I think it makes sense to revert, assuming the clang frontend bugs are fixed? It may be necessary to regenerate the .covmapping file for the instrprof-comdat.h test.

No fixes on clang coverage frontend yet :(. Reverting this will reintroduce the three bugs, and that's what I am concerned. Or revert it only after fixing it on clang coverage?

I see. Usually the policy is to not knowingly regress functionality. In this case I think there's more than one reasonable way to handle things though (seeing as D85036 seems to have caused some issues of its own). Happy to defer to your preference - if you revert this before getting around to the clang frontend bugs, please update the referenced bugzilla entries.

BTW, what's your opinion on the coverage bug (introduced by this change) on the description of https://reviews.llvm.org/D95987. Should line 5 be marked as executed in line coverage? I think it should.

I agree.

nikic added a subscriber: nikic.Feb 20 2021, 11:37 AM

I've filed https://bugs.llvm.org/show_bug.cgi?id=49297 to track this issue as a release blocker for LLVM 12.