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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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.

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

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

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

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

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

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; ^

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

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:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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/

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

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

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

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

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

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

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

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

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

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

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

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, ); + EmitBlockWithFallThrough(CaseDest, CurCase); }

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

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

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

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

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

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