[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

vsk added a comment. Still think this looks good. Have you tried running this on the llvm test suite, or some other interesting corpus? Would be curious to see any pre/post patch numbers. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4703-4720 +// 2) The sign of the diff

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

vsk added a comment. > In D67122#1659721 , @vsk wrote: > >> Still think this looks good. Have you tried running this on the llvm test >> suite, or some other interesting corpus? Would be curious to see any >> pre/post patch numbers. > > > There were 1.

[PATCH] D35925: [Coverage] Precise region termination with deferred regions

vsk added a comment. In https://reviews.llvm.org/D35925#823978, @arphaman wrote: > This is awesome! > > I noticed in the sample output that llvm-cov is now forced to print some new > region markers because the terminator introduces a new region on the same > line, e.g. > > |// CHECK-LABEL: _Z

[PATCH] D34590: [ubsan] Diagnose invalid uses of builtins (clang)

vsk edited reviewers, added: eugenis; removed: efriedma. vsk added a comment. Ping. https://reviews.llvm.org/D34590 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36096: [ubsan] Make the 'vptr check disabled' warning more helpful

vsk created this revision. If -fsanitize=vptr is passed without -fsanitize=null being specified, it will say: warning: implicitly disabling vptr sanitizer because null checking wasn't enabled (try specifying -fsanitize=null or -fsanitize=undefined) Otherwise if the vptr check is enabled and

[PATCH] D32842: Specify which sanitizers are covered by a sanitizer blacklist

vsk added a comment. In https://reviews.llvm.org/D32842#826383, @shenhan wrote: > Ping? Can we make a decision on this? > I've this simple one D35849: [UBSan] Provide default blacklist filename for > UBSan which, depending on this, shall be > discarded or mov

[PATCH] D35849: [UBSan] Provide default blacklist filename for UBSan

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. This lgtm, we shouldn't defer this until https://reviews.llvm.org/D32842 is done, as that may take a while. https://reviews.llvm.org/D35849 ___ cfe-co

[PATCH] D36112: [ubsan] Have -fsanitize=vptr emit a null check if -fsanitize=null isn't available

vsk created this revision. In r309007, I made -fsanitize=null a hard prerequisite for -fsanitize=vptr. I did not see the need for the two checks to have separate null checking logic for the same pointer. I expected the two checks to either always be enabled together, or to be mutually compatibl

[PATCH] D36096: [ubsan] Make the 'vptr check disabled' warning more helpful

vsk abandoned this revision. vsk added a comment. Abandoned in favor of https://reviews.llvm.org/D36112, since it turns out -fsanitize=null,vptr doesn't work for some use cases. https://reviews.llvm.org/D36096 ___ cfe-commits mailing list cfe-commi

[PATCH] D35925: [Coverage] Precise region termination with deferred regions

vsk updated this revision to Diff 109142. vsk marked 2 inline comments as done. vsk edited the summary of this revision. vsk added a comment. Thanks for the review! I'll hold off on landing this until the llvm-cov changes are in, so that people don't hit the issue where unexpected line execution

[PATCH] D34801: [coverage] Make smaller regions for the first case of a switch.

vsk added a comment. In https://reviews.llvm.org/D34801#828382, @efriedma wrote: > I'm going to look over the overall algorithm one more time to make sure this > direction makes sense. The current code for ending regions on > break/continue/return/noreturn doesn't really work well, Maybe it's

[PATCH] D36112: [ubsan] Have -fsanitize=vptr emit a null check if -fsanitize=null isn't available

vsk marked an inline comment as done. vsk added a comment. In https://reviews.llvm.org/D36112#828891, @arphaman wrote: > That makes sense. It's kinda weird not to report the `null`, but I guess it > makes sense if the `null` sanitiser is off. It is kinda weird, but any such diagnostic would fi

[PATCH] D36250: [coverage] Special-case calls to noreturn functions.

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, lgtm. Repository: rL LLVM https://reviews.llvm.org/D36250 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-

[PATCH] D35925: [Coverage] Precise region termination with deferred regions

vsk closed this revision. vsk added a comment. Committed in r310010 https://reviews.llvm.org/D35925 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35925: [Coverage] Precise region termination with deferred regions

vsk added a comment. Reverted in r310154, because llvm-cov isn't doing a good job of displaying the new regions properly. I'll re-land this after llvm-cov is fixed. https://reviews.llvm.org/D35925 ___ cfe-commits mailing list cfe-commits@lists.llvm

[PATCH] D36250: [coverage] Special-case calls to noreturn functions.

vsk added a comment. I tested this out on clang's ItaniumMangle.cpp and the crash is resolved. Comment at: lib/CodeGen/CoverageMappingGen.cpp:723 + this->Visit(Child); +handleFileExit(getEnd(E)); + This is doing the right thing. I think it should just

[PATCH] D36250: [coverage] Special-case calls to noreturn functions.

vsk accepted this revision. vsk added a comment. Thanks, lgtm. Repository: rL LLVM https://reviews.llvm.org/D36250 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36492: [RFC][time-report] Add preprocessor timer

vsk added subscribers: mzolotukhin, vsk. vsk added a comment. Thanks for working on this. Collecting better timing information in the frontend sgtm. It's cheap to do, and we can use the information to guide our efforts re: attacking the compile-time problem. Feel free to add me to future timing

[PATCH] D36642: [Lexer] Report more precise skipped regions (PR34166)

vsk created this revision. This patch teaches the preprocessor to report more precise source ranges for code that is skipped due to conditional directives. The new behavior includes the '#' from the opening directive and the full text of the line containing the closing directive in the skipped

[PATCH] D36642: [Lexer] Report more precise skipped regions (PR34166)

vsk updated this revision to Diff 13. vsk added a comment. Thanks for the review. I've updated the patch so that we do better with "#\" directives. https://reviews.llvm.org/D36642 Files: include/clang/Lex/Preprocessor.h lib/Lex/PPDirectives.cpp test/CoverageMapping/preprocessor.c t

[PATCH] D36492: [time-report] Add preprocessor timer

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D36492 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commit

[PATCH] D36642: [Lexer] Report more precise skipped regions (PR34166)

vsk updated this revision to Diff 111596. vsk marked an inline comment as done. vsk added a comment. - Address Eli's comment. https://reviews.llvm.org/D36642 Files: include/clang/Lex/Preprocessor.h lib/Lex/PPDirectives.cpp test/CoverageMapping/preprocessor.c test/Index/skipped-ranges.c

[PATCH] D36642: [Lexer] Report more precise skipped regions (PR34166)

vsk added inline comments. Comment at: lib/Lex/PPDirectives.cpp:570 + // We'll warn about reaching the end of file later. + if (C == '\0' || C == '\r' || C == '\n') +break; efriedma wrote: > This doesn't really handle backslash-escaped newlines

[PATCH] D36969: [Basic] Add a DiagnosticOr type

vsk added a comment. Would it be more convenient to extend ErrorInfo instead of creating a new diagnostic wrapper? E.g one benefit of passing around Expected's is that there's some assurance the diagnostics will be reported. Repository: rL LLVM https://reviews.llvm.org/D36969 ___

[PATCH] D35271: Fix printing policy for AST context loaded from file

vsk added a comment. In https://reviews.llvm.org/D35271#847306, @jklaehn wrote: > In https://reviews.llvm.org/D35271#809159, @vsk wrote: > > > I wonder if it's possible to do away with the calls to 'updated()'... it > > seems strange that we initialize the same preprocessor repeatedly. Is there

[PATCH] D36969: [Basic] Add a DiagnosticError llvm::ErrorInfo subclass

vsk added a comment. Nice! I'd like to get your thoughts on two candidate ergonomic changes: Comment at: unittests/Basic/DiagnosticTest.cpp:81 + llvm::Expected> Value = + llvm::make_error(PartialDiagnosticAt( + SourceLocation(), PartialDiagnostic(diag::err_cannot

[PATCH] D35271: Fix printing policy for AST context loaded from file

vsk added a comment. Thanks for adding the test! This is looking good. Comment at: unittests/Frontend/ASTUnitTest.cpp:32 +llvm::SmallString<256> Dir; +ASSERT_FALSE(llvm::sys::fs::createUniqueDirectory("astunit-test", Dir)); +TestDir = Dir.str(); If

[PATCH] D36969: [Basic] Add a DiagnosticError llvm::ErrorInfo subclass

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. LGTM! Repository: rL LLVM https://reviews.llvm.org/D36969 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mail

[PATCH] D36492: [time-report] Add preprocessor timer

vsk added inline comments. Comment at: lib/Lex/Preprocessor.cpp:746 void Preprocessor::Lex(Token &Result) { + llvm::TimeRegion(PPOpts->getTimer()); + MatzeB wrote: > modocache wrote: > > erik.pilkington wrote: > > > Doesn't this just start a timer and immediate

[PATCH] D35271: Fix printing policy for AST context loaded from file

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, LGTM! This seems like a pretty straightforward bug fix. Since it's not my usual area maybe it'd be worth waiting a day or so for more feedback. https://reviews.llvm.org/D35271 __

[PATCH] D69970: [CGDebugInfo] Emit subprograms for decls when AT_tail_call is understood (reland with fixes)

vsk closed this revision. vsk added a comment. Yes, this landed in 568db780bb7267651a902da8e85bc59fc89aea70 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69970/new/ https://reviews.llvm.org/D69970 ___

[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements

vsk added inline comments. Comment at: compiler-rt/lib/ubsan/ubsan_value.cpp:29 +const char *__ubsan::getObjCClassName(ValueHandle Pointer) { +#if defined(__APPLE__) + // We need to query the ObjC runtime for some information, but do not want delcypher wrote: >

[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements

vsk updated this revision to Diff 238393. vsk marked an inline comment as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71491/new/ https://reviews.llvm.org/D71491 Files: clang/docs/UndefinedBehaviorSanitizer.rst clang/include/clang/Basic/Sanitizers.def clang/lib/CodeGen/CGObj

[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. @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] 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 go-to

[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 nested

[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 SINC

[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 Repos

[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 clang/lib/CodeGen/CGExpr

[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 CXXABI::Em

[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 t

[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: https://reviews.llvm.org/D8833

[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 wo

[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] 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 wro

[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 &R = Segments[I]; - if (!(L.Line < R.Line) && !(L.Line == R.Line && L.Col < R.C

[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/ https://reviews.llvm.org/D8733

[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: clang/lib/Lex/

[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 libCover

[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 ch

[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 for

[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] 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)); } alanphipps

[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] 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 of

[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 cfe-commits@lists.l

[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 cfe-commi

[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 t

[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] 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 https://l

[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] 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)); v

[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 attachme

[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 th

[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] 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 pro

[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 import

[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 m

[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] 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] 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 = linkonce_o

[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] 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 &Entry : FileEntries) { -

[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 chan

[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 https://reviews.

[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 LLVM

[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] 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 h

[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] 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 ___ cfe-

[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] 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 && "Got

[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] 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 &ZeroRegion = getRegion(); -ZeroRegion.setDeferred(true); -LastTerminatedRegion = {EndLoc, RegionStack.size()}; +if (!HasTerminateStmt) {

[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 c

[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] 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 awkw

[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 appe

[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] 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 -fp

[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 == 0

[PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

vsk added a comment. In D73534#1933988 , @djtodoro wrote: > In D73534#1933985 , @djtodoro wrote: > > > Oh sorry, I thought it all has been fixed, since all the tests pass. > > > > We should revert the D75036

[PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

vsk added a comment. @manojgupta I apologize for not catching this earlier. The issue should really be fixed by 636665331bbd4c369a9f33c4d35fb9a863c94646 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73534/new/ http

[PATCH] D74813: [RFC] Add hash of block contents to function block names

vsk edited reviewers, added: ahatanak, erik.pilkington, dexonsmith; removed: vsk. vsk added a comment. At a high level the idea sounds good to me. For out OSes, symbol name instability results in serious performance regressions as this invalidates text/data orderfiles. Why shouldn’t this be on

[PATCH] D74813: [RFC] Add hash of block contents to function block names

vsk added a comment. In D74813#1883241 , @alexbdv wrote: > As for making it default - would rather have this under a flag as hashing the > block contents does have some overhead and I imagine this feature wouldn't be > beneficial in most scenarios. Also,

[PATCH] D69471: [Coverage] Revise format to reduce binary size

vsk planned changes to this revision. vsk added a comment. @rnk Thanks for chasing this down. I'll update the function record structs to use free functions instead of multiple inheritance. I don't plan on getting rid of the awkward method calls at this point. The coverage reader is still templa

[PATCH] D69471: [Coverage] Revise format to reduce binary size

vsk updated this revision to Diff 245756. vsk added a comment. This revision is now accepted and ready to land. Get rid of multiple inheritance in the coverage::accessors namespace. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69471/new/ https://reviews.llvm.org/D69471 Files: clang/

[PATCH] D69471: [Coverage] Revise format to reduce binary size

This revision was automatically updated to reflect the committed changes. Closed by commit rG99317124e1c7: [Coverage] Revise format to reduce binary size (authored by vsk). Changed prior to commit: https://reviews.llvm.org/D69471?vs=245756&id=247396#toc Repository: rG LLVM Github Monorepo C

[PATCH] D75615: Revert "[CGBlocks] Improve line info in backtraces containing *_helper_block"

vsk added inline comments. Comment at: clang/test/CodeGenObjC/debug-info-blocks.m:24 // CHECK: load {{.*}}, !dbg ![[DESTROY_LINE:[0-9]+]] -// CHECK: ret void, !dbg ![[DESTROY_LINE]] +// CHECK-DAG: [[DBG_LINE]] = !DILocation(line: 0, scope: ![[COPY_SP:[0-9]+]])

<    1   2   3   4   5   6   7   >