[PATCH] D34262: [ubsan] PR33081: Skip the standard type checks for volatile

vsk added a comment. Thanks for the review. I'll make the suggested test changes and commit. https://reviews.llvm.org/D34262 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34262: [ubsan] PR33081: Skip the standard type checks for volatile

vsk created this revision. Skip checks for null dereference, alignment violation, object size violation, and dynamic type violation if the pointer points to volatile data. https://bugs.llvm.org/show_bug.cgi?id=33081 https://reviews.llvm.org/D34262 Files: lib/CodeGen/CGExpr.cpp

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

vsk updated this revision to Diff 102603. vsk marked an inline comment as done. vsk added a comment. Address Adrian's comment about using an enum to simplify some calls. https://reviews.llvm.org/D34121 Files: lib/CodeGen/CGExpr.cpp lib/CodeGen/CGExprScalar.cpp

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

vsk added a comment. In https://reviews.llvm.org/D34121#779347, @dtzWill wrote: > Don't mean to block this, but just FYI I won't be able to look into this > carefully until later this week (sorry!). > > Kicked off a rebuild using these patches just now, though! O:) No problem, thanks for

[PATCH] D33305: [ubsan] Add a check for pointer overflow UB

vsk added a comment. @sberg I agree with @regehr's analysis, and do think that this is a real overflow. Once https://reviews.llvm.org/D34121 lands, we will report this issue in a better way: runtime error: addition of unsigned offset to 0x7fff59dfe990 overflowed to 0x7fff59dfe980

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

vsk updated this revision to Diff 102261. vsk marked an inline comment as done. vsk edited the summary of this revision. vsk added a comment. Thanks for the review! I'll wait for another 'lgtm'. https://reviews.llvm.org/D34121 Files: lib/CodeGen/CGExpr.cpp lib/CodeGen/CGExprScalar.cpp

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

vsk created this revision. The pointer overflow check gives false negatives when dealing with expressions in which an unsigned value is subtracted from a pointer. This is summarized in PR33430 [1]: ubsan permits the result of the submission to be greater than "p", but it should not. To fix the

[PATCH] D33910: [ubsan] Detect invalid unsigned pointer index expression (clang)

vsk added a comment. I've encountered some new diagnostics when running tests on a stage2 instrumented clang, and will need more time to investigate them. Sorry for the delayed communication, I am a bit swamped this week owing to wwdc and being a build cop. https://reviews.llvm.org/D33910

[PATCH] D33910: [ubsan] Detect invalid unsigned pointer index expression (clang)

vsk updated this revision to Diff 101479. vsk marked 3 inline comments as done. vsk added a comment. Thanks for the review comments. I've changed the calls which use Builder as suggested, and fixed up the tests. It sounds like this patch is in good shape, so I'll commit this after two days

[PATCH] D33910: [ubsan] Detect invalid unsigned pointer index expression (clang)

vsk created this revision. Adding an unsigned offset to a base pointer has undefined behavior if the result of the expression would precede the base. An example from @regehr: int foo(char *p, unsigned offset) { return p + offset >= p; // This may be optimized to '1'. } foo(p, -1);

[PATCH] D33588: Fix two sources of UB in __next_hash_pow2 (from __hash_table)

vsk added inline comments. Comment at: include/__hash_table:139 { -return size_t(1) << (std::numeric_limits::digits - __clz(__n-1)); +return (__n > 1) ? (size_t(1) << (std::numeric_limits::digits - __clz(__n-1))) : __n; } EricWF wrote: > Shouldn't

[PATCH] D33305: [ubsan] Add a check for pointer overflow UB

vsk added a comment. Thanks for the review! I've rebased the patch and plan on checking it in tomorrow. At the moment I'm getting some additional test coverage (running check-libcxx and testing more backends). https://reviews.llvm.org/D33305 ___

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

vsk added a comment. In https://reviews.llvm.org/D32842#768038, @eugenis wrote: > This change scares me a bit, to be honest. Is this really that big of a > problem? Blacklists are not supposed to be big, maybe we can tolerate a few > false negatives? I'd like to make it possible to deploy a

[PATCH] D33305: [ubsan] Add a check for pointer overflow UB

vsk updated this revision to Diff 100475. vsk edited the summary of this revision. vsk added a comment. Ping. https://reviews.llvm.org/D33305 Files: docs/UndefinedBehaviorSanitizer.rst include/clang/Basic/Sanitizers.def lib/CodeGen/CGExpr.cpp lib/CodeGen/CGExprScalar.cpp

[PATCH] D33588: Fix two sources of UB in __next_hash_pow2 (from __hash_table)

vsk updated this revision to Diff 100461. vsk edited the summary of this revision. vsk added a comment. Thanks @EricWF for pointing me to the right place to add a test. I've tried to follow the style used by the existing tests. PTAL. https://reviews.llvm.org/D33588 Files:

[PATCH] D33588: Fix two sources of UB in __next_hash_pow2 (from __hash_table)

vsk added a comment. In https://reviews.llvm.org/D33588#765768, @mclow.lists wrote: > I can reproduce this, but I'd rather figure out why we're calling > `__next_hash_pow2(0)` or `(1)` before deciding how to fix it. It looks like we hit the UB while attempting to shrink a hash table during a

[PATCH] D33588: Fix two sources of UB in __next_hash_pow2 (from __hash_table)

vsk created this revision. There are two sources of undefined behavior in __next_hash_pow2: an invalid shift (occurs when __n is 0 or 1), and an invalid call to CLZ (when __n=1). This patch corrects both issues. It's NFC for all values of __n which do not trigger UB, and leads to defined results

[PATCH] D33305: [ubsan] Add a check for pointer overflow UB

vsk added a comment. Thanks for the comments, responses inline -- Comment at: lib/CodeGen/CGExprScalar.cpp:3854 + const Twine ) { + Value *GEPVal = Builder.CreateInBoundsGEP(Ptr, IdxList, Name); + filcab wrote: >

[PATCH] D33305: [ubsan] Add a check for pointer overflow UB

vsk created this revision. Herald added a subscriber: krytarowski. Check pointer arithmetic for overflow. For some more background on this check, see: https://wdtz.org/catching-pointer-overflow-bugs.html https://reviews.llvm.org/D20322 Patch by Will Dietz and John Regehr! This version of

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

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

[PATCH] D30949: [CodeGen] Add an option to enable LLVM IR linting

vsk abandoned this revision. vsk added a comment. I'll try doing this some other way, maybe with optimization remarks. https://reviews.llvm.org/D30949 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D32724: [Modules] Handle sanitizer feature mismatches when importing modules

vsk updated this revision to Diff 98371. vsk marked 2 inline comments as done. vsk added a comment. Address comments from Adrian. (I settled on renaming 'getModularSanitizers' to 'getPPTransparentSanitizers'.) https://reviews.llvm.org/D32724 Files: include/clang/Basic/Sanitizers.h

[PATCH] D32724: [Modules] Handle sanitizer feature mismatches when importing modules

vsk updated this revision to Diff 98363. vsk added a comment. I've uploaded the correct diff this time, sorry for the confusion. https://reviews.llvm.org/D32724 Files: include/clang/Basic/Sanitizers.h lib/Basic/LangOptions.cpp lib/Frontend/CompilerInvocation.cpp

[PATCH] D32724: [Modules] Handle sanitizer feature mismatches when importing modules

vsk updated this revision to Diff 98349. vsk marked 7 inline comments as done. vsk added a comment. Add a comment explaining how and why ASTReader checks for sanitizer feature mismatches. https://reviews.llvm.org/D32724 Files: include/clang/Basic/Sanitizers.h lib/Basic/LangOptions.cpp

[PATCH] D32724: [Modules] Handle sanitizer feature mismatches when importing modules

vsk updated this revision to Diff 98343. vsk retitled this revision from "[Modules] Include the enabled sanitizers in the module hash" to "[Modules] Handle sanitizer feature mismatches when importing modules". vsk edited the summary of this revision. vsk added a comment. If compatible

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

vsk added a comment. In https://reviews.llvm.org/D32724#749868, @dexonsmith wrote: > In https://reviews.llvm.org/D32724#747728, @aprantl wrote: > > > Is it the right solution to use the module hash for correctness, or should > > the mismatch of the serialized langopts trigger a module rebuild

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

vsk added a comment. Offline, @aprantl mentioned a concern about module hashes being required for correctness. I'm not sure whether this is OK or not (@bruno, any thoughts?). AFAICT there are items included in the module hash that, were they removed, would break implicit module builds (e.g

[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

[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

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

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

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

[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#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 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; // _S may not be aligned properly. int main() { return g_S.i; // No alignment check for _S.i is emitted.

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

[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

[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

[PATCH] D32144: [Coverage] Don't emit mappings for functions in dependent contexts (fixes PR32679)

vsk created this revision. The coverage implementation marks functions which won't be emitted as 'deferred', so that it can emit empty coverage regions for them later (once their linkages are known). Functions in dependent contexts are an exception: if there isn't a full instantiation of a

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

vsk added a comment. Thanks for the review! Comment at: test/CodeGenCXX/ubsan-suppress-checks.cpp:25 +// LAMBDA: and i64 %[[THISINT2]], 3, !nosanitize +// LAMBDA: call void @__ubsan_handle_type_mismatch + efriedma wrote: > LAMBDA-NOT: call void

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

vsk created this revision. Clang should look for a default UBSan blacklist in its resource dir. Depends on https://reviews.llvm.org/D32043. https://reviews.llvm.org/D32047 Files: lib/Driver/SanitizerArgs.cpp test/Driver/Inputs/resource_dir/ubsan_blacklist.txt

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

vsk created this revision. This patch removes a limitation which causes us to load at most one default sanitizer blacklist when multiple sanitizers are enabled. E.g if asan + cfi are enabled, and default blacklists for both sanitizers are present, we would only load one of the blacklists. The

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

vsk updated this revision to Diff 95053. vsk edited the summary of this revision. vsk added a comment. Eli pointed out that it's possible to make alignment checks for extern globals work better (llvm.org/PR32630). One solution depends on emitting alignment checks on bases of member expressions

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

vsk updated this revision to Diff 92732. vsk added a comment. Per Eli's comment: test that we don't regress alignment-checking for extern globals which aren't arrays. I verified that for this case, there is not functional change. However, there *is* a somewhat surprising IR change even at -O0,

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

vsk updated this revision to Diff 92572. vsk added a comment. Add a test which shows that we don't regress alignment-checking when accessing extern variables. https://reviews.llvm.org/D30283 Files: lib/CodeGen/CGExpr.cpp lib/CodeGen/CGExprCXX.cpp lib/CodeGen/CodeGenFunction.cpp

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

vsk added a comment. In https://reviews.llvm.org/D30283#707007, @efriedma wrote: > It's possible to misalign a global definition by misaligning its section with > a linker script... but we can probably ignore that possibility. The current implementation does ignore this case. > It's very

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

vsk added a comment. Hi Eli, thanks for your feedback :). In https://reviews.llvm.org/D30283#702085, @efriedma wrote: > I'm not sure we actually want to skip these checks for DeclRefExps. I mean, > you can rely on the backend to correctly align a local variable (assuming the > stack is

[PATCH] D30949: [CodeGen] Add an option to enable LLVM IR linting

vsk created this revision. LLVM has a nifty linter which checks for some common kinds of unusual or undefined behavior by doing some basic IR-level static analysis. Add a CC1 option to clang which enables this analysis. Having the linter available through clang could be a useful debugging tool.

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

[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

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

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

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

[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

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

[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

[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

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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

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

[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

[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

[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() { } > }; > >

[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

[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

[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

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

[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

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

[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

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

[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] 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 '>x', before allowing the load to occur. Both of these checks are redundant, because 'this' must have been null-checked before the

[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

[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] D29437: [ubsan] Detect signed overflow UB in remainder operations

vsk added a comment. In https://reviews.llvm.org/D29437#664379, @regehr wrote: > Does this check need to be sensitive to the dialect of C/C++ that the user > asked for? I know that it used to be the case that the standard could be read > either way for this case, but as you observe it is now

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

vsk added a comment. In https://reviews.llvm.org/D29369#664366, @regehr wrote: > Out of curiosity, how many of these superfluous checks are not subsequently > eliminated by InstCombine? I don't have numbers from a benchmark prepped. Here's what we get with the 'ubsan-promoted-arith.cpp' test

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

vsk created this revision. Teach ubsan to diagnose remainder operations which have undefined behavior due to signed overflow. My copy of the C11 spec draft (6.5.5.6) says that: if the quotient a/b is representable, (a/b)*b + a%b shall equal a; otherwise, the behavior of both a/b and a%b is

<    1   2   3   4   5   6   7   >