[PATCH] D138846: MC/DC in LLVM Source-Based Code Coverage: LLVM back-end and compiler-rt

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

In D138846#4657193 , @zequanwu wrote:

> In D138846#4657175 , @hans wrote:
>
>> We're seeing crashes in `initializeValueProfRuntimeRecord` that bisects to 
>> this commit. I think Zequan is investigating: 
>> https://bugs.chromium.org/p/chromium/issues/detail?id=1503919
>
> It turns out to be that our rust code with old version of rustc without this 
> change, so mixture of raw profile versions are used, causing segment fault.

Thank you for the update! Is this also the case of the issue @glandium reports 
above as well?  I think both issues manifested as a ValueProf issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138846/new/

https://reviews.llvm.org/D138846

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138846: MC/DC in LLVM Source-Based Code Coverage: LLVM back-end and compiler-rt

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

In D138846#4657152 , @glandium wrote:

> In D138846#4656607 , @glandium 
> wrote:
>
>> Code built with older versions of LLVM (e.g. rust) and running with the 
>> updated runtime crash at startup with this change.
>
> FWIW, neither followups fixed this issue. The crash happens in 
> __llvm_profile_instrument_target.

Hmm, then it's different from the repro given earlier.  Can you provide a test 
case?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138846/new/

https://reviews.llvm.org/D138846

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138846: MC/DC in LLVM Source-Based Code Coverage: LLVM back-end and compiler-rt

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

In D138846#4656594 , @hans wrote:

> Just a heads up that we're seeing crashes in the Rust compiler which bisects 
> to this change. Investigating in 
> https://bugs.chromium.org/p/chromium/issues/detail?id=1500558

Thank you for the repro!  This was a very straightforward fix; effectively the 
inlining of an instrumented function into multiple functions led to the 
creation of duplicate data variables, ultimately leading to a segfault in 
emitNamedata() when eraseFromParent() was called multiple times from the same 
NamePtr.  Prior to my change to abstract the creation of the data variable, 
there was no explicit check for this, but it was inadvertently avoided by 
checking that multiple counter sections were not created.

I created a pull request for the fix here: 
https://github.com/llvm/llvm-project/pull/71998


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138846/new/

https://reviews.llvm.org/D138846

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138846: MC/DC in LLVM Source-Based Code Coverage: LLVM back-end and compiler-rt

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

Hi @hans @w2yehia  Please let me know if you have any additional concerns.  I'd 
like to reland this within the next day or so. Thank you!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138846/new/

https://reviews.llvm.org/D138846

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138846: MC/DC in LLVM Source-Based Code Coverage: LLVM back-end and compiler-rt

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

In D138846#4649609 , @w2yehia wrote:

> Please update InstrProfilingPlatformAIX.c as well, specifically add new dummy 
> vars for the new section.
> Edit: I can post the patch if you wish.

I just added the dummy vars, but you can have a look.  You're welcome to post 
the patch too and it would be great if you could assist in testing for AIX.   
Thank you!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138846/new/

https://reviews.llvm.org/D138846

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138846: MC/DC in LLVM Source-Based Code Coverage: LLVM back-end and compiler-rt

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

> I added steps to download the profile here: 
> https://bugs.chromium.org/p/chromium/issues/detail?id=1485303#c4 
> I think this should be reverted while being investigated: 
> https://github.com/llvm/llvm-project/commit/53a2923bf67bc164558d489493176630123abf7e

Thank you for the repro! It was a huge help. There was in fact a bug in 
InstrProfReader.cpp where the wrong profile format version was being checked 
before attempting to read MC/DC bitmap bytes.  The check was added to ensure 
backward compatibility with older versions.  I fixed that check and added a 
testcase to ensure v10 of the format can still be handled successfully.

> I just noticed this also broke some lit tests on mac: 
> https://bugs.chromium.org/p/chromium/issues/detail?id=1485487#c0
> That's also visible on greendragon: 
> https://green.lab.llvm.org/green/view/Clang/job/clang-stage1-RA/35721/testReport/

The Mac failures were due a missing adjustment in clang to ensure profile 
sections are properly page aligned.  When I separated out the patches to make 
the process easier, that change ended up in https://reviews.llvm.org/D138849 
with the other MC/DC clang changes when it should've been included in this 
patch.  I've added it and verified that the Mac tests pass with this patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138846/new/

https://reviews.llvm.org/D138846

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138846: MC/DC in LLVM Source-Based Code Coverage: LLVM back-end and compiler-rt

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

In D138846#4649110 , @akhuang wrote:

> I'm still working on a repro, but after this patch we're seeing "truncated 
> profile data" errors in chromium (crbug.com/1485303)

Thank you for the heads-up! If there's an issue, I'm eager to ensure it is 
addressed. Your repro will be helpful.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138846/new/

https://reviews.llvm.org/D138846

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136385: Add support for MC/DC in LLVM Source-Based Code Coverage

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

Thank you for the comments thus-far.  I have split this patch into three 
stacked patches and updated them according to comments found on this review:

Clang Front-end: https://reviews.llvm.org/D138849
LLVM back-end/compiler-rt: https://reviews.llvm.org/D138846
llvm-cov visualization/evaluation: https://reviews.llvm.org/D138847


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136385/new/

https://reviews.llvm.org/D136385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136385: Add support for MC/DC in LLVM Source-Based Code Coverage

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

Thank you (both) for looking at it! I appreciate your time.

In D136385#3891311 , @peter.smith 
wrote:

> I suspect that this patch will need to get split up into smaller parts. While 
> it is very useful to see everything at once, it may be better to use this as 
> a kind of support for the RFC, and then split off the implementation into 
> smaller independently reviewable parts, even if the full functionality isn't 
> useable until the last patch lands. For example the LLVM changes could be 
> looked at and accepted prior to clang.

I agree -- I will explore splitting the review up, probably next week.  And 
I'll review the suggested adjustments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136385/new/

https://reviews.llvm.org/D136385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130786: [clang-repl] Disable execution unittests on unsupported platforms.

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

In D130786#3688503 , @sunho wrote:

> @alanphipps I just confirmed that the buildbot failures were fixed by the new 
> fix https://reviews.llvm.org/rG65c9265f4158. Could you check out if this 
> fixes the failure on your end too?

Yes it appears to be passing now. Thank you!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130786/new/

https://reviews.llvm.org/D130786

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130786: [clang-repl] Disable execution unittests on unsupported platforms.

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

I'm still seeing a failure on my downstream Arm compiler on Linux in the unit 
tests -- I thought I saw the same failure on the buildbots:

  FAIL: llvm_regressions :: Clang-Unit/Interpreter/ClangReplInterpreterTests/1/8
  3 

  4 Script(shard):
  5 --
  6 
GTEST_OUTPUT=json:/scratch/build_jenkins/workspace/Downstream_Changes/llvm_cgt/arm-llvm/RelWithAsserts/llvm/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests-Clang-Unit-39149-1-8.json
 GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=8 GTEST_SHARD_INDEX=1 
/scratch/build_jenkins/workspace/Downstream_Changes/llvm_cgt/arm-llvm/RelWithAsserts/llvm/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests
  7 --
  8  
  9 Note: This is test shard 2 of 8.
  10[==] Running 1 test from 1 test suite.
  11[--] Global test environment set-up.
  12[--] 1 test from IncrementalProcessing
  13[ RUN  ] IncrementalProcessing.FindMangledNameSymbol
  14 #0 0x00676fc3 
llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) 
(/scratch/build_jenkins/workspace/Downstream_Changes/llvm_cgt/arm-llvm/RelWithAsserts/llvm/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests+0x676fc3)
  15 #1 0x00674e1c llvm::sys::RunSignalHandlers() 
(/scratch/build_jenkins/workspace/Downstream_Changes/llvm_cgt/arm-llvm/RelWithAsserts/llvm/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests+0x674e1c)
  16 #2 0x006775c6 SignalHandler(int) 
(/scratch/build_jenkins/workspace/Downstream_Changes/llvm_cgt/arm-llvm/RelWithAsserts/llvm/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests+0x6775c6)
  17 #3 0x7fa295c6f330 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x10330)
  18 #4 0x00b54a97 clang::IncrementalExecutor::cleanUp() 
(/scratch/build_jenkins/workspace/Downstream_Changes/llvm_cgt/arm-llvm/RelWithAsserts/llvm/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests+0xb54a97)
  19 #5 0x00b521e4 clang::Interpreter::~Interpreter() 
(/scratch/build_jenkins/workspace/Downstream_Changes/llvm_cgt/arm-llvm/RelWithAsserts/llvm/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests+0xb521e4)
  20 #6 0x00489a13 (anonymous 
namespace)::IncrementalProcessing_FindMangledNameSymbol_Test::TestBody() 
(/scratch/build_jenkins/workspace/Downstream_Changes/llvm_cgt/arm-llvm/RelWithAsserts/llvm/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests+0x489a13)
  21 #7 0x0068889c testing::Test::Run() 
(/scratch/build_jenkins/workspace/Downstream_Changes/llvm_cgt/arm-llvm/RelWithAsserts/llvm/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests+0x68889c)
  22 #8 0x006895c9 testing::TestInfo::Run() 
(/scratch/build_jenkins/workspace/Downstream_Changes/llvm_cgt/arm-llvm/RelWithAsserts/llvm/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests+0x6895c9)
  23 #9 0x00689c77 testing::TestSuite::Run() 
(/scratch/build_jenkins/workspace/Downstream_Changes/llvm_cgt/arm-llvm/RelWithAsserts/llvm/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests+0x689c77)
  24#10 0x00699647 
testing::internal::UnitTestImpl::RunAllTests() 
(/scratch/build_jenkins/workspace/Downstream_Changes/llvm_cgt/arm-llvm/RelWithAsserts/llvm/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests+0x699647)
  25#11 0x00698ec3 testing::UnitTest::Run() 
(/scratch/build_jenkins/workspace/Downstream_Changes/llvm_cgt/arm-llvm/RelWithAsserts/llvm/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests+0x698ec3)
  26#12 0x00680fec main 
(/scratch/build_jenkins/workspace/Downstream_Changes/llvm_cgt/arm-llvm/RelWithAsserts/llvm/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests+0x680fec)
  27#13 0x7fa294d64f45 __libc_start_main 
/build/eglibc-xkFqqE/eglibc-2.19/csu/libc-start.c:321:0
  28#14 0x0047fff5 _start 
(/scratch/build_jenkins/workspace/Downstream_Changes/llvm_cgt/arm-llvm/RelWithAsserts/llvm/tools/clang/unittests/Interpreter/./ClangReplInterpreterTests+0x47fff5)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130786/new/

https://reviews.llvm.org/D130786

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121997: [clang][driver] Fix compilation database dump with multiple architectures

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

Should there be an aarch64 guard on the test? There exist downstream Arm 
compilers that don't support Arm64 and fail.  I know other tests explicitly 
state "REQUIRES: aarch64-registered-target"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121997/new/

https://reviews.llvm.org/D121997

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119136: [clang] Implement Change scope of lambda trailing-return-type

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

We've also been seeing failures in our downstream Arm compiler when running the 
Perennial C++14 compliance tests related to this change.  Specifically, the 
tests expect a diagnostic to be issued when the lambda capture variable is 
outside of the lambda's {} scope. Another tests uses the lambda capture 
variable in a decltype-style return type which is also outside of the scope.

Do these failures sound like what others have run into?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119136/new/

https://reviews.llvm.org/D119136

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96280: [clang][cli] Round-trip the whole CompilerInvocation

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

In D96280#2611651 , @alanphipps wrote:

> We also encounter a build failure on the Mac related to above but in a 
> different file:
> clang/lib/CodeGen/CGStmtOpenMP.cpp:1916:3: error: too few template arguments 
> for class template 'SmallVector'
> SmallVector EffectiveArgs;
> ^
> clang/include/clang/Basic/LLVM.h:35:42: note: template is declared here
> template class SmallVector;

Disregard -- my issue is related to another commit and is addressed by 
https://reviews.llvm.org/D98265

In D96280#2611651 , @alanphipps wrote:

> We also encounter a build failure on the Mac related to above but in a 
> different file:
> clang/lib/CodeGen/CGStmtOpenMP.cpp:1916:3: error: too few template arguments 
> for class template 'SmallVector'
> SmallVector EffectiveArgs;
> ^
> clang/include/clang/Basic/LLVM.h:35:42: note: template is declared here
> template class SmallVector;




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96280/new/

https://reviews.llvm.org/D96280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94973: [clang][OpenMP] Use OpenMPIRBuilder for workshare loops.

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

In D94973#2617304 , @Meinersbur wrote:

> Is is that same problem that D98265  
> addresses?

Yes, that appears to be the same.  Thank you!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94973/new/

https://reviews.llvm.org/D94973

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94973: [clang][OpenMP] Use OpenMPIRBuilder for workshare loops.

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

Hello! I am seeing a downstream build failure on Mac as a result of your 
changes:

  clang/lib/CodeGen/CGStmtOpenMP.cpp:1916:3: error: too few template arguments 
for class template 'SmallVector'
  SmallVector EffectiveArgs;
  ^
  clang/include/clang/Basic/LLVM.h:35:42: note: template is declared here
  template class SmallVector;


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94973/new/

https://reviews.llvm.org/D94973

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96280: [clang][cli] Round-trip the whole CompilerInvocation

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

We also encounter a build failure on the Mac related to above but in a 
different file:
clang/lib/CodeGen/CGStmtOpenMP.cpp:1916:3: error: too few template arguments 
for class template 'SmallVector'
SmallVector EffectiveArgs;
^
clang/include/clang/Basic/LLVM.h:35:42: note: template is declared here
template class SmallVector;


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96280/new/

https://reviews.llvm.org/D96280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage

[Alan Phipps via Phabricator via cfe-commits]

alanphipps marked an inline comment as done.
alanphipps added inline comments.



Comment at: llvm/test/tools/llvm-cov/branch-noShowBranch.test:3
+// RUN: llvm-profdata merge %S/Inputs/branch-c-general.proftext -o %t.profdata
+// RUN: llvm-cov show %S/Inputs/branch-c-general.o32l -instr-profile 
%t.profdata -path-equivalence=/tmp,%S %S/branch-c-general.c | FileCheck %s
+// RUN: llvm-cov report %S/Inputs/branch-c-general.o32l 
--show-branch-summary=false -instr-profile %t.profdata -show-functions 
-path-equivalence=/tmp,%S %S/branch-c-general.c | FileCheck %s 
-check-prefix=REPORT

dblaikie wrote:
> alanphipps wrote:
> > dblaikie wrote:
> > > This test depends on another test file as input which is generally not 
> > > done in the LLVM test suite - inputs to tests are placed in the "Inputs" 
> > > subdirectory, rather than in the test directory itself. I realize a few 
> > > other tests already here (demangle, hideUnexpectedSubviews, style, and 
> > > threads) already do this - but it'd be good to not add more cases (& fix 
> > > those existing cases where possible)
> > > 
> > > Could you fix this so this test (& possibly others, if you have the 
> > > opportunity/bandwidth) doesn't depend on other test files, but only on 
> > > files in the Inputs subdirectory?
> > Certainly I can do that -- I believe there are two other tests in which I 
> > did this other than this test: branch-export-json.txt and 
> > branch-export-lcov.test.  I will adjust those tests that I added within the 
> > next days but I don't think I have the bandwidth to change other tests that 
> > also do this.
> > 
> > 
> Sure thing, thanks a bunch!
> 
> At least having fewer instances of the unfavorable idioms means they're less 
> likely to be repeated in the future, even if there are some leftover 
> instances/anachronisms.
Changes have been committed here: https://reviews.llvm.org/rGebcc8dcb68aa
Thanks for pointing this out!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84467/new/

https://reviews.llvm.org/D84467

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage

[Alan Phipps via Phabricator via cfe-commits]

alanphipps marked an inline comment as done.
alanphipps added inline comments.



Comment at: llvm/test/tools/llvm-cov/branch-noShowBranch.test:3
+// RUN: llvm-profdata merge %S/Inputs/branch-c-general.proftext -o %t.profdata
+// RUN: llvm-cov show %S/Inputs/branch-c-general.o32l -instr-profile 
%t.profdata -path-equivalence=/tmp,%S %S/branch-c-general.c | FileCheck %s
+// RUN: llvm-cov report %S/Inputs/branch-c-general.o32l 
--show-branch-summary=false -instr-profile %t.profdata -show-functions 
-path-equivalence=/tmp,%S %S/branch-c-general.c | FileCheck %s 
-check-prefix=REPORT

dblaikie wrote:
> This test depends on another test file as input which is generally not done 
> in the LLVM test suite - inputs to tests are placed in the "Inputs" 
> subdirectory, rather than in the test directory itself. I realize a few other 
> tests already here (demangle, hideUnexpectedSubviews, style, and threads) 
> already do this - but it'd be good to not add more cases (& fix those 
> existing cases where possible)
> 
> Could you fix this so this test (& possibly others, if you have the 
> opportunity/bandwidth) doesn't depend on other test files, but only on files 
> in the Inputs subdirectory?
Certainly I can do that -- I believe there are two other tests in which I did 
this other than this test: branch-export-json.txt and branch-export-lcov.test.  
I will adjust those tests that I added within the next days but I don't think I 
have the bandwidth to change other tests that also do this.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84467/new/

https://reviews.llvm.org/D84467

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

In D84467#2449045 , @vsk wrote:

> Unfortunately I don't think I'll be able to commit this on your behalf. Would 
> you be open to obtaining commit access from Chris and landing this directly? 
> I ask because, for a patch of this size, there's typically a fair amount of 
> post-commit feedback (primarily in the form of bot failure notifications, but 
> occasionally design feedback as well). It's common for tests to pass locally, 
> but not on some specific CI node, and it can take quite a bit of time to 
> resolve those issues.

Sure, that's reasonable, thanks.  I'll give that a shot.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84467/new/

https://reviews.llvm.org/D84467

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

In D84467#2423636 , @vsk wrote:

> Thank you, lgtm!

Thank you! I also would like to ask if you could commit it for me as I don't 
have access for that.  Note that a handful of tests have binary files which 
were not uploaded with the diff.  What do you advise I do to get those up here 
so that I can commit it (unfortunately my system doesn't have arcanist setup 
and that would take some time).  Thanks again for all of your help!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84467/new/

https://reviews.llvm.org/D84467

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91747: [Clang] Add __STDCPP_THREADS__ to standard predefine macros

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

Looks like _LIBCPP_HAS_NO_THREADS is being set for libcxxabi, and the build now 
fails with this change:

llvm-project/libcxxabi/../libcxx/include/__config:1172:2: error: 
_LIBCPP_HAS_NO_THREADS cannot be set when __STDCPP_THREADS__ is set


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91747/new/

https://reviews.llvm.org/D91747

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage

[Alan Phipps via Phabricator via cfe-commits]

alanphipps marked 2 inline comments as done.
alanphipps added inline comments.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1169
+createBranchRegion(S->getCond(), BodyCount,
+   subtractCounters(CondCount, BodyCount));
   }

vsk wrote:
> If `theWhileStmt->getCond()` is-a BinaryOperator, it would not be considered 
> an instrumented condition and no branch region would be created here. OTOH, 
> if the condition is instrumented (e.g. as it would be in `while (x);`), a 
> branch region would be created.
> 
> Is this the expected outcome? It seems a little bit inconsistent compared to 
> either a) allowing the RecursiveASTVisitor to identify expressions that 
> require branch regions, or b) guaranteeing that each while statement comes 
> with a branch region for its condition.
> 
> I have the same question about the `createBranchRegion` calls in VisitDoStmt, 
> VisitForStmt, etc. (But I'm not concerned about the calls to 
> `createBranchRegion` in VisitBinL{And,Or} and VisitSwitch*.)
Right, that's the expected outcome, but I think I see what you're saying in 
that it could be confusing to call "createBranchRegion()" that may not actually 
create a branch region in some cases.  

I could move the check for isInstrumentedCondition() out of 
createBranchRegion() to make it clear that a branch region is only created for 
when that is TRUE, but I wanted to encapsulate as much as I could in the 
function to avoid duplication.  Perhaps it would be better to rename 
"createBranchRegion()" to something more like 
"createBranchRegionForInstrumentedCond()"?

The same behavior exists for VisitBinL{And,Or} since those conditions could 
also be nested BinaryOperators.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1277
+// Create Branch Region around condition.
+createBranchRegion(S->getCond(), BodyCount,
+   subtractCounters(LoopCount, BodyCount));

vsk wrote:
> Is the condition of a CXXForRangeStmt something that's visible to the user? 
> It looks more like a clang implementation detail (but I could be mistaken).
Technically the condition isn't visible, per se, but the range-based construct 
implies a branchable condition that other vendors do track, so I still think 
it's useful to show it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84467/new/

https://reviews.llvm.org/D84467

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

In D84467#2264205 , @vsk wrote:

> @alanphipps thanks for bearing with me. I think this is about ready to land. 
> I do ask that you back out any punctuation/whitespace changes in code/tests 
> that aren't directly modified in your diff (the clang-format-diff script can 
> help with this). It looks like some libCoverage and test changes are no 
> longer in the current version of this patch, as compared to the previous 
> version (https://reviews.llvm.org/D84467?id=287269). Was that intentional?

I apologize for the long delay myself -- I think I discovered a bug in the code 
that I'm trying to work out.  In answer to your question about the patches, no 
it wasn't intentional; I believe I produced the patch incorrectly, only 
capturing the most recent changes.  I'll update this soon.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84467/new/

https://reviews.llvm.org/D84467

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage

[Alan Phipps via Phabricator via cfe-commits]

alanphipps updated this revision to Diff 290301.
alanphipps added a comment.

- Rename isLeafCondition() to isInstrumentedCondition() and rephrase associated 
comments
- Add branch regions on C++ range-based loops


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84467/new/

https://reviews.llvm.org/D84467

Files:
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenPGO.cpp
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/loops.cpp
  clang/test/CoverageMapping/macro-expressions.cpp

Index: clang/test/CoverageMapping/macro-expressions.cpp
===
--- clang/test/CoverageMapping/macro-expressions.cpp
+++ clang/test/CoverageMapping/macro-expressions.cpp
@@ -79,7 +79,8 @@
   // CHECK-NEXT: File 0, [[@LINE+2]]:6 -> [[@LINE+2]]:8 = (#0 + #6)
   // CHECK-NEXT: Expansion,File 0, [[@LINE+1]]:16 -> [[@LINE+1]]:21 = (#0 + #6)
   do {} while (NEXPR(i));
-  // CHECK-NEXT: Expansion,File 0, [[@LINE+3]]:8 -> [[@LINE+3]]:12 = #0
+  // CHECK-NEXT: Expansion,File 0, [[@LINE+4]]:8 -> [[@LINE+4]]:12 = #0
+  // CHECK-NEXT: Branch,File 0, [[@LINE+3]]:21 -> [[@LINE+3]]:22 = #7, #0
   // CHECK-NEXT: Expansion,File 0, [[@LINE+2]]:23 -> [[@LINE+2]]:26 = #0
   // CHECK: File 0, [[@LINE+1]]:42 -> [[@LINE+1]]:44 = #7
   for (DECL(int, j) : ARR(int, 1, 2, 3)) {}
Index: clang/test/CoverageMapping/loops.cpp
===
--- clang/test/CoverageMapping/loops.cpp
+++ clang/test/CoverageMapping/loops.cpp
@@ -4,7 +4,9 @@
 // CHECK: rangedFor
 void rangedFor() {  // CHECK-NEXT: File 0, [[@LINE]]:18 -> {{[0-9]+}}:2 = #0
   int arr[] = { 1, 2, 3, 4, 5 };
-  int sum = 0;  // CHECK: Gap,File 0, [[@LINE+1]]:20 -> [[@LINE+1]]:21 = #1
+  int sum = 0;  
+// CHECK-NEXT: Branch,File 0, [[@LINE+2]]:14 -> [[@LINE+2]]:15 = #1, (#0 - #3) 
+// CHECK: Gap,File 0, [[@LINE+1]]:20 -> [[@LINE+1]]:21 = #1
   for(auto i : arr) {   // CHECK: File 0, [[@LINE]]:21 -> [[@LINE+6]]:4 = #1
 if (i == 3)
   continue; // CHECK: File 0, [[@LINE]]:7 -> [[@LINE]]:15 = #2
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -792,20 +792,20 @@
 return (Cond->EvaluateAsInt(Result, CVM.getCodeGenModule().getContext()));
   }
 
-  /// Create a Branch Region around a leaf-level condition for code coverage
+  /// Create a Branch Region around an instrumentable condition for coverage
   /// and add it to the function's SourceRegions.  A branch region tracks a
-  /// "True" counter and a "False" counter for all leaf-level boolean
-  /// expressions that result in the generation of a branch.
+  /// "True" counter and a "False" counter for boolean expressions that
+  /// result in the generation of a branch.
   void createBranchRegion(const Expr *C, Counter TrueCnt, Counter FalseCnt) {
 // Check for NULL conditions.
 if (!C)
   return;
 
-// Ensure we are a leaf-level condition (i.e. no "&&" or "||").  Push
+// Ensure we are an instrumentable condition (i.e. no "&&" or "||").  Push
 // region onto RegionStack but immediately pop it (which adds it to the
 // function's SourceRegions) because it doesn't apply to any other source
 // code other than the Condition.
-if (CodeGenFunction::isLeafCondition(C)) {
+if (CodeGenFunction::isInstrumentedCondition(C)) {
   // If a condition can fold to true or false, the corresponding branch
   // will be removed.  Create a region with both counters hard-coded to
   // zero. This allows us to visualize them in a special way.
@@ -1250,6 +1250,10 @@
 addCounters(BC.BreakCount, subtractCounters(LoopCount, BodyCount));
 if (OutCount != ParentCount)
   pushRegion(OutCount);
+
+// Create Branch Region around condition.
+createBranchRegion(S->getCond(), BodyCount,
+   subtractCounters(LoopCount, BodyCount));
   }
 
   void VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S) {
Index: clang/lib/CodeGen/CodeGenPGO.cpp
===
--- clang/lib/CodeGen/CodeGenPGO.cpp
+++ clang/lib/CodeGen/CodeGenPGO.cpp
@@ -212,7 +212,8 @@
   /// version so that we facilitate backward compatibility.
   bool VisitBinaryOperator(BinaryOperator *S) {
 if (ProfileVersion >= llvm::IndexedInstrProf::Version7)
-  if (S->isLogicalOp() && CodeGenFunction::isLeafCondition(S->getRHS()))
+  if (S->isLogicalOp() &&
+  CodeGenFunction::isInstrumentedCondition(S->getRHS()))
 CounterMap[S->getRHS()] = NextCounter++;
 return Base::VisitBinaryOperator(S);
   }
I

[PATCH] D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage

[Alan Phipps via Phabricator via cfe-commits]

alanphipps marked an inline comment as done.
alanphipps added inline comments.



Comment at: clang/lib/CodeGen/CodeGenFunction.h:4359
+  /// condition (i.e. no "&&" or "||").
+  static bool isLeafCondition(const Expr *C);
+

vsk wrote:
> alanphipps wrote:
> > vsk wrote:
> > > alanphipps wrote:
> > > > vsk wrote:
> > > > > vsk wrote:
> > > > > > It might be helpful to either require that `C` be the RHS of a 
> > > > > > logical binop (e.g. by passing the binop expr in), or to document 
> > > > > > that requirement.
> > > > > Given a logical binop like `E = a && !(b || c)`, 
> > > > > `isLeafCondition(E->getRHS())` is true. That seems a bit 
> > > > > counter-intuitive, because `E->getRHS()` contains another leaf 
> > > > > condition. Would it make sense to rename the condition (perhaps to 
> > > > > something like 'InstrumentedCondition')? Have I misunderstood what a 
> > > > > leaf condition is?
> > > > Background: isLeafCondition() is an auxiliary routine that is used 
> > > > during 1.) counter allocation on binop RHS (CodeGenPGO.cpp), 2.) 
> > > > counter instrumentation (CodeGenFunction.cpp), and 3.) branch region 
> > > > generation (CoverageMappingGen.cpp).  In the #3 case, it isn't always 
> > > > looking at the RHS of a logical binop but can be used to assess whether 
> > > > any condition is instrumented/evaluated.
> > > > 
> > > > Given your example condition:
> > > > 
> > > > E = a && !(b || c)
> > > > 
> > > > You are correct that isLeafCondition(E->getRHS()) will return true, but 
> > > > I think it should actually return false here (and bypass logical-NOT), 
> > > > so I will adapt this. 
> > > > 
> > > > However, given a condition that includes an binary operator (like 
> > > > assign):
> > > > 
> > > > E = a && (x = !(b || c))
> > > > 
> > > > isLeafCondition(E->getRHS()) will also return true, and I think that is 
> > > > the correct behavior.  Given that, then I agree that maybe 
> > > > isLeafCondition() should be renamed to isInstrumentedCondition() since 
> > > > it's not technically just leaves that are tracked in the presence of 
> > > > operators.
> > > What is special about the assignment expression nested within "a && (x = 
> > > !(b || c))" that necessitates an extra counter compared to "a && !(b || 
> > > c)"?
> > I'm exempting the logical NOT operator basically to match the functionality 
> > of other coverage vendors (Bullseye, GCOV).  It's a simplistic operator in 
> > the sense that (as far as I can tell) it only affects the sense of the 
> > generated branch on a condition.
> > 
> > As for the assignment, we're effectively creating a new condition that is 
> > evaluatable (even if a branch may technically not be generated), and so 
> > creating a counter for it is more interesting (and matches other vendor 
> > behavior).
> > 
> > But I'm open to persuasion.  We could just be more conservative and create 
> > counters for logical NOT, but I think there's value in matching some of the 
> > expected behavior of other vendors.  This is also something we could 
> > continue to fine-tune in future patches.
> I agree that there isn't a need to insert a fresh counter to accommodate a 
> logical NOT operator in a condition. But I don't see how an assignment 
> expression effectively creates a new, evaluatable condition. It seems like 
> the count for the assignment expr can be derived by looking up the count for 
> its parent region.
> 
> At this stage I'm more interested in understanding the high-level design. If 
> the question of whether/not to add fresh counters for assignment exprs in a 
> condition is effectively about optimization, then I'm fine with tabling it.
I just realized I didn't make this name change to isInstrumentedCondition(). I 
will do that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84467/new/

https://reviews.llvm.org/D84467

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage

[Alan Phipps via Phabricator via cfe-commits]

alanphipps updated this revision to Diff 287269.
alanphipps marked 2 inline comments as done.
alanphipps added a comment.

Updating for formatting and comments (and some test adjustments after rebase).  
Bypassing logical-NOT operators in CodeGenFunction::isLeafCondition(), which 
checks the condition for the presence of logical-AND and logical-OR. (this can 
be further expanded, if necessary)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84467/new/

https://reviews.llvm.org/D84467

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenPGO.cpp
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/test/CoverageMapping/branch-constfolded.cpp
  clang/test/CoverageMapping/branch-logical-mixed.cpp
  clang/test/CoverageMapping/branch-macros.cpp
  clang/test/CoverageMapping/branch-mincounters.cpp
  clang/test/CoverageMapping/branch-templates.cpp
  llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
  llvm/include/llvm/ProfileData/InstrProf.h
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
  llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
  llvm/test/tools/llvm-cov/branch-c-general.c
  llvm/test/tools/llvm-cov/branch-export-json.test
  llvm/test/tools/llvm-cov/branch-export-lcov.test
  llvm/test/tools/llvm-cov/branch-logical-mixed.cpp
  llvm/test/tools/llvm-cov/branch-noShowBranch.test
  llvm/tools/llvm-cov/CodeCoverage.cpp
  llvm/tools/llvm-cov/CoverageExporterJson.cpp
  llvm/tools/llvm-cov/CoverageExporterLcov.cpp
  llvm/tools/llvm-cov/CoverageReport.cpp
  llvm/tools/llvm-cov/CoverageSummaryInfo.cpp
  llvm/tools/llvm-cov/CoverageViewOptions.h
  llvm/tools/llvm-cov/SourceCoverageView.cpp
  llvm/tools/llvm-cov/SourceCoverageView.h
  llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp
  llvm/tools/llvm-cov/SourceCoverageViewText.cpp

Index: llvm/tools/llvm-cov/SourceCoverageViewText.cpp
===
--- llvm/tools/llvm-cov/SourceCoverageViewText.cpp
+++ llvm/tools/llvm-cov/SourceCoverageViewText.cpp
@@ -223,14 +223,13 @@
   /*ShowTitle=*/false, ViewDepth + 1);
 }
 
-void SourceCoverageViewText::renderBranchView(raw_ostream &OS,
-  BranchView &BRV,
+void SourceCoverageViewText::renderBranchView(raw_ostream &OS, BranchView &BRV,
   unsigned ViewDepth) {
   // Render the child subview.
   if (getOptions().Debug)
 errs() << "Branch at line " << BRV.getLine() << '\n';
 
-  for (const auto R : BRV.Regions) {
+  for (const auto &R : BRV.Regions) {
 double TruePercent = 0.0;
 double FalsePercent = 0.0;
 unsigned Total = R.ExecutionCount + R.FalseExecutionCount;
@@ -249,8 +248,9 @@
 }
 
 colored_ostream(OS, raw_ostream::RED,
-getOptions().Colors && !R.ExecutionCount,
-/*Bold=*/false, /*BG=*/true) << "True";
+getOptions().Colors && !R.ExecutionCount,
+/*Bold=*/false, /*BG=*/true)
+<< "True";
 
 if (getOptions().ShowBranchCounts)
   OS << ": " << formatCount(R.ExecutionCount) << ", ";
@@ -258,8 +258,9 @@
   OS << ": " << format("%0.2f", TruePercent) << "%, ";
 
 colored_ostream(OS, raw_ostream::RED,
-getOptions().Colors && !R.FalseExecutionCount,
-/*Bold=*/false, /*BG=*/true) << "False";
+getOptions().Colors && !R.FalseExecutionCount,
+/*Bold=*/false, /*BG=*/true)
+<< "False";
 
 if (getOptions().ShowBranchCounts)
   OS << ": " << formatCount(R.FalseExecutionCount);
Index: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp
===
--- llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp
+++ llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp
@@ -656,8 +656,7 @@
   OS << EndExpansionDiv;
 }
 
-void SourceCoverageViewHTML::renderBranchView(raw_ostream &OS,
-  BranchView &BRV,
+void SourceCoverageViewHTML::renderBranchView(raw_ostream &OS, BranchView &BRV,
   unsigned ViewDepth) {
   // Render the child subview.
   if (getOptions().Debug)
@@ -665,9 +664,8 @@
 
   OS << BeginExpansionDiv;
   OS << BeginPre;
-  for (const auto R : BRV.Regions)
-  {
-// Calculate TruePercent and False Percent
+  for (const auto &R : BRV.Regions) {
+// Calculate TruePercent and False Percent.
 double TruePercent = 0.0;
 double FalsePercent = 0.0;
 unsigned Total = R.ExecutionCount + R.FalseExecutionCount;
@@ -677,21 +675,24 @@
   FalsePercent = ((double)(R.FalseExecutionCount) / (double)Total) * 100.0;
 }
 
-// Display Line + Column
+// Display Line + Column.
 std::string LineNoStr = utostr(uint64_t(R.LineStart));
 std::string ColNoStr = utostr(uint64_t(R.ColumnStart));
 std::string TargetName = "L" + LineNoStr;

[PATCH] D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage

[Alan Phipps via Phabricator via cfe-commits]

alanphipps marked 13 inline comments as done.
alanphipps added a comment.
Herald added a subscriber: wenlei.

In D84467#2180421 , @vsk wrote:

> I haven't taken a close look at the tests yet, but plan on getting to it soon.
>
> I'm not sure whether this is something you've already tried, but for this 
> kind of change, it can be helpful to verify that a stage2 clang self-host 
> with assertions enabled completes successfully. For coverage specifically, 
> it's possible to do that by setting '-DLLVM_BUILD_INSTRUMENTED_COVERAGE=On' 
> during the stage2 cmake step.

Thank you for recommending this.  I was able to do an instrumented stage2 build 
successfully, with assertions enabled, and it built fine, and no problems 
running LIT.  I have to say that it was also cool to see branch coverage on the 
source code I added to support branch coverage!




Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:742
+// function's SourceRegions) because it doesn't apply to any other source
+// code other than the Condition.
+if (CodeGenFunction::isLeafCondition(C)) {

vsk wrote:
> alanphipps wrote:
> > vsk wrote:
> > > Is there some alternative to pushing and then immediately popping branch 
> > > regions? Did you try adding the region directly to the function's 
> > > SourceRegions, and find that there was some issue with the more direct 
> > > approach?
> > The push/pop combo does look strange, and you're right -- I can just add it 
> > directly to SourceRegions, but the algorithm in popRegions() will do 
> > additional things like adjust the start/end location if the region spans a 
> > nested file depth. I think that's still useful to do.  Another option is I 
> > could factor that algorithm out of popRegions() into a new function and 
> > also call that from createBranchRegions().
> If it's possible/straightforward to factor the region start/end adjustment 
> code out of popRegions(), that would be really nice. If not, the current 
> approach looks reasonable.
I looked at it again, and I decided to table the refactor for now.  The 
adjustment code is coupled to SourceRegions as is the rest of popRegions(), and 
it may be necessary to rework the whole of the function to get a nicer refactor.



Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:659
 
+void SourceCoverageViewHTML::renderBranchView(raw_ostream &OS,
+  BranchView &BRV,

vsk wrote:
> alanphipps wrote:
> > vsk wrote:
> > > Wdyt of displaying branch-taken counts using tooltips? This would match 
> > > the way region execution counts are shown, which could be nice, because 
> > > the information wouldn't interrupt the flow of source code.
> > I think using tooltips is a great idea, and I did look into it.  
> > Ultimately, I decided to defer that work to a future patch to allow for 
> > some time to research a good way to properly distinguish the branch-taken 
> > counts from the region counts.
> Sounds reasonable. Can the branch coverage text/html output should be opt-in 
> via a cl::opt until it's finalized? That should make it possible to iterate 
> on the format without changing what users see.
Yep, I added two options:

--show-branch-summary, which shows the total branch coverage in the summary 
report, and this is ON by default.
--show-branches={count,percent}, which shows source-level coverage on a given 
file, which is OFF by default. So it's opt-in.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84467/new/

https://reviews.llvm.org/D84467

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

In D84467#2180421 , @vsk wrote:

> I haven't taken a close look at the tests yet, but plan on getting to it soon.
>
> I'm not sure whether this is something you've already tried, but for this 
> kind of change, it can be helpful to verify that a stage2 clang self-host 
> with assertions enabled completes successfully. For coverage specifically, 
> it's possible to do that by setting '-DLLVM_BUILD_INSTRUMENTED_COVERAGE=On' 
> during the stage2 cmake step.

I actually haven't tried that -- I've attempted to, but I've run into some 
cmake issues trying to generate the bootstrap build configuration, but I'll 
look into it further.  But I have successfully been able to use a host clang 
compiler to build my target compiler with assertions enabled and coverage 
instrumentation enabled, and I was able to use that to confirm that the 
coverage tests (with my additional tests) sufficiently cover the code I added, 
and no assertions fired.




Comment at: clang/lib/CodeGen/CodeGenFunction.h:4359
+  /// condition (i.e. no "&&" or "||").
+  static bool isLeafCondition(const Expr *C);
+

vsk wrote:
> alanphipps wrote:
> > vsk wrote:
> > > vsk wrote:
> > > > It might be helpful to either require that `C` be the RHS of a logical 
> > > > binop (e.g. by passing the binop expr in), or to document that 
> > > > requirement.
> > > Given a logical binop like `E = a && !(b || c)`, 
> > > `isLeafCondition(E->getRHS())` is true. That seems a bit 
> > > counter-intuitive, because `E->getRHS()` contains another leaf condition. 
> > > Would it make sense to rename the condition (perhaps to something like 
> > > 'InstrumentedCondition')? Have I misunderstood what a leaf condition is?
> > Background: isLeafCondition() is an auxiliary routine that is used during 
> > 1.) counter allocation on binop RHS (CodeGenPGO.cpp), 2.) counter 
> > instrumentation (CodeGenFunction.cpp), and 3.) branch region generation 
> > (CoverageMappingGen.cpp).  In the #3 case, it isn't always looking at the 
> > RHS of a logical binop but can be used to assess whether any condition is 
> > instrumented/evaluated.
> > 
> > Given your example condition:
> > 
> > E = a && !(b || c)
> > 
> > You are correct that isLeafCondition(E->getRHS()) will return true, but I 
> > think it should actually return false here (and bypass logical-NOT), so I 
> > will adapt this. 
> > 
> > However, given a condition that includes an binary operator (like assign):
> > 
> > E = a && (x = !(b || c))
> > 
> > isLeafCondition(E->getRHS()) will also return true, and I think that is the 
> > correct behavior.  Given that, then I agree that maybe isLeafCondition() 
> > should be renamed to isInstrumentedCondition() since it's not technically 
> > just leaves that are tracked in the presence of operators.
> What is special about the assignment expression nested within "a && (x = !(b 
> || c))" that necessitates an extra counter compared to "a && !(b || c)"?
I'm exempting the logical NOT operator basically to match the functionality of 
other coverage vendors (Bullseye, GCOV).  It's a simplistic operator in the 
sense that (as far as I can tell) it only affects the sense of the generated 
branch on a condition.

As for the assignment, we're effectively creating a new condition that is 
evaluatable (even if a branch may technically not be generated), and so 
creating a counter for it is more interesting (and matches other vendor 
behavior).

But I'm open to persuasion.  We could just be more conservative and create 
counters for logical NOT, but I think there's value in matching some of the 
expected behavior of other vendors.  This is also something we could continue 
to fine-tune in future patches.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:46
 class SourceMappingRegion {
+  /// Primary Counter that is also used for Branch Regions for "True" branches
   Counter Count;

vsk wrote:
> nit: Please end comments with a '.'
> nit: Please end comments with a '.'

Will do; I'll upload a diff soon with formatting cleanup.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:523
+
+RegionStack.emplace_back(Count, FalseCount, StartLoc, EndLoc,
+ FalseCount.hasValue());

vsk wrote:
> It looks like this sets up a deferred region when a branch count (FalseCount) 
> is available. Could you explain why, and when the deferred region is expected 
> to be complete?
Does adding a region to the RegionStack always imply setting up a deferred 
region that needs to be completed?  My intention was to add the branch region 
to the RegionStack but not to setup or complete a deferred region for branch 
regions.  

Branch regions are straightforward since we already know start location and end 
location when they are created, but I have probably misunderstood what a 
deferred region implies.




==

[PATCH] D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

Thank you for your comments on the review! I will respond soon; also have been 
swamped this week.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84467/new/

https://reviews.llvm.org/D84467

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage

[Alan Phipps via Phabricator via cfe-commits]

alanphipps marked 2 inline comments as done.
alanphipps added inline comments.



Comment at: clang/lib/CodeGen/CGStmt.cpp:1368
   CaseDest = createBasicBlock("sw.bb");
-  EmitBlockWithFallThrough(CaseDest, &S);
+  EmitBlockWithFallThrough(CaseDest, CurCase);
 }

This fixes a defect that resulted in the wrong counter being instrumented for 
each Case.



Comment at: clang/lib/CodeGen/CodeGenFunction.h:4359
+  /// condition (i.e. no "&&" or "||").
+  static bool isLeafCondition(const Expr *C);
+

vsk wrote:
> vsk wrote:
> > It might be helpful to either require that `C` be the RHS of a logical 
> > binop (e.g. by passing the binop expr in), or to document that requirement.
> Given a logical binop like `E = a && !(b || c)`, 
> `isLeafCondition(E->getRHS())` is true. That seems a bit counter-intuitive, 
> because `E->getRHS()` contains another leaf condition. Would it make sense to 
> rename the condition (perhaps to something like 'InstrumentedCondition')? 
> Have I misunderstood what a leaf condition is?
Background: isLeafCondition() is an auxiliary routine that is used during 1.) 
counter allocation on binop RHS (CodeGenPGO.cpp), 2.) counter instrumentation 
(CodeGenFunction.cpp), and 3.) branch region generation 
(CoverageMappingGen.cpp).  In the #3 case, it isn't always looking at the RHS 
of a logical binop but can be used to assess whether any condition is 
instrumented/evaluated.

Given your example condition:

E = a && !(b || c)

You are correct that isLeafCondition(E->getRHS()) will return true, but I think 
it should actually return false here (and bypass logical-NOT), so I will adapt 
this. 

However, given a condition that includes an binary operator (like assign):

E = a && (x = !(b || c))

isLeafCondition(E->getRHS()) will also return true, and I think that is the 
correct behavior.  Given that, then I agree that maybe isLeafCondition() should 
be renamed to isInstrumentedCondition() since it's not technically just leaves 
that are tracked in the presence of operators.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84467/new/

https://reviews.llvm.org/D84467



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79961: [PGO] Fix computation of fuction Hash

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

It looks like clang/test/Profile/Inputs/c-general.profdata.v5 is being read as 
v6 rather than v5.  Can you double check?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79961/new/

https://reviews.llvm.org/D79961



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79628: [Clang][Driver] Add Bounds and Thread to SupportsCoverage list

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

clang/test/CodeGen/sanitize-coverage.c is also failing our downstream embedded 
ARMv7 validations.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79628/new/

https://reviews.llvm.org/D79628



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72897: List implicit operator== after implicit destructors in a vtable.

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

This test is still failing on the arm bots and also with my downstream ARM 
compiler validation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72897/new/

https://reviews.llvm.org/D72897



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits