[PATCH] D55843: [CodeGen] Handle mixed-width ops in mixed-sign mul-with-overflow lowering

vsk updated this revision to Diff 178752. vsk marked an inline comment as done. vsk added a comment. - Simplify logic per Eli's review feedback. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55843/new/ https://reviews.llvm.org/D55843 Files: clang/lib/CodeGen/CGBuiltin.cpp clang/tes

[PATCH] D67774: [Mangle] Check ExternalASTSource before adding prefix to asm label names

vsk created this revision. vsk added reviewers: teemperor, jasonmolenda, rjmccall. LLDB may synthesize decls using asm labels. These decls cannot have a mangle different than the one specified in the label name. I.e., the mangle-suppression prefix should not be added. Fixes an expression evaluati

[PATCH] D67774: [Mangle] Add flag to asm labels to disable global prefixing

vsk updated this revision to Diff 221066. vsk retitled this revision from "[Mangle] Check ExternalASTSource before adding prefix to asm label names" to "[Mangle] Add flag to asm labels to disable global prefixing". vsk edited the summary of this revision. vsk added a comment. Thanks for your fee

[PATCH] D67774: [Mangle] Add flag to asm labels to disable global prefixing

vsk updated this revision to Diff 221391. vsk marked 3 inline comments as done. vsk added a comment. - Address latest review feedback. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67774/new/ https://reviews.llvm.org/D67774 Files: clang/include/clang/Basic/Attr.td clang/include/cla

[PATCH] D67774: [Mangle] Add flag to asm labels to disable global prefixing

vsk added inline comments. Comment at: clang/lib/AST/Mangle.cpp:130 + return; +} + rjmccall wrote: > This is actually backwards, right? A literal label is one that doesn't get > the global prefix and therefore potentially needs the `\01` prefix to > s

[PATCH] D67774: [Mangle] Add flag to asm labels to disable '\01' prefixing

vsk updated this revision to Diff 221645. vsk retitled this revision from "[Mangle] Add flag to asm labels to disable global prefixing" to "[Mangle] Add flag to asm labels to disable '\01' prefixing". vsk edited the summary of this revision. vsk added a comment. - Address latest round of comment

[PATCH] D67774: [Mangle] Add flag to asm labels to disable '\01' prefixing

vsk updated this revision to Diff 221801. vsk added a comment. - Add a comment describing where non-literal labels are used. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67774/new/ https://reviews.llvm.org/D67774 Files: clang/include/clang/Basic/Attr.td clang/include/clang/Basic/A

[PATCH] D46403: [CFI] Force LLVM to die if the implicit blacklist files cannot be found.

vsk requested changes to this revision. vsk added a comment. This revision now requires changes to proceed. Toolchain vendors aren't currently required to provide default blacklists for every sanitizer clang supports. We don't ship a default ubsan blacklist on macOS, so this patch would break ub

[PATCH] D46403: [CFI] Force LLVM to die if the implicit blacklist files cannot be found.

vsk added a comment. In https://reviews.llvm.org/D46403#1086923, @cmtice wrote: > Ok, I'll work on making this CFI-specific and adding a test case. Sounds good, thanks! I'd suggest modifying test/Driver/fsanitize-blacklist.c for your purposes. It should be possible to pair an empty resource di

[PATCH] D46403: [CFI] Force LLVM to die if the implicit blacklist files cannot be found.

vsk added inline comments. Comment at: lib/Driver/SanitizerArgs.cpp:118 BlacklistFiles.push_back(Path.str()); +else if (BL.Mask == CFI) + D.Diag(clang::diag::err_drv_no_such_file) << Path; CFI can be enabled with other sanitizers, such as ubsan. I

[PATCH] D46403: [CFI] Force LLVM to die if the implicit blacklist files cannot be found.

vsk added inline comments. Comment at: lib/Driver/SanitizerArgs.cpp:118 BlacklistFiles.push_back(Path.str()); +else if (BL.Mask == CFI) + D.Diag(clang::diag::err_drv_no_such_file) << Path; vsk wrote: > CFI can be enabled with other sanitizers, suc

[PATCH] D46403: [CFI] Force LLVM to die if the implicit blacklist files cannot be found.

vsk added a comment. One problem with this direction is that clang doesn't ship a cfi blacklist in its default install, so, this leaves cfi users with stock toolchains to fend for themselves. I think it'd be a good idea to ship a basic cfi blacklist in clang's resource dir to avoid inadvertent

[PATCH] D46403: [CFI] Force LLVM to die if the implicit blacklist files cannot be found.

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D46403#1088759, @pcc wrote: > We should be installing `compiler-rt/lib/cfi/cfi_blacklist.txt`, no? Oh, I see. This is already taken care of, then. @cmtice This looks fi

[PATCH] D46403: [CFI] Force LLVM to die if the implicit blacklist files cannot be found.

vsk added a comment. Do you have commit access? If not I'd be happy to land this for you. Comment at: test/Driver/Inputs/resource_dir_with_cfi_blacklist/cfi_blacklist.txt:19 +# in order to call std::allocator_traits::construct. +fun:_ZNSt23_Sp_counted_ptr_inplace*

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

vsk added a comment. Thanks for working on this! Comment at: docs/ReleaseNotes.rst:277 + behaviour. But they are not *always* intentional, and are somewhat hard to + track down. Could you mention whether the group is enabled by -fsanitize=undefined? =

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

vsk added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1694 // handle things like function to ptr-to-function decay etc. Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) { Expr *E = CE->getSubExpr(); lebedev.ri wrote: > vsk wrote: > > I thi

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

vsk added a subscriber: klimek. vsk added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1694 // handle things like function to ptr-to-function decay etc. Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) { Expr *E = CE->getSubExpr(); lebedev.

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

vsk added a comment. In https://reviews.llvm.org/D48958#1160435, @lebedev.ri wrote: > Thank you for taking a look! > > In https://reviews.llvm.org/D48958#1160381, @vsk wrote: > > > I have some minor comments but overall I think this is in good shape. It > > would be great to see some compile-tim

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

vsk added a comment. I have some minor comments but overall I think this is in good shape. It would be great to see some compile-time numbers just to make sure this is tractable. I'm pretty sure -fsanitize=null would fire more often across a codebase than this check, so I don't anticipate a big

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

vsk added a comment. In https://reviews.llvm.org/D48958#1160494, @lebedev.ri wrote: > In https://reviews.llvm.org/D48958#1160479, @vsk wrote: > > > In https://reviews.llvm.org/D48958#1160435, @lebedev.ri wrote: > > > > > Thank you for taking a look! > > > > > > In https://reviews.llvm.org/D48958#

[PATCH] D68733: Use -fdebug-compilation-dir to form absolute paths in coverage mappings

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, lgtm! In PR43614 I mentioned adding an extra argument to llvm-cov to specify the base directory. On second thought, the existing `-path-equivalence` option should make that unnecessary.

[PATCH] D69137: [profile] Do not cache __llvm_profile_get_filename result

This revision was automatically updated to reflect the committed changes. Closed by commit rG937241b0d9e8: [profile] Do not cache __llvm_profile_get_filename result (authored by vsk). Herald added projects: clang, Sanitizers. Herald added subscribers: Sanitizers, cfe-commits. Repository: rG LLV

[PATCH] D68733: Use -fdebug-compilation-dir to form absolute paths in coverage mappings

vsk added a comment. In D68733#1723734 , @liaoyuke wrote: > In D68733#1702609 , @vsk wrote: > > > Thanks, lgtm! > > > > In PR43614 I mentioned adding an extra argument to llvm-cov to specify the > > base directory.

[PATCH] D79967: Fix debug info for NoDebug attr

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. @yaxunl thanks, this patch lgtm. @dblaikie I've kicked off a thread on cfe-dev about the topics you brought up ("Design discussion re: DW_TAG_call_site support in clang") and cc'd you. CHANGES SIN

[PATCH] D80463: Debug Info: Mark os_log helper functions as artificial

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/D80463/new/ https://reviews.llvm.org/D80463 ___ cfe-commits mailing list cfe-commit

[PATCH] D60108: [os_log] Mark os_log_helper `nounwind`

vsk marked an inline comment as done. vsk added inline comments. Comment at: test/CodeGenObjCXX/os_log.mm:3 +// RUN: -fexceptions -fcxx-exceptions -O1 | FileCheck %s + +// Check that no EH cleanup is emitted around the call to __os_log_helper. ahatanak wrote: >

[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 `CodeRe

[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. 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 adjustSkippedRange(SourceManag

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

[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 '-DLLVM

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

[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 /Users/buildslave/jenkins/workspace/lldb-cmake-standalone/clang-build/lib/cma

[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] 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` crashes

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

[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 successfully.

[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 https://lists.llvm.org/cgi-bi

[PATCH] D81122: Reland: Use -fdebug-compilation-dir to form absolute paths in coverage mappings

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Lgtm. Thanks for making the addition to the command guide! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81122/new/ https://reviews.llvm.org/D81122

[PATCH] D81122: Reland: Use -fdebug-compilation-dir to form absolute paths in coverage mappings

vsk added a subscriber: phosek. vsk added a comment. In D81122#2074213 , @keith wrote: > FYI I actually removed that piece this morning since I felt like since this > now supports `-path-equivalence=.,foo` which is the "expected" behavior from > lldb, th

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

vsk updated this revision to Diff 271508. vsk added a comment. Herald added projects: clang, Sanitizers. Herald added a subscriber: Sanitizers. Rebase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71491/new/ https://reviews.llvm.org/D71491 Files:

[PATCH] D82547: [Debugify] Expose debugify (original mode) as CC1 option

vsk requested changes to this revision. vsk added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/CodeGen/BackendUtil.cpp:855 +class ClangCustomPassManager : public legacy::PassManager { +public: Please factor out OptCustom

[PATCH] D83514: [Lexer] Fix missing coverage line after #endif

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/D83514/new/ https://reviews.llvm.org/D83514

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

vsk added a comment. @zequanwu thanks for working on this. Instead of threading CommentSkipped through PPCallbacks, wdyt of taking advantage of the existing CommentHandler interface? To simplify things, we can add a static method like 'CoverageMappingGen::setupPreprocessorCallbacks(Preprocessor

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

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG8c4a65b9b2ca: [ubsan] Check implicit casts in ObjC for-in statements (authored by vsk). Repository: rG LLVM Github Mono

[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

[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| } The

[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] 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. Just

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

vsk marked 2 inline comments as done. vsk added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:1860 + CGM.getObjCRuntime().GetClass(*this, InterfaceTy->getDecl()); + Args.add(RValue::get(Cls), C.getObjCClassType()); + llvm::Value *IsClass = -

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

vsk updated this revision to Diff 273815. vsk added a comment. Use the `IsKindOfClass` CallArgList when emitting the check, and add a runtime test to ensure that an objc-cast diagnostic is not emitted on correct code. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

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

[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 `__ubsan_handle_invalid_

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

[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] 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 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" +// CHECK-unsigned-shift-base-NORECO

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

[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] 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 setParsingFunctionBody(b

[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] 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] D79337: Silence warnings when compiling x86 with latest MSVC

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, this lgtm. If you could split this up into two commits before landing it, I'd appreciate it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D793

[PATCH] D40668: [Blocks] Inherit sanitizer options from parent decl

vsk created this revision. There is no way to apply sanitizer suppressions to ObjC blocks. A reasonable default is to have blocks inherit their parent's sanitizer options. rdar://32769634 https://reviews.llvm.org/D40668 Files: lib/CodeGen/CGBlocks.cpp test/CodeGenObjC/no-sanitize.m Index

[PATCH] D40698: [ubsan] Diagnose reached-unreachable after noreturn calls

vsk created this revision. It's possible to reach an 'unreachable' instruction if a call to a noreturn function returns. Diagnose this behavior. Note: Most of the changes in this patch -- passing empty SourceLocations in places where they are either not needed or do not apply -- are NFC. Testing

[PATCH] D40698: [ubsan] Diagnose reached-unreachable after noreturn calls

vsk planned changes to this revision. vsk added a comment. Ah, I've found a problem while writing run-time tests. I'll need to take another cut at this. https://reviews.llvm.org/D40698 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://

[PATCH] D40698: [ubsan] Diagnose noreturn functions which return

vsk updated this revision to Diff 125069. vsk retitled this revision from "[ubsan] Diagnose reached-unreachable after noreturn calls" to "[ubsan] Diagnose noreturn functions which return". vsk edited the summary of this revision. vsk added a comment. - Emit the check in the noreturn function, so

[PATCH] D40700: [ubsan] Diagnose noreturn functions which return (compiler-rt)

vsk created this revision. Herald added subscribers: dberris, kubamracek. This is a test and a wording update for the clang change: https://reviews.llvm.org/D40698 https://reviews.llvm.org/D40700 Files: lib/ubsan/ubsan_handlers.cc test/ubsan/TestCases/Misc/unreachable.cpp Index: test/ubs

[PATCH] D40698: [ubsan] Diagnose noreturn functions which return

vsk updated this revision to Diff 125071. vsk added a comment. - Leave out an unrelated change in the handling of NakedAttr. https://reviews.llvm.org/D40698 Files: docs/UndefinedBehaviorSanitizer.rst lib/CodeGen/CGBuiltin.cpp lib/CodeGen/CGCall.cpp lib/CodeGen/CGExpr.cpp lib/CodeGen/C

[PATCH] D40698: [ubsan] Diagnose noreturn functions which return

vsk added inline comments. Comment at: lib/CodeGen/CGCall.cpp:2756 SourceLocation EndLoc) { + if (FI.isNoReturn()) { +// Noreturn functions don't return. efriedma wrote: > Unfortunately, this won't catch cases where

[PATCH] D40698: [ubsan] Diagnose noreturn functions which return

vsk planned changes to this revision. vsk added inline comments. Comment at: lib/CodeGen/CGCall.cpp:2756 SourceLocation EndLoc) { + if (FI.isNoReturn()) { +// Noreturn functions don't return. efriedma wrote: > vsk wr

[PATCH] D40700: [ubsan] Diagnose noreturn functions which return (compiler-rt)

vsk updated this revision to Diff 125220. vsk added a comment. - Update to test diagnostics seen after a call to the noreturn function. https://reviews.llvm.org/D40700 Files: lib/ubsan/ubsan_handlers.cc test/ubsan/TestCases/Misc/Inputs/returns-unexpectedly.c test/ubsan/TestCases/Misc/unre

[PATCH] D40698: [ubsan] Diagnose noreturn functions which return

vsk updated this revision to Diff 125221. vsk edited the summary of this revision. vsk added a comment. - Diagnose in the scenario Eli pointed out, by stripping the 'noreturn' attribute and emitting a check after the call. - Test updates. https://reviews.llvm.org/D40698 Files: docs/Undefined

[PATCH] D40940: [ubsan] Use pass_object_size info in bounds checks

vsk created this revision. Herald added a reviewer: george.burgess.iv. Teach UBSan's bounds check to opportunistically use pass_object_size information to check array accesses. rdar://33272922 https://reviews.llvm.org/D40940 Files: lib/CodeGen/CGExpr.cpp lib/CodeGen/CodeGenFunction.h tes

[PATCH] D40941: [ubsan] Use pass_object_size info in bounds checks (compiler-rt)

vsk created this revision. Herald added subscribers: dberris, kubamracek. This is a test update for the clang change in https://reviews.llvm.org/D40940 https://reviews.llvm.org/D40941 Files: test/ubsan/TestCases/Misc/bounds.cpp Index: test/ubsan/TestCases/Misc/bounds.cpp ===

[PATCH] D40940: [ubsan] Use pass_object_size info in bounds checks

vsk updated this revision to Diff 126037. vsk added a comment. Thanks for your feedback. - Give up on 0-sized types. - Give up on pass_object_size(2 | 3). https://reviews.llvm.org/D40940 Files: lib/CodeGen/CGExpr.cpp lib/CodeGen/CodeGenFunction.h test/CodeGen/ubsan-pass-object-size.c In

[PATCH] D40940: [ubsan] Use pass_object_size info in bounds checks

vsk updated this revision to Diff 126064. vsk added a comment. - Handle constant size modifiers while we're at it (e.g "int foo(int p[static 10])"). https://reviews.llvm.org/D40940 Files: lib/CodeGen/CGExpr.cpp lib/CodeGen/CodeGenFunction.h test/CodeGen/ubsan-pass-object-size.c Index: t

[PATCH] D40668: [Blocks] Inherit sanitizer options from parent decl

vsk added a comment. Friendly ping. https://reviews.llvm.org/D40668 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40941: [ubsan] Use pass_object_size info in bounds checks (compiler-rt)

vsk added inline comments. Comment at: test/ubsan/TestCases/Misc/bounds.cpp:9 +int get_int(int *const p __attribute__((pass_object_size(0))), int i) { + // CHECK-A-2: bounds.cpp:[[@LINE+1]]:10: runtime error: index 2 out of bounds for type 'int *' + return p[i]; --

[PATCH] D40668: [Blocks] Inherit sanitizer options from parent decl

vsk added a comment. In https://reviews.llvm.org/D40668#949096, @zaks.anna wrote: > LGTM. > > Thanks! > > I was wondering if there are other places where this propagation needs to be > added, for example, other calls to GenerateBlockFunction. Thanks for the review :). Yes there is one other si

[PATCH] D40941: [ubsan] Use pass_object_size info in bounds checks (compiler-rt)

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

[PATCH] D40940: [ubsan] Use pass_object_size info in bounds checks

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

[PATCH] D40940: [ubsan] Use pass_object_size info in bounds checks

vsk added a comment. I backed out the part of this patch which deals with array parameters declared like p[10] or p[static 10]: r320185. Comment at: lib/CodeGen/CGExpr.cpp:833 + // Arrays don't have pass_object_size attributes, but if they have a constant + // size modifier

[PATCH] D41044: Implementation of -fextend-lifetimes and -fextend-this-ptr to aid with debugging of optimized code

vsk added a project: debug-info. vsk added a comment. Thanks for your patches @wolfgangp! I agree with Eli that we should evaluate enabling this automatically with -Og. I'll test this out on a few internal projects and report back. https://reviews.llvm.org/D41044 ___

[PATCH] D41140: [Coverage] Always emit unused coverage mappings in the same order.

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

[PATCH] D41149: [CodeGen] Specialize mixed-sign mul-with-overflow (fix PR34920)

vsk created this revision. vsk added reviewers: efriedma, rjmccall, dtzWill. This patch introduces a specialized way to lower overflow-checked multiplications with mixed-sign operands. This fixes link failures and ICEs on code like this: void mul(int64_t a, uint64_t b) { int64_t res; __

[PATCH] D38861: [CodeGen] Error on unsupported checked multiplies early

vsk abandoned this revision. vsk added a comment. Abandoned in favor of a proper fix: https://reviews.llvm.org/D41149 https://reviews.llvm.org/D38861 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listi

[PATCH] D41149: [CodeGen] Specialize mixed-sign mul-with-overflow (fix PR34920)

vsk updated this revision to Diff 12. vsk added a comment. - Make sure the result can be stored into the result ptr. https://reviews.llvm.org/D41149 Files: lib/CodeGen/CGBuiltin.cpp test/CodeGen/builtins-overflow.c Index: test/CodeGen/builtins-overflow.c ===

[PATCH] D41149: [CodeGen] Specialize mixed-sign mul-with-overflow (fix PR34920)

vsk added inline comments. Comment at: test/CodeGen/builtins-overflow.c:402 + int result; + if (__builtin_mul_overflow(y, x, &result)) +return LongLongErrorCode; efriedma wrote: > I think the rules for __builtin_mul_overflow say you have to check whether >

[PATCH] D41149: [CodeGen] Specialize mixed-sign mul-with-overflow (fix PR34920)

vsk updated this revision to Diff 126854. vsk marked an inline comment as done. vsk edited the summary of this revision. vsk added a comment. - Handle unsigned result types. - Extend the test driver to validate 54 different combinations of signed, unsigned, and result types: https://gist.github

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

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

[PATCH] D44672: [CodeGen] Disable UBSan for coroutine functions

vsk added a comment. In D44672#1637891 , @leonardchan wrote: > It seems that this test leads to an `UNREACHABLE` under the new pass manager > (can check this by adding `-fexperimental-new-pass-manager` to the test. I > think this is because the passes fo

[PATCH] D66493: [NewPM] Run ubsan-coroutines test under the legacy pass manager only

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/D66493/new/ https://reviews.llvm.org/D66493

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

vsk added inline comments. Comment at: clang/docs/UndefinedBehaviorSanitizer.rst:177 ``-fsanitize=undefined``. + - ``-fsanitize=pointer-offsetting``: Enables ``nullptr-and-nonzero-offset`` + and ``pointer-overflow``. Why does this need to be a separat

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

vsk added inline comments. Comment at: clang/docs/UndefinedBehaviorSanitizer.rst:177 ``-fsanitize=undefined``. + - ``-fsanitize=pointer-offsetting``: Enables ``nullptr-and-nonzero-offset`` + and ``pointer-overflow``. lebedev.ri wrote: > vsk wrote: > >

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

vsk added a subscriber: dtzWill. vsk added a comment. Thanks, this is looking pretty good. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4538 + llvm::Value * /*OffsetOverflows*/> +EmitGEPOffsetInBytes(Value *BasePtr, Value *GEPVal, + llvm::L

<    1   2   3   4   5   6   7   >