[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)

vsk marked an inline comment as done. vsk added a comment. In https://reviews.llvm.org/D29369#665878, @filcab wrote: > Why the switch to `if` instead of a fully-covered switch/case? It lets me avoid repeating two function calls: switch (CGF.getLangOpts().getSignedOverflowBehavior()) { case

[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)

vsk marked an inline comment as done. vsk added a comment. In https://reviews.llvm.org/D29369#666308, @vsk wrote: > In https://reviews.llvm.org/D29369#665878, @filcab wrote: > > > Why the switch to `if` instead of a fully-covered switch/case? > > > It lets me avoid repeating two function calls:

[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)

vsk updated this revision to Diff 87030. vsk edited the summary of this revision. vsk added a comment. - Use switches per Filipe's comment, and fix a comment in the test case. https://reviews.llvm.org/D29369 Files: lib/CodeGen/CGExprScalar.cpp test/CodeGen/compound-assign-overflow.c test/

[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)

vsk created this revision. Given a load of a member variable from an instance method ('this->x'), ubsan inserts a null check for 'this', and another null check for '&this->x', before allowing the load to occur. Both of these checks are redundant, because 'this' must have been null-checked before t

[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)

vsk updated this revision to Diff 87334. vsk edited the summary of this revision. vsk added a comment. @arphaman thank you for the feedback! Per Alex's comments: - Add test for explicit use of the 'this' pointer. - Handle cases where the 'this' pointer may be casted (implicitly, or otherwise).

[PATCH] D29723: [Sema] Add lvalue-to-rvalue cast in direct-list-initialization of enum

vsk created this revision. After r264564, we allowed direct-list-initialization of an enum from an integral value in C++1z mode, so long as that value can convert to the enum's underlying type. In this kind of initialization, we need a lvalue-to-rvalue conversion for the initializer value if it i

[PATCH] D34680: clang-cl crashes with -fprofile-instr-use flag

vsk added a comment. Do you know which function clang was processing when it crashed? That would help us find a test case. https://reviews.llvm.org/D34680 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman

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

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks for the patch! LGTM. Comment at: lib/CodeGen/CoverageMappingGen.cpp:686 +// FIXME: a break in a switch should terminate regions for all preceding +// case statements

[PATCH] D34886: Add a fixit for -Wobjc-protocol-property-synthesis

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. lgtm with a nitpick. Comment at: include/clang/Sema/Sema.h:3351 + ObjCInterfaceDecl *IDecl, SourceRange AtEnd); + void DefaultSynthesizeProperti

[PATCH] D34680: clang-cl crashes with -fprofile-instr-use flag

vsk added a comment. Thanks for the stack trace. Clang shouldn't ever be assigning counters to decls without bodies. As far as I can tell, this has only been happening when the microsoft ABI is in use, which may explain why we don't hit the issue more often. The fix required changing the way we

[PATCH] D34981: RecursiveASTVisitor should visit the nested name qualifiers in a template specialisation

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. I haven't touched any AST code but this looks good to me: it's in line with what's done in TraverseRecordHelper and the test case is comprehensive. Comment at: unittests/Tooling/R

[PATCH] D34301: [Sema] Make sure the definition of a referenced virtual function is emitted when it is final

vsk added a comment. I suggest changing the API for 'canDevirtualizeCall' a bit but this is looking great overall. The code movement and test case changes look good. Comment at: include/clang/AST/DeclCXX.h:1902 + bool canDevirtualizeCall(const Expr *Base, bool IsAppleKext, +

[PATCH] D34121: [ubsan] Teach the pointer overflow check that "p - <= p" (PR33430)

vsk added a comment. @dtzWill do you have any further comments on this one? I'd like to get another 'lgtm' before committing, and it'd be nice to get this in before llvm 5.0 branches (7/19). FWIW we've been living on this for a few weeks internally without any issues: https://github.com/apple/s

[PATCH] D34301: [Sema] Make sure the definition of a referenced virtual function is emitted when it is final

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, this looks great. https://reviews.llvm.org/D34301 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman

[PATCH] D34121: [ubsan] Teach the pointer overflow check that "p - <= p" (PR33430)

vsk added a comment. In https://reviews.llvm.org/D34121#808486, @dtzWill wrote: > In https://reviews.llvm.org/D34121#806978, @vsk wrote: > > > @dtzWill do you have any further comments on this one? > > > > I'd like to get another 'lgtm' before committing, and it'd be nice to get > > this in befo

[PATCH] D35385: [Driver] Darwin: Link in the profile runtime archive first

vsk created this revision. While building a project with code coverage enabled, we can link in dependencies which export a weak definition of __llvm_profile_filename. After r306710, linking in the profiling runtime could pull in a weak definition of this symbol from a dependency, instead of from

[PATCH] D35212: [Index] Prevent canonical decl becoming nullptr

vsk added subscribers: arphaman, vsk. vsk added a comment. Do you have a test case? https://reviews.llvm.org/D35212 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35338: Add the -fdestroy-globals flag

vsk added a comment. This is interesting. Do you have any results/metrics to share (e.g some any binary size reduction for projects you've looked at)? Comment at: lib/Frontend/CompilerInvocation.cpp:585 Opts.CXXCtorDtorAliases = Args.hasArg(OPT_mconstructor_aliases); + Opt

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

vsk added a comment. 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 any way to finalize an ASTInfoCollector after ReadAST happens (or ASTReaderListeners in general)? https://reviews.llvm.org/

[PATCH] D35212: [Index] Prevent canonical decl becoming nullptr

vsk added a comment. Take a look at the tests in clang/test/Index/Core. You should be able to write a test which uses c-index-test to dump the symbols in this source. https://reviews.llvm.org/D35212 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D35212: [Index] Prevent canonical decl becoming nullptr

vsk added a comment. LGTM, thank you! (Let me know if you need someone to commit this for you.) https://reviews.llvm.org/D35212 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35338: Add the -fdestroy-globals flag

vsk added a subscriber: bruno. vsk added a comment. That seems like a nice win and I like the convenience of this approach. That said I've just remembered that there's a thread on cfe-dev about this: [RFC] Suppress C++ static destructor registration I don't think a consensus was reached. From wh

[PATCH] D32047: [Driver] Add support for default UBSan blacklists

vsk added a comment. I'd like to address the current problems with sanitizer blacklists before moving on to this patch: https://reviews.llvm.org/D32842 The summary is that the entries in a blacklist file apply to all sanitizers, resulting in potential false negatives when multiple default black

[PATCH] D35735: [ubsan] Null-check pointers in -fsanitize=vptr (PR33881)

vsk created this revision. The instrumentation generated by -fsanitize=vptr does not null check a user pointer before loading from it. This causes crashes in the face of UB member calls (this=nullptr), i.e it causes user programs to crash only after UBSan is turned on. The fix is to make run-time

[PATCH] D35736: [ubsan] -fsanitize=vptr now requires -fsanitize=null, update tests

vsk created this revision. Herald added a subscriber: kubamracek. See: https://bugs.llvm.org/show_bug.cgi?id=33881 Depends on https://reviews.llvm.org/D35735 https://reviews.llvm.org/D35736 Files: test/ubsan/TestCases/TypeCheck/PR33221.cpp test/ubsan/TestCases/TypeCheck/vptr-corrupted-vtab

[PATCH] D35729: [Frontend] - Mark some ASTUnit methods as const

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

[PATCH] D35735: [ubsan] Null-check pointers in -fsanitize=vptr (PR33881)

vsk updated this revision to Diff 107741. vsk marked an inline comment as done. vsk added a comment. - Drop 'REQUIRES: asserts'. https://reviews.llvm.org/D35735 Files: docs/UndefinedBehaviorSanitizer.rst include/clang/Basic/DiagnosticDriverKinds.td include/clang/Basic/DiagnosticGroups.td

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

vsk added a reviewer: vsk. vsk added a comment. This won't do the right thing if more than one sanitizer with a default blacklist is enabled. It's also problematic that a default blacklist for one sanitizer can blacklist code for a different sanitizer. See: https://reviews.llvm.org/D32043 https

[PATCH] D35735: [ubsan] Null-check pointers in -fsanitize=vptr (PR33881)

vsk marked an inline comment as done. vsk added a comment. I made the suggested test changes and updated the release notes: r309007 Repository: rL LLVM https://reviews.llvm.org/D35735 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:/

[PATCH] D35736: [ubsan] -fsanitize=vptr now requires -fsanitize=null, update tests

vsk closed this revision. vsk added a comment. Thanks! This landed in r309008. https://reviews.llvm.org/D35736 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

vsk added a comment. Hi Eli, are you waiting on something before landing this? Repository: rL LLVM https://reviews.llvm.org/D34801 ___ 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 created this revision. The current coverage implementation doesn't handle region termination very precisely. Take for example an `if' statement with a `return': void f() { if (true) { return; // The `if' body's region is terminated here. } // This line gets the same covera

[PATCH] D41976: Low-hanging fruit optimization in string::__move_assign().

vsk added reviewers: EricWF, mclow.lists, hiraditya. vsk added a comment. Adding some folks who may be interested. https://reviews.llvm.org/D41976 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo

[PATCH] D41921: [Parse] Forward brace locations to TypeConstructExpr

vsk updated this revision to Diff 130021. vsk marked 2 inline comments as done. vsk added a comment. - Address feedback from @aaron.ballman https://reviews.llvm.org/D41921 Files: include/clang/AST/ExprCXX.h include/clang/Sema/Initialization.h include/clang/Sema/Sema.h lib/CodeGen/Covera

[PATCH] D42043: c-index: CXString: fix MSAN read-past-end bug

vsk added reviewers: akyrtzi, benlangmuir. vsk added inline comments. Comment at: tools/c-index-test/c-index-test.c:3268 - filename = clang_getFileName(file); - index_data->main_filename = clang_getCString(filename); - clang_disposeString(filename); + index_data->main_filen

[PATCH] D42043: c-index: CXString: fix MSAN read-past-end bug

vsk added a comment. Thanks for working on this :). Comment at: tools/libclang/CXString.cpp:213 + if (string.IsNullTerminated) { +CString = (const char *) string.Contents; + } else { elsteveogrande wrote: > vsk wrote: > > Basic question: If a non-owning C

[PATCH] D42259: c-index-test: small fix to CXString handling and disposal

vsk added a comment. LGTM, for the record Repository: rC Clang https://reviews.llvm.org/D42259 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42043: c-index: CXString: fix MSAN read-past-end bug

vsk added a comment. Mind rebasing this when you have a chance? Repository: rC Clang https://reviews.llvm.org/D42043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42043: c-index: CXString: fix MSAN read-past-end bug

vsk added a reviewer: arphaman. vsk added a comment. In https://reviews.llvm.org/D42043#981409, @elsteveogrande wrote: > Fixes, but first, a question for reviewers: > > Looking at the description of `clang_disposeString`: > > /** >* \brief Free the given string. >*/ > CINDEX_LINKAGE v

[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)

vsk added a comment. In https://reviews.llvm.org/D29369#672166, @dtzWill wrote: > After some thought, can we discuss why this is a good idea? The goal is to lower ubsan's compile-time + instrumentation overhead at -O0, since this reduces the friction of debugging a ubsan-instrumented project.

[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)

vsk added a comment. Ah, I did miss ParenExpr. Maybe it would be better to use Expr::isImplicitCXXThis, since it handles this case and some more. Later, we can go back and see if it's feasible to handle static/const casts in isImplicitCXXThis to catch more cases. In https://reviews.llvm.org/D2

[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)

vsk updated this revision to Diff 88075. vsk edited the summary of this revision. vsk added a reviewer: arphaman. vsk added a comment. - Check 'this' once per method. This supports the partial sanitization use-case. - Flesh out the predicate for avoiding null checks on object pointers. I couldn't

[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)

vsk marked 2 inline comments as done. vsk added inline comments. Comment at: test/CodeGenCXX/ubsan-suppress-null-checks.cpp:8 + int load_member() { +// CHECK: call void @__ubsan_handle_type_mismatch +// CHECK-NOT: call void @__ubsan_handle_type_mismatch

[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)

vsk updated this revision to Diff 88259. vsk marked an inline comment as done. vsk added a comment. - Tighten up the tests per Alex's suggestion. https://reviews.llvm.org/D29530 Files: lib/CodeGen/CGExpr.cpp lib/CodeGen/CGExprCXX.cpp lib/CodeGen/CodeGenFunction.cpp lib/CodeGen/CodeGenFu

[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)

vsk added a comment. In https://reviews.llvm.org/D29369#676521, @dtzWill wrote: > In https://reviews.llvm.org/D29369#673064, @vsk wrote: > > > In https://reviews.llvm.org/D29369#672166, @dtzWill wrote: > > > > > After some thought, can we discuss why this is a good idea? > > > > > > The goal is t

[PATCH] D29723: [Sema] Add lvalue-to-rvalue cast in direct-list-initialization of enum

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

[PATCH] D30131: [profiling] Don't skip non-base constructors if there is a virtual base (fixes PR31992)

vsk created this revision. We don't assign profile counters to non-base constructors. That results in a loss coverage for constructors in classes with virtual bases, which are complete constructors. Make an exception for non-base constructors in classes with virtual bases. I checked that we get

[PATCH] D30131: [profiling] PR31992: Don't skip interesting non-base constructors

vsk updated this revision to Diff 89151. vsk retitled this revision from "[profiling] Don't skip non-base constructors if there is a virtual base (fixes PR31992)" to "[profiling] PR31992: Don't skip interesting non-base constructors". vsk edited the summary of this revision. vsk added a comment.

[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)

vsk added a comment. Ping, is the argument in favor of making the change in my last comment satisfactory? https://reviews.llvm.org/D29369 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com

[PATCH] D30283: [ubsan] Reduce alignment checking of C++ object pointers

vsk created this revision. This patch teaches ubsan to insert an alignment check for the 'this' pointer at the start of each method/lambda. This allows clang to emit significantly fewer alignment checks overall, because if 'this' is aligned, so are its fields. This is essentially the same thing r

[PATCH] D30285: [ubsan] Don't check alignment if the alignment is 1

vsk created this revision. If a pointer is 1-byte aligned, there's no use in checking its alignment. Somewhat surprisingly, ubsan can spend a significant amount of time doing just that! This loosely depends on https://reviews.llvm.org/D30283. Testing: check-clang, check-ubsan, and a stage2 ubsan

[PATCH] D30131: [profiling] PR31992: Don't skip interesting non-base constructors

vsk added a comment. In https://reviews.llvm.org/D30131#684310, @arphaman wrote: > LGTM. > > One point to note, when we are displaying coverage for constructors that have > both the base and complete versions instrumented, e.g.: > > class Foo { > public: > Foo() { } > }; > > clas

[PATCH] D30345: [CodeGen][Blocks] Refactor capture handling in code that generates block copy/destroy routines

vsk added a comment. Looks NFC to me. Comment at: lib/CodeGen/CGBlocks.cpp:1414 + +} // end anonymous namespace + I don't see the need for two GenericInfo types (although it's plausible it'll make sense with your upcoming changes!). I had in mind a single 'enu

[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)

vsk updated this revision to Diff 89733. vsk added a comment. - Make the suggested readability improvements, and fix a comment in the test case. https://reviews.llvm.org/D29369 Files: lib/CodeGen/CGExprScalar.cpp test/CodeGen/compound-assign-overflow.c test/CodeGen/ubsan-promoted-arith.c

[PATCH] D29437: [ubsan] Detect signed overflow UB in remainder operations

vsk updated this revision to Diff 89734. vsk added a comment. - Add a small test that shows why the 'isIntegerType' check is required (we'd crash otherwise). https://reviews.llvm.org/D29437 Files: lib/CodeGen/CGExprScalar.cpp test/CodeGen/ubsan-promoted-arith.cpp Index: test/CodeGen/ubsa

[PATCH] D27455: UBSan docs: Explicitly mention that `-fsanitize=unsigned-integer-overflow` does not catch UB.

vsk added a comment. LGTM, fwiw. (IIRC kcc mentioned that samsonov no longer works on the sanitizers.) https://reviews.llvm.org/D27455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commit

[PATCH] D30423: [ubsan] Detect UB loads from bitfields

vsk created this revision. We frequently run into user bugs caused by UB loads of out-of-range values from enum / BOOL bitfields. Teach UBSan to diagnose the issue. https://reviews.llvm.org/D30423 Files: lib/CodeGen/CGAtomic.cpp lib/CodeGen/CGExpr.cpp lib/CodeGen/CodeGenFunction.h test

[PATCH] D30599: [ubsan] Extend the nonnull argument check to ObjC

vsk created this revision. UBSan's nonnull argument check applies when a parameter has the "nonnull" attribute. The check currently works for FunctionDecls, but not for ObjCMethodDecls. This patch extends the check to work for ObjC. To do this, I introduced a new AbstractCallee class to represent

[PATCH] D30599: [ubsan] Extend the nonnull argument check to ObjC

vsk added a comment. In https://reviews.llvm.org/D30599#692210, @jroelofs wrote: > Can the null check be performed in the callee? Yes, but I think that would result in perplexing diagnostics, because we wouldn't be able to report the source location of the buggy calls. > That'd make this chec

[PATCH] D30599: [ubsan] Extend the nonnull argument check to ObjC

vsk marked an inline comment as done. vsk added a comment. Thanks for the reviews! Comment at: lib/CodeGen/CodeGenFunction.h:308 + /// \brief An abstract representation of regular/ObjC call/message targets. + class AbstractCallee { aprantl wrote: > The \bri

[PATCH] D30423: [ubsan] Detect UB loads from bitfields

vsk added a comment. Thanks for the feedback! Comment at: test/CodeGenCXX/ubsan-bitfields.cpp:21 + // CHECK: call void @__ubsan_handle_load_invalid_value + return s->e1; +} arphaman wrote: > Can we avoid the check if the bitfield is 2 bits wide? I don't think

[PATCH] D30423: [ubsan] Detect UB loads from bitfields

vsk added a comment. In https://reviews.llvm.org/D30423#694003, @jroelofs wrote: > I think this might miss loads from bitfield ivars. I'll add a test that shows that this case is covered. > Also, what about the conversion that happens for properties whose backing > ivar is a bitfield? (or doe

[PATCH] D30423: [ubsan] Detect UB loads from bitfields

vsk updated this revision to Diff 90869. vsk added a comment. - Improve objc test coverage per Jon's suggestions. https://reviews.llvm.org/D30423 Files: lib/CodeGen/CGAtomic.cpp lib/CodeGen/CGExpr.cpp lib/CodeGen/CodeGenFunction.h test/CodeGenCXX/ubsan-bitfields.cpp test/CodeGenObjC/u

[PATCH] D30423: [ubsan] Detect UB loads from bitfields

vsk added inline comments. Comment at: test/CodeGenCXX/ubsan-bitfields.cpp:21 + // CHECK: call void @__ubsan_handle_load_invalid_value + return s->e1; +} vsk wrote: > arphaman wrote: > > Can we avoid the check if the bitfield is 2 bits wide? > I don't think so,

[PATCH] D30729: [ubsan] Skip range checks for width-limited values

vsk created this revision. UBSan can check that scalar loads provide in-range values. When we load a value from a bitfield, we know that the range of the value is constrained by the bitfield's width. This patch teaches UBSan how to use that information to skip emitting some range checks. This dep

[PATCH] D30729: [ubsan] Skip range checks for width-limited values

vsk updated this revision to Diff 90975. vsk added a comment. - Fix an incorrect comment on the field "e2" in struct S. We emit a check for it because 0b11 = -1 = 3. - Skip the range check on the field "e2" in struct S2, because the range of the bitfield is the same as the range clang infers for

[PATCH] D30423: [ubsan] Detect UB loads from bitfields

vsk added inline comments. Comment at: test/CodeGenObjC/ubsan-bool.m:26 + // OBJC: [[ICMP:%.*]] = icmp ule i8 [[ASHR]], 1, !nosanitize + // OBJC: call void @__ubsan_handle_load_invalid_value + filcab wrote: > jroelofs wrote: > > vsk wrote: > > > jroelofs wrote

[PATCH] D30729: [ubsan] Skip range checks for width-limited values

vsk added a comment. In https://reviews.llvm.org/D30729#695463, @arphaman wrote: > You should also add a test for `enum E : unsigned`. Ubsan doesn't try to infer the range of enums with a fixed, underlying type. I'll add a test case to make sure we don't insert a check. Com

[PATCH] D30729: [ubsan] Skip range checks for width-limited values

vsk updated this revision to Diff 91030. vsk added a comment. - Make check-not's a bit stricter per Filipe's comment. - Add a negative test for fixed enums. https://reviews.llvm.org/D30729 Files: lib/CodeGen/CGExpr.cpp lib/CodeGen/CodeGenFunction.h test/CodeGenCXX/ubsan-bitfields.cpp Ind

[PATCH] D30423: [ubsan] Detect UB loads from bitfields

vsk added a comment. In https://reviews.llvm.org/D30423#695608, @filcab wrote: > LGTM since my issue is only an issue on ObjC platforms and it seems those are > the semantics it should have there. Thanks for the review. > P.S: If it documented that the only possible values for BOOL are YES an

[PATCH] D30762: [ubsan] Add a nullability sanitizer

vsk created this revision. Teach UBSan how to detect violations of the _Nonnull annotation when passing arguments to callees, in assignments, and in return stmts. Because _Nonnull does not affect IRGen, the new checks are disabled by default. The new driver flags are: -fsanitize=nullability-ar

[PATCH] D30762: [ubsan] Add a nullability sanitizer

vsk updated this revision to Diff 91100. vsk added reviewers: aprantl, arphaman. vsk added a comment. - Improve the wording of the docs. - Drop a weak test from 'ubsan-null-retval.m'. https://reviews.llvm.org/D30762 Files: docs/UndefinedBehaviorSanitizer.rst include/clang/Basic/Sanitizers.d

[PATCH] D30762: [ubsan] Add a nullability sanitizer

vsk marked 8 inline comments as done. vsk added a comment. Thanks for your comments, and sorry for jumping the gun earlier with an updated diff. I'll attach a fixed-up diff shortly. Comment at: lib/CodeGen/CGDecl.cpp:1911 +if (auto Nullability = Ty->getNullability(getConte

[PATCH] D30762: [ubsan] Add a nullability sanitizer

vsk updated this revision to Diff 91180. vsk added a comment. - Add a test for the mixed _Nonnull arg + __attribute__((nonnull)) arg case. - Reword docs per Adrian's comments, fix up doxygen comments, add better code comments, drop redundant "Nullability" truthiness checks. https://reviews.llvm

[PATCH] D30762: [ubsan] Add a nullability sanitizer

vsk added a comment. One question for Anna (inline). I will update the diff with the documentation/code comments/renaming fixes once I hear back. Thanks again for the comments! Comment at: docs/UndefinedBehaviorSanitizer.rst:101 + ``-fsanitize=nullability-assign``, and th

[PATCH] D30762: [ubsan] Add a nullability sanitizer

vsk marked 9 inline comments as done. vsk added a comment. The plan is to start off with -fsanitize=nullability, and then create a nullability-pedantic group later if it's really necessary. I think I've addressed all of the inline comments, and will upload a new diff shortly.

[PATCH] D30762: [ubsan] Add a nullability sanitizer

vsk updated this revision to Diff 91382. vsk marked 4 inline comments as done. vsk added a comment. - Rework documentation, add better code comments, and tighten up some check lines. https://reviews.llvm.org/D30762 Files: docs/UndefinedBehaviorSanitizer.rst include/clang/Basic/Sanitizers.d

[PATCH] D30283: [ubsan] Reduce alignment checking of C++ object pointers

vsk added a reviewer: rsmith. vsk added a comment. Ping. I appreciate that there are a lot of test changes to sift through here -- please let me know if I can make the patch easier to review in any way. https://reviews.llvm.org/D30283 ___ cfe-commi

[PATCH] D21695: [clang] Version support for UBSan handlers

vsk accepted this revision. vsk added a reviewer: vsk. vsk added a comment. This revision is now accepted and ready to land. Thanks for working on this. LGTM with a nit. Comment at: lib/CodeGen/CGExpr.cpp:2506 + assert(CheckHandler >= 0 && + CheckHandler < sizeof(Sanit

[PATCH] D27180: Testbed and skeleton of a new expression parser

vsk added a comment. Thanks for working on this! I'm happy with the direction of this patch. It makes sense that clang would have a tool to help test AST reconstruction from 'external' sources. As you pointed out, it provides a good avenue for clients of the clang AST's to get better test cover

[PATCH] D27180: Testbed and skeleton of a new expression parser

vsk added a comment. Looks good. Apart from some nitpicks, I'm happy with this. This isn't really my area so it'd be good to get an explicit lgtm from one of the reviewers. Comment at: tools/clang-import-test/CMakeLists.txt:7 + clang-import-test.cpp + ) + @b

[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}

vsk created this revision. vsk added a reviewer: ahatanak. vsk added subscribers: kubabrecka, cfe-commits. On some Apple platforms, the ObjC BOOL type is defined as a signed char. When performing instrumentation for -fsanitize=bool, we'd like to treat the range of BOOL like it's always {0, 1}. Whi

[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}

vsk marked 3 inline comments as done. vsk added a comment. Thanks for your feedback. I will updated the patch shortly. Comment at: lib/CodeGen/CGExpr.cpp:1221 +/// Check if \p Ty is defined as BOOL in a system header. In ObjC language +/// modes, it's safe to treat such a type

[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}

vsk updated this revision to Diff 80960. vsk marked 3 inline comments as done. vsk added a comment. - Use NSAPI's 'is-BOOL' predicate. - Simplify test. https://reviews.llvm.org/D27607 Files: lib/CodeGen/CGExpr.cpp test/CodeGenObjC/ubsan-bool.m Index: test/CodeGenObjC/ubsan-bool.m

[PATCH] D32043: [Driver] Load all necessary default sanitizer blacklists

vsk added a comment. In https://reviews.llvm.org/D32043#728427, @pcc wrote: > This seems reasonable to me, although it's unfortunate that the design of the > sanitizer blacklist feature does not (at present) allow different blacklists > for different sanitizers. IMO this might be a real probl

[PATCH] D32406: [Coverage][Windows] Null pointer dereference in CodeGenPGO::skipRegionMappingForDecl (fixes PR32761)

vsk added a comment. Thanks for your patch! I have a few requests. First, please split off the SkipCoverageMapping change from this patch. If you don't have commit access yet, let me know and I can commit it for you. You can also ping Chris L for commit access. Second, the test can be minimize

[PATCH] D32043: [Driver] Load all necessary default sanitizer blacklists

vsk added a comment. Ok. I will take a step back and see what it will take to implement per-sanitizer blacklists. https://reviews.llvm.org/D32043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo

[PATCH] D32406: [Coverage][Windows] Null pointer dereference in CodeGenPGO::skipRegionMappingForDecl (fixes PR32761)

vsk added a comment. > Are you sure you want to do that? This is the only use of this boolean. IMO removing 'SkipCoverageMapping' isn't related to fixing the original bug. Since it's simple enough to make it a separate change, I thought it'd be best. In https://reviews.llvm.org/D32406#735847, @

[PATCH] D32456: [ubsan] Make the alignment check work with some extern decls (PR32630)

vsk created this revision. UBSan's alignment check does not detect unaligned loads and stores from extern struct declarations. Example: struct S { int i; }; extern struct S g_S; // &g_S may not be aligned properly. int main() { return g_S.i; // No alignment check for &g_S.i is emitte

[PATCH] D32456: [ubsan] Make the alignment check work with some extern decls (PR32630)

vsk added a comment. In https://reviews.llvm.org/D32456#736120, @efriedma wrote: > > Note: it would still not be able to catch the following case -- > > Do you know why? As-written, the alignment check doesn't apply to loads and stores on non-pointer POD types. The reason why is probably that

[PATCH] D32456: [ubsan] Make the alignment check work with some extern decls (PR32630)

vsk added a comment. In https://reviews.llvm.org/D32456#737228, @efriedma wrote: > > If the alignment isn't explicitly set, it is initialized to 0. > > That's what I thought. I don't like this; clang should explicitly set an > alignment for every global. Ok, I will set this aside for now and

[PATCH] D32456: [ubsan] Make the alignment check work with some extern decls (PR32630)

vsk added a comment. In https://reviews.llvm.org/D32456#737273, @efriedma wrote: > Err, that's not what I meant... > > If the alignment of a global in LLVM IR is "zero", it doesn't really mean > zero. It means "guess the alignment I want based on the specified type of > the global". And that'

[PATCH] D32519: [Sema] Avoid using a null type pointer (fixes PR32750)

vsk created this revision. isMicrosoftMissingTypename() uses a Type pointer without first checking that it's non-null. PR32750 reports a case where the pointer is in fact

[PATCH] D32519: [Sema] Avoid using a null type pointer (fixes PR32750)

vsk added inline comments. Comment at: test/SemaCXX/MicrosoftExtensions.cpp:516 +template struct A {}; +template struct B : A > { A::C::D d; }; // expected-error {{missing 'typename' prior to dependent type name 'A::C::D'}} +} nikola wrote: > nitpick: you don't

[PATCH] D28867: [Profile] Add off-by-default -Wprofile-instr-missing warning

vsk updated this revision to Diff 96847. vsk retitled this revision from "[Profile] Warn about out-of-date profiles only when there are mismatches" to "[Profile] Add off-by-default -Wprofile-instr-missing warning". vsk edited the summary of this revision. vsk added a reviewer: davidxl. vsk added

[PATCH] D32724: [Modules] Include the enabled sanitizers in the module hash

vsk created this revision. When building with implicit modules it's possible to hit a scenario where modules are built without -fsanitize=address, and are subsequently imported into CUs with -fsanitize=address enabled. This can cause strange failures at runtime. One case I've seen affects libcxx,

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

vsk created this revision. Sanitizer blacklists currently apply to all enabled sanitizers. E.g if multiple sanitizers are enabled, it isn't possible to specify a blacklist for one sanitizer and not another. This makes it impossible to load default blacklists for more than one sanitizer, because d

[PATCH] D32043: [Driver] Load all necessary default sanitizer blacklists

vsk abandoned this revision. vsk added a comment. Obsoleted by: https://reviews.llvm.org/D32842 https://reviews.llvm.org/D32043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

vsk added reviewers: kcc, kubamracek. vsk added a subscriber: zaks.anna. vsk added a comment. @zaks.anna suggested some more reviewers who may be interested. https://reviews.llvm.org/D32842 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

[PATCH] D32724: [Modules] Include the enabled sanitizers in the module hash

vsk updated this revision to Diff 98030. vsk edited the summary of this revision. vsk added a comment. - Exclude sanitizers which cannot affect AST generation from the module hash. - Improve the test to check modules are actually rebuilt when we expect them to be rebuilt, and not rebuilt otherwis

<    1   2   3   4   5   6   7   >