[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

vsk added a comment. So long as it doesn't change behavior expected by tapi on Darwin, I think it's OK. I doubt any other platforms are similarly affected. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122336/new/ https://reviews.llvm.org/D122336

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

vsk added a subscriber: cishida. vsk added a comment. Sorry for ay delayed replies - I've switched teams at Apple and find it difficult to keep up with llvm reviews. > it's my understanding is that we might be generating coverage record for > unused functions for TAPI. Coverage function

[PATCH] D111793: [Driver][Darwin] Use T reference instead of getToolChain().getTriple().

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, lgtm. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111793/new/ https://reviews.llvm.org/D111793

[PATCH] D106733: [clang/darwin] Pass libclang_rt.profile last on linker command

vsk added a comment. @thakis thanks for doing this, lgtm as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106733/new/ https://reviews.llvm.org/D106733 ___ cfe-commits mailing list

[PATCH] D104556: [InstrProfiling] Make CountersPtr in __profd_ relative

vsk added a comment. A one-time exception to the .profraw compatibility policy sounds reasonable to me. A little more context: llvm has historically rev'd the .profraw format with abandon to deliver performance/size improvements (as David & co. did with name compression) and new functionality

[PATCH] D20401: [Lexer] Don't merge macro args from different macro files

vsk added a comment. In D20401#2770059 , @nickdesaulniers wrote: > I know this was sped up slightly in 3339c568c43e4644f02289e5edfc78c860f19c9f, > but this change makes `updateConsecutiveMacroArgTokens` the hottest function > in clang in a bottom up

[PATCH] D102818: [PGO] Don't reference functions unless value profiling is enabled

vsk accepted this revision. vsk added a comment. Thanks, lgtm as well. On Darwin, the __llvm_prf_data section is marked with S_ATTR_LIVE_SUPPORT to allow the linker to dead strip functions even if they are pointed-to by a profd global. Removing the reference altogether should yield even better

[PATCH] D101000: Coverage: Document how to collect a profile without a filesystem

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. This looks great, thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101000/new/ https://reviews.llvm.org/D101000 ___ cfe-commits mailing

[PATCH] D101000: Coverage: Document how to collect a profile without a filesystem

vsk added a subscriber: efriedma. vsk added a comment. Thanks for doing this! + Eli to comment on whether any workarounds documented in https://lists.llvm.org/pipermail/llvm-dev/2017-September/117156.html are still necessary. To my knowledge it's not currently necessary to explicitly pass

[PATCH] D98061: [InstrProfiling] Generate runtime hook for ELF platforms

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks for the detailed explanation of the -fprofile-list workflow; given the difference constraints, this patch lgtm. Please document the divergent behavior re: no .profraw file when #counters ==

[PATCH] D98135: [OpenMP][InstrProfiling] Fix a missing instr profiling counter

vsk added a comment. In D98135#2615492 , @MaskRay wrote: > LG, but I hope an OpenMP expert can take a look. Perhaps @vsk can comment on > where the tests should be improved (currently we hardly have any > `@llvm.instrprof.increment` IR test for clang

[PATCH] D98061: [InstrProfiling] Generate runtime hook for ELF platforms

vsk added a comment. In D98061#2615386 , @phosek wrote: > In D98061#2615334 , @vsk wrote: > >> In D98061#2615250 , @phosek wrote: >> >>> In D98061#2615239

[PATCH] D98061: [InstrProfiling] Generate runtime hook for ELF platforms

vsk added a comment. In D98061#2615250 , @phosek wrote: > In D98061#2615239 , @vsk wrote: > >> @ributzka may have stronger thoughts about when -fprofile-instr-generate >> must imply that a known set of symbols

[PATCH] D98061: [InstrProfiling] Generate runtime hook for ELF platforms

vsk added a subscriber: ributzka. vsk added a comment. @ributzka may have stronger thoughts about when -fprofile-instr-generate must imply that a known set of symbols appear with external visibility. Up until now, the answer has been "always", and this is what tapi enforces for MachO. It's

[PATCH] D97101: [Coverage] Emit gap region between statements if first statements contains terminate statements.

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, this looks great. Comment at: clang/test/CoverageMapping/switch.cpp:62 default: // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:10 = #4 break;

[PATCH] D97101: [Coverage] Emit zero count gap region between statements if first statements contains return, throw, goto, co_return

vsk added a comment. Thanks for the patient explanation. Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1391 +// Clear DeferredRegion because we don't need to complete it after switch. +DeferredRegion = None; + Is the reason why we don't need to

[PATCH] D97101: [Coverage] Emit zero count gap region between statements if first statements contains return, throw, goto, co_return

vsk added inline comments. Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:942 pushRegion(Counter::getZero()); -auto = getRegion(); -ZeroRegion.setDeferred(true); -LastTerminatedRegion = {EndLoc, RegionStack.size()}; +if (!HasTerminateStmt) { +

[PATCH] D95753: [Coverage] Store compilation dir separately in coverage mapping

vsk added a comment. Just skimming, haven't had a chance to look closely. Comment at: llvm/unittests/ProfileData/CoverageMappingTest.cpp:149 if (R != Files.end()) return R->second; +unsigned Index = Files.size() + 1; assert(R->second > 0 &&

[PATCH] D97101: [Coverage] Emit zero count gap region between statements if first statements contains return, throw, goto, co_return

vsk added inline comments. Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:559 - /// Location of the last terminated region. - Optional> LastTerminatedRegion; + /// If there is a return, co_return, goto or throw inside. + bool HasTerminateStmt = false;

[PATCH] D97001: [Coverage] Normalize compilation dir as well

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97001/new/ https://reviews.llvm.org/D97001 ___

[PATCH] D97001: [Coverage] Normalize compilation dir as well

vsk added a comment. Would it be straightforward to test this, like we do other filename encodings in abspath.cpp? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97001/new/ https://reviews.llvm.org/D97001

[PATCH] D85176: [Coverage] Emit gap region after conditions when macro is present.

vsk accepted this revision. vsk added a comment. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85176/new/ https://reviews.llvm.org/D85176 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D85176: [Coverage] Emit gap region after conditions when macro is present.

vsk added a comment. Could you check in the reproducer program (`void p`) as a regression test? Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:995 + +AfterLoc = getPreciseTokenLocEnd(getIncludeOrExpansionLoc(AfterLoc)); +assert(AfterLoc.isValid());

[PATCH] D95753: Store compilation dir separately in coverage mapping

vsk added inline comments. Comment at: llvm/docs/CoverageMappingFormat.rst:277 +There is one difference between versions 6 and 5: + +There is one difference between versions 5 and 4: I think the above bullet point could go in this section? Repository: rG

[PATCH] D95753: Store compilation dir separately in coverage mapping

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, lgtm. The pre-merge checks appear to have flagged some issues, but I don't anticipate any major revision. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D95753: Store compilation dir separately in coverage mapping

vsk added a comment. Two minor missing items: - Please include a binary test that verifies llvm-cov can prepare a report for a binary containing a Version6 coverage blob (for compatibility testing). - Please also include a short blurb in docs/CoverageMappingFormat.rst explaining the format

[PATCH] D95753: Store compilation dir separately in coverage mapping

vsk added a comment. This looks great, that turned out to be a lot cleaner than I expected. Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1717 + FilenameStrs[0] = getCurrentDirname(); + FilenameRefs[0] = FilenameStrs[0]; for (const auto : FileEntries) {

[PATCH] D96000: Don't emit coverage mapping for excluded functions

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, lgtm. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96000/new/ https://reviews.llvm.org/D96000

[PATCH] D96000: Don't emit coverage mapping for excluded functions

vsk added inline comments. Comment at: clang/test/CodeGen/profile-filter.c:27 +// EXCLUDE: @__covrec_{{[0-9A-F]+}}u = linkonce_odr hidden constant <{ i64, i32, i64, i64, [{{.*}} x i8] }> <{ {{.*}} }>, section "__llvm_covfun" +// EXCLUDE-NOT: @__covrec_{{[0-9A-F]+}}u =

[PATCH] D95918: [Coverage] Propogate counter to condition of conditional operator

vsk added a comment. How was the issue spotted? If there was a crash or an incorrect coverage bug that's not triggered by one of the existing frontend tests, it'd be worth adding a regression test. If the problem can't be replicated without involving llvm-cov, this can be an integration test.

[PATCH] D95918: [Coverage] Propogate counter to condition of conditional operator

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, lgtm. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95918/new/ https://reviews.llvm.org/D95918

[PATCH] D95753: [CoverageMapping] Don't absolutize source paths

vsk added a comment. Thanks for pointing DW_AT_comp_dir out. For coverage, gating the use of relative paths on a flag sounds reasonable to me. It should also be possible to extend the .covmapping format to include something like DW_AT_comp_dir: compared to adding a flag, this would take a lot

[PATCH] D95753: [CoverageMapping] Don't absolutize source paths

vsk added a comment. For context, the reason abspaths are used here is because users don't expect coverage results to go missing if the build directory changes (e.g. given `cd A; cc x.c; cd ../B; cc x.c`, the expectation is that coverage for two distinct x.c's is reported). I think it's

[PATCH] D94820: Support for instrumenting only selected files or functions

vsk added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:851 +return "csllvm"; + default: +llvm_unreachable("unknown instrumentation type"); If we add an instrumentation kind, it may be more convenient to trigger a

[PATCH] D94324: [InitLLVM] Ensure SIGPIPE handler installed before sigaction()

This revision was automatically updated to reflect the committed changes. Closed by commit rGe05baf40de8a: [InitLLVM] Ensure SIGPIPE handler installed before sigaction() (authored by vsk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94324/new/

[PATCH] D94324: [InitLLVM] Ensure SIGPIPE handler installed before sigaction()

vsk added a comment. In D94324#2487572 , @aganea wrote: > What are the exact conditions to repro Nick's failure? On Fedora 32, I ran > `py llvm-lit sigpipe-handling.c` before this patch and before > rGbf401256edd00e921a5d3a0bf4cf6ee66ae51cd6 >

[PATCH] D94324: [InitLLVM] Ensure SIGPIPE handler installed before sigaction()

vsk updated this revision to Diff 315498. vsk added a comment. Reduce the test a bit further. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94324/new/ https://reviews.llvm.org/D94324 Files: clang/test/Driver/sigpipe-handling.c

[PATCH] D94324: [InitLLVM] Ensure SIGPIPE handler installed before sigaction()

vsk updated this revision to Diff 315473. vsk added a comment. Fix a typo in the test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94324/new/ https://reviews.llvm.org/D94324 Files: clang/test/Driver/sigpipe-handling.c

[PATCH] D94324: [InitLLVM] Ensure SIGPIPE handler installed before sigaction()

vsk updated this revision to Diff 315472. vsk added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Add a clang driver test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94324/new/ https://reviews.llvm.org/D94324

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

vsk added a comment. In D84467#2448934 , @alanphipps wrote: > 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

[PATCH] D92001: [ubsan] Fix crash on __builtin_assume_aligned

vsk added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2524 // defined. - if (Ty->getPointeeType().isVolatileQualified()) + if (!Ty->getPointeeType().isNull() && Ty->getPointeeType().isVolatileQualified()) return; Is the pointee

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

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thank you, lgtm! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84467/new/ https://reviews.llvm.org/D84467 ___ cfe-commits mailing list

[PATCH] D89959: UBSAN: emit distinctive traps in trapping mode

vsk accepted this revision. vsk added subscribers: kcc, eugenis. vsk added a comment. This revision is now accepted and ready to land. Thanks, lgtm with two comments -- > Because of the extra traps there is a small code-size penalty, but it's > pretty small compared to what we accept just for

[PATCH] D89959: UBSAN: emit distinctive traps in trapping mode

vsk added a comment. I might've missed it, but the debug location merging behavior could use a test. Here's one way to write it: https://godbolt.org/z/Yb9PY9. In `@_Z13keep_locationPi`, the !dbg attachment on the trap should match `!DILocation(line: 1`. In `@_Z15merge_locationsPi`, the

[PATCH] D89959: UBSAN: emit distinctive traps in trapping mode

vsk added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3458 +llvm::CallInst *TrapCall = +Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::ubsantrap), + llvm::ConstantInt::get(CGM.Int32Ty, CheckHandlerID));

[PATCH] D89959: UBSAN: emit distinctive traps in trapping mode

vsk added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3447 // If we're optimizing, collapse all calls to trap down to just one per // function to save on code size. + if (TrapBBs.size() <= CheckHandlerID) 'per check, per function'?

[PATCH] D22943: [Driver] Add FIXME's where we can't use effective triples (NFC)

vsk abandoned this revision. vsk added a comment. This isn't on my radar at the moment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D22943/new/ https://reviews.llvm.org/D22943 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D57265: [PM/CC1] Add -f[no-]split-cold-code CC1 options to toggle splitting

vsk added a comment. Thanks Hans. I've merged the revert into the apple fork. While the commit was in-tree, we noticed that this enabled HCS during LTO. There was a surprising ~17% regression of 483.xalancbmk with LTO (+ PGO) enabled: `2020-10-16 CINT2006/483.xalancbmk 17.87%`. We don't

[PATCH] D57265: [PM/CC1] Add -f[no-]split-cold-code CC1 options to toggle splitting

vsk accepted this revision. vsk added a comment. Thank you! LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57265/new/ https://reviews.llvm.org/D57265 ___ cfe-commits mailing list

[PATCH] D57265: [PM/CC1] Add -f[no-]split-cold-code CC1 options to toggle splitting

vsk added a comment. @plotfi Sorry this work has stalled, unfortunately I haven't had any bandwidth to drive it forward. At this point I don't think there are any outstanding concerns with this patch. If anyone is willing to rebase and land it, I would be really grateful. It looks like part

[PATCH] D89078: Add `-f[no-]split-cold-code` toggling outlining & enable in -O

vsk added a comment. @compnerd thank you for working on upstreaming this patch! Would you be open to commandeering D57265 instead? This is my (unfortunately stalled) attempt to upstream the same patch, and it has some review concerns from Fedor Sergeev, Philip

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

vsk removed a subscriber: loic-joly-sonarsource. vsk added inline comments. Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1169 +createBranchRegion(S->getCond(), BodyCount, + subtractCounters(CondCount, BodyCount)); }

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

vsk added inline comments. Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1169 +createBranchRegion(S->getCond(), BodyCount, + subtractCounters(CondCount, BodyCount)); } If `theWhileStmt->getCond()` is-a BinaryOperator, it would

[PATCH] D88336: [ubsan] nullability-arg: Fix crash on C++ member function pointers

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG06bc685fa240: [ubsan] nullability-arg: Fix crash on C++ member pointers (authored by vsk). Changed prior to commit:

[PATCH] D88336: [ubsan] nullability-arg: Fix crash on C++ member function pointers

vsk added a comment. Thanks for the review! Comment at: clang/lib/CodeGen/CGExpr.cpp:1181 + if (T->isMemberPointerType()) +return CGM.getCXXABI().EmitMemberPointerIsNotNull( +*this, V, T->getAs()); ahatanak wrote: > You can fold `T->getAs()` into

[PATCH] D88336: [ubsan] nullability-arg: Fix crash on C++ member function pointers

vsk added a comment. In D88336#2296051 , @ahatanak wrote: > It looks like this still doesn't check null correctly (i.e., compare to -1) > for data member pointers. Is that correct? Thanks for catching this. The new revision takes advantage of

[PATCH] D88336: [ubsan] nullability-arg: Fix crash on C++ member function pointers

vsk updated this revision to Diff 294457. vsk added a comment. Simplify, per Akira's comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88336/new/ https://reviews.llvm.org/D88336 Files: clang/lib/CodeGen/CGCall.cpp

[PATCH] D88336: [ubsan] nullability-arg: Fix crash on C++ member function pointers

vsk created this revision. vsk added reviewers: jkorous, ahatanak. Herald added a subscriber: dexonsmith. Herald added a project: clang. vsk requested review of this revision. Extend -fsanitize=nullability-arg to handle call sites which accept C++ member function pointers. rdar://62476022

[PATCH] D87332: [profile] Add %t LLVM_PROFILE_FILE option to substitute $TMPDIR

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG62c372770d2e: [profile] Add %t LLVM_PROFILE_FILE option to substitute $TMPDIR (authored by vsk). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D87648: [Coverage][NFC] Remove skipped region after added into MappingRegions

vsk added a comment. (Depending on what the potential performance gains look like, it might be advisable to keep things simple with the current O(n^2) approach. Optimizing things can carry some risk -- not sure if we've already ruled this out, but e.g. perhaps there's some edge case with

[PATCH] D87648: [Coverage][NFC] Remove skipped region after added into MappingRegions

vsk added a comment. Unfortunately it does look like the work done in gatherSkippedRegions is O(n^2) in the number of functions, at the moment. If the goal is to speed it up, it'd be good to grab some performance numbers for some representative compile unit (the sqlite3 amalgamation is my

[PATCH] D84988: [Coverage] Add empty line regions to SkippedRegions

vsk added a comment. @zequanwu FTR, I don't have any outstanding concerns (I understand you might be asking for a second reviewer to chime in though). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84988/new/ https://reviews.llvm.org/D84988

[PATCH] D84988: [Coverage] Add empty line regions to SkippedRegions

vsk added inline comments. Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:38 + "disable it on test)"), +llvm::cl::init(true)); + We might consider marking this as cl::Hidden. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D84988: [Coverage] Add empty line regions to SkippedRegions

vsk added a comment. In D84988#2264199 , @zequanwu wrote: > Thanks for reviewing. One last thing I need to resolve is to handle the > coverage test case as skipped regions are emitted for empty lines, and > haven't thought a good way other than adding

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

vsk added a comment. @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

[PATCH] D84988: [Coverage] Add empty line regions to SkippedRegions

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. I took a closer look at the lexer changes, and think that these look fine. Thanks again for working on this. LGTM with a minor change, described inline. Comment at:

[PATCH] D87332: [profile] Add %t LLVM_PROFILE_FILE option to substitute $TMPDIR

vsk updated this revision to Diff 290590. vsk added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. - Document the new option. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87332/new/

[PATCH] D84988: [Coverage] Add empty line regions to SkippedRegions

vsk added a comment. I'm not as familiar with the preprocessor bits, but this looks like it's in good shape. Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:597 const auto = Segments[I]; - if (!(L.Line < R.Line) && !(L.Line == R.Line && L.Col <

[PATCH] D84988: [Coverage] Add empty line regions to SkippedRegions

vsk added inline comments. Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:483 bool GapRegion = CR.value().Kind == CounterMappingRegion::GapRegion; if (CR.index() + 1 == Regions.size() || zequanwu wrote: > vsk wrote: > > zequanwu

[PATCH] D86000: Add an unsigned shift base sanitizer

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Lgtm, thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D86000

[PATCH] D86000: Add an unsigned shift base sanitizer

vsk added inline comments. Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:6 + volatile unsigned _v = (val);\ + volatile unsigned _a = (amount); \ + unsigned res = _v << _a; \ jfb wrote: > vsk wrote: > > Does the test not

[PATCH] D86116: [Coverage] Adjust skipped regions only if {Prev,Next}TokLoc is in the same file as regions' {start, end}Loc

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Lgtm, thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86116/new/ https://reviews.llvm.org/D86116

[PATCH] D84988: [Coverage] Add empty line regions to SkippedRegions

vsk added inline comments. Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:483 bool GapRegion = CR.value().Kind == CounterMappingRegion::GapRegion; if (CR.index() + 1 == Regions.size() || zequanwu wrote: > vsk wrote: > > Why is this

[PATCH] D84988: [Coverage] Add empty line regions to SkippedRegions

vsk added inline comments. Comment at: clang/include/clang/Lex/Lexer.h:131 + const char *NewLinePtr; + Could you leave a comment describing what this is? Comment at: clang/include/clang/Lex/Preprocessor.h:960 + void

[PATCH] D84988: WIP [Coverage] Add empty line regions to SkippedRegions

vsk added a comment. Hi @zequanwu, are you looking for review for this patch? I wasn't sure because of the WIP label. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84988/new/ https://reviews.llvm.org/D84988

[PATCH] D86000: Add an unsigned shift base sanitizer

vsk added a comment. In D86000#2219322 , @jfb wrote: > In D86000#2219288 , @vsk wrote: > >> It'd be nice to fold the new check into an existing sanitizer group to bring >> this to a wider audience. Do you foresee

[PATCH] D86000: Add an unsigned shift base sanitizer

vsk added inline comments. Comment at: clang/test/Driver/fsanitize.c:911 +// CHECK-unsigned-shift-base-RECOVER-NOT: "-fsanitize-trap=unsigned-shift-base" +// CHECK-unsigned-shift-base-NORECOVER-NOT: "-fno-sanitize-recover=unsigned-shift-base" +//

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

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

[PATCH] D86000: Add an unsigned shift base sanitizer

vsk added a comment. It'd be nice to fold the new check into an existing sanitizer group to bring this to a wider audience. Do you foresee adoption issues for existing -fsanitize=integer adopters? Fwiw some recently-added implicit conversion checks were folded in without much/any pushback.

[PATCH] D85176: [Coverage] Enable emitting gap area between macros

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, looks good to me! Please check for any issues in a stage2 coverage build before landing this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D79279: Add overloaded versions of builtin mem* functions

vsk added inline comments. Comment at: compiler-rt/lib/ubsan/ubsan_handlers.cpp:659 + uptr PtrOrSize) { GET_REPORT_OPTIONS(true); + handleInvalidBuiltin(Data, Opts, PtrOrSize); It looks like

[PATCH] D85176: [Coverage] Enable emitting gap area between macros

vsk added inline comments. Comment at: clang/test/CoverageMapping/macro-expressions.cpp:63 // CHECK-NEXT: File 0, [[@LINE+1]]:16 -> [[@LINE+1]]:18 = #2 if (EXPR(i)) {} // CHECK-NEXT: Expansion,File 0, [[@LINE+2]]:9 -> [[@LINE+2]]:14 = (#0 + #3) The gap

[PATCH] D84405: CodeGen: Improve generated IR for __builtin_mul_overflow(uint, uint, int)

vsk added a comment. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84405/new/ https://reviews.llvm.org/D84405 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

vsk added a comment. 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

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

vsk added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.h:4359 + /// condition (i.e. no "&&" or "||"). + static bool isLeafCondition(const Expr *C); + alanphipps wrote: > vsk wrote: > > vsk wrote: > > > It might be helpful to either require

[PATCH] D83592: [Coverage] Add comment to skipped regions

vsk added a comment. @zequanwu the change that dropped merging of adjacent comments looks good. If no further issues cropped up during stage2 testing, please feel free to re-land. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592

[PATCH] D84405: CodeGen: Improve generated IR for __builtin_mul_overflow(uint, uint, int)

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, looks good to me with a small test addition. Comment at: clang/test/CodeGen/builtins-overflow.c:123 + // CHECK: br i1 [[C2]] + int r; + if (__builtin_mul_overflow(x, y,

[PATCH] D83592: [Coverage] Add comment to skipped regions

vsk added a comment. In D83592#2170912 , @zequanwu wrote: > I don't why when using the cmake option > `-DLLVM_BUILD_INSTRUMENTED_COVERAGE=On` to test coverage for clang itself, it > does tracking comments. > Also, with that option on, `llvm-cov`

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

vsk added a reviewer: zequanwu. vsk added subscribers: ikudrin, arphaman. vsk added a comment. @alanphipps thanks for the patch! I'm a bit swamped at the moment, but I hope to give a detailed review by this Wednesday. Comment at: clang/lib/CodeGen/CodeGenFunction.h:4359 +

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

vsk added a comment. @phosek I suspect this is causing a cmake error on the lldb standalone bot, would you mind taking a look? http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake-standalone/1858/ CMake Error at

[PATCH] D84405: CodeGen: Improve generated IR for __builtin_mul_overflow(uint, uint, int)

vsk added a comment. How were you able to show that the specialized IRGen is equivalent to the generic kind? I tried doing this with direct inspection (https://godbolt.org/z/o5WEn3), but wasn't able to convince myself. In the past I used a test driver to compare the before/after compiler

[PATCH] D83592: [Coverage] Add comment to skipped regions

vsk added a comment. @zequanwu I'm not sure whether this is something you've already tried, but for frontend changes, 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

[PATCH] D83592: [Coverage] Add comment to skipped regions

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, lgtm. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592 ___ cfe-commits mailing list

[PATCH] D83592: [Parser] Add comment to skipped regions

vsk added a comment. The testing looks really good. Just a few more requests for documentation (inline). Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:309 + /// Return a new SpellingRegion for the SkippedRange if it's valid. + Optional

[PATCH] D83592: [Parser] Add comment to skipped regions

vsk added a comment. @zequanwu at a high level, I think this is the right approach and it looks nice overall. I do have some feedback (inline). Thanks! Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:310 + /// Return true if SR should be emitted as SkippedRange. + bool

[PATCH] D83592: [Parser] Add comment to skipped regions

vsk added a comment. > ! In D83592#2156758 , @zequanwu > wrote: > One way I could think is probably when we visit decl in > `CounterCoverageMappingBuilder`, check for if there is `SkippedRegion` in the > same line and then mark the decl range as

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Looks good to me. Comment at: clang/test/CodeGenObjCXX/os_log.mm:16 // An `invoke` of a `nounwind` callee is simplified to a direct // call by an optimization in llvm.

[PATCH] D83592: [Parser] Add comment to skipped regions

vsk added a comment. Could you add an end-to-end llvm-cov test (see e.g. compiler-rt/test/profile/Linux/coverage_ctors.cpp)? Here are some important cases I think we should check: - `/* comment at the start of a line */ expr;` - `expr; /* comment at the end of a line */` - `expr; // comment at

[PATCH] D83592: [Parser] Add comment to skipped regions

vsk added a comment. Before updating any tests, maybe it's worth doing a quick experiment with comments placed in different contexts, to see whether adding these skipped regions is really sufficient. For example, given: 1| for (auto x : collection) { 2|// Explain the loop. 3| }

[PATCH] D83592: [Parser] Add comment to skipped regions

vsk added a comment. In D83592#2148638 , @zequanwu wrote: > In D83592#2148376 , @vsk wrote: > > > taking advantage of the existing CommentHandler interface? To simplify > > things, we can add a static method like

  1   2   3   4   5   6   7   >