[PATCH] D50927: [Sema] Remove location from implicit capture init expr

vsk created this revision. vsk added reviewers: rsmith, rtrieu, erichkeane. A lambda's closure is initialized when the lambda is declared. For implicit captures, the initialization code emitted from EmitLambdaExpr references source locations *within the lambda body* in the function containing the

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

vsk added subscribers: jkorous, vsapsai. vsk added a comment. + Jan and Volodymyr. This seemed to be in good shape the last time I looked at it. Not having touched libclang for a while I don't think I can give an official lgtm. Repository: rC Clang https://reviews.llvm.org/D42043 ___

[PATCH] D50927: [Sema] Remove location from implicit capture init expr

vsk updated this revision to Diff 161536. vsk added a comment. - Here is a GIF that might help illustrate the bug: http://net.vedantk.com/static/llvm/lambda-implicit-capture-bug.gif - Update test/SemaCXX/uninitialized.cpp to highlight the behavior change which comes from using getBeginOrDeclLoc(

[PATCH] D50901: [clang][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned and signed checks

vsk added inline comments. Comment at: test/CodeGen/catch-implicit-integer-truncations-basics-negatives.c:23 +__attribute__((no_sanitize("integer"))) unsigned char blacklist_1_convert_unsigned_int_to_unsigned_char(unsigned int x) { + // CHECK: } + return x; I

[PATCH] D46694: [diagtool] Install diagtool

vsk added a comment. Sgtm. I think it would be worthwhile to release-note this. Repository: rC Clang https://reviews.llvm.org/D46694 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commit

[PATCH] D46918: [Coverage] Discard the last uncompleted deferred region in a decl

vsk created this revision. vsk added a reviewer: arphaman. Discard the last uncompleted deferred region in a decl, if one exists. This prevents lines at the end of a function containing only whitespace or closing braces from being marked as uncovered, if they follow a region terminator (return/bre

[PATCH] D46918: [Coverage] Discard the last uncompleted deferred region in a decl

vsk updated this revision to Diff 147360. vsk added a comment. - Add a regression test for switches. https://reviews.llvm.org/D46918 Files: lib/CodeGen/CoverageMappingGen.cpp test/CoverageMapping/deferred-region.cpp test/CoverageMapping/label.cpp test/CoverageMapping/moremacros.c test

[PATCH] D47097: [WIP][DebugInfo] Preserve scope in auto generated StoreInst

vsk added a comment. I think CodeGenFunction::EmitParmDecl is the right place to set up an ApplyDebugLocation instance. You can look at CodeGenFunction::EmitAutoVarInit for an example of how to use ApplyDebugLocation. Comment at: test/CodeGen/debug-info-preserve-scope.c:1 +//

[PATCH] D47097: [WIP][DebugInfo] Preserve scope in auto generated StoreInst

vsk added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:2062 EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true); +ApplyDebugLocation::CreateArtificial(*this); + } There are two issues here: 1) ApplyDebugLocation is a RAII helper, which mea

[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst

vsk added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:2074 + if (DoStore) { + auto DL = ApplyDebugLocation::CreateArtificial(*this); + EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true); Ideally this would precede the calls to CreateMemTemp w

[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst

vsk added inline comments. Comment at: test/CodeGen/debug-info-preserve-scope.c:10 + +// CHECK: alloca i32 {{.*}} !dbg ![[dbgLocForStore:[0-9]+]] +// CHECK: store i32 {{.*}} !dbg ![[dbgLocForStore:[0-9]+]] In these two check lines, you're capturing the variable d

[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst

vsk added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:72 + // Set debug location in order to preserve the scope + Alloca->setDebugLoc(Builder.getCurrentDebugLocation()); if (AllocaAddr) I think we need to be a bit more careful here. The current debug

[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst

vsk added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:72 + // Set debug location in order to preserve the scope + Alloca->setDebugLoc(Builder.getCurrentDebugLocation()); if (AllocaAddr) aprantl wrote: > vsk wrote: > > I think we need to be a bit more

[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst

vsk added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:72 + // Set debug location in order to preserve the scope + Alloca->setDebugLoc(Builder.getCurrentDebugLocation()); if (AllocaAddr) aprantl wrote: > vsk wrote: > > aprantl wrote: > > > vsk wrote:

[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst

vsk added a comment. In https://reviews.llvm.org/D47097#248, @aprantl wrote: > In https://reviews.llvm.org/D47097#223, @gramanas wrote: > > > In https://reviews.llvm.org/D47097#149, @probinson wrote: > > > > > Can we step back a second and better explain what the problem is? With > >

[PATCH] D46918: [Coverage] Discard the last uncompleted deferred region in a decl

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

[PATCH] D38441: [compiler-rt] [cmake] Add a separate CMake var to control profile runtime

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

[PATCH] D37542: [ubsan] Save a ptrtoint when emitting alignment checks

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

[PATCH] D38210: [ubsan] Port the function sanitizer to C

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

[PATCH] D38210: [ubsan] Port the function sanitizer to C

vsk planned changes to this revision. vsk added a comment. In https://reviews.llvm.org/D38210#887635, @pcc wrote: > Wouldn't we get false positives if there is an indirect call in C++ code that > calls into C code (or vice versa)? Ah, right, I'm surprised I didn't hit that while testing. > I

[PATCH] D38567: [config] Warn when POSIX_C_SOURCE breaks threading support on Darwin

vsk created this revision. Prior to macOS 10.13 and iOS 11, defining POSIX_C_SOURCE before including resulted in hard-to-understand errors. That definition causes a bunch of important definitions from the system headers to be skipped, so users see failures like "can't find mach_port_t". This pat

[PATCH] D38567: [config] Warn when POSIX_C_SOURCE breaks threading support on Darwin

vsk added a comment. I'm not sure how to test the warning against anything but the macOS SDK. When I tried, I hit a -Wincompatible-sysroot issue. I can leave those changes out of this patch if we want to be more conservative. https://reviews.llvm.org/D38567 _

[PATCH] D38567: [config] Warn when POSIX_C_SOURCE breaks threading support on Darwin

vsk abandoned this revision. vsk added a comment. For those following along, Alex worked out that this doesn't affect apple-clang 802. We took a closer look and found that the build break just affects clang-900, and was introduced in this r290889. The fix (r293167) didn't make it into clang-900

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

vsk created this revision. LLVM's smul.with.overflow intrinsic isn't supported on X86 for bit widths larger than 64, or on X86-64 for bit widths larger than 128. The failure mode is either a linker error ("the __muloti4 builtin isn't available for this target") or an assertion failure ("Selection

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

vsk updated this revision to Diff 118857. vsk added a comment. Herald added a subscriber: aheejin. - Update to check against a whitelist of supported targets. https://reviews.llvm.org/D38861 Files: lib/CodeGen/CGBuiltin.cpp test/CodeGen/builtins-overflow-unsupported.c test/CodeGen/builtin

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

vsk added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:2263 + } +} + rjmccall wrote: > Is there a reason this only fails on x86? If LLVM doesn't have generic > wide-operation lowering, this probably needs to be a target whitelist rather > th

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

vsk added a comment. In https://reviews.llvm.org/D38861#896435, @rjmccall wrote: > Okay. Sounds good to me. > > We intend to actually implement the generic lowering, I hope? Yes, I'll make a note of that in PR34920, and am tracking the bug internally in rdar://problem/34963321. https://revi

[PATCH] D38859: [clang] Enable clang build with LLVM_BUILD_INSTRUMENTED without setting LLVM_PROFTDATA

vsk added a comment. llvm-profdata is tightly coupled with the host compiler: while this setup may work if you get lucky, I don't think it's resilient to changes in libProfData. Also, using the instrumented llvm-profdata will be slow and create extra profiles. As an alternative, have you consi

[PATCH] D38859: [clang] Enable clang build with LLVM_BUILD_INSTRUMENTED without setting LLVM_PROFTDATA

vsk added a comment. In https://reviews.llvm.org/D38859#896517, @alexshap wrote: > yeah, i agree, this is not a good idea. My thoughts were different - right > now it's not particularly convenient that one has to specify LLVM_PROFDATA > when it's not actually used by the build. > Maybe we can

[PATCH] D38859: [clang] Enable clang build with LLVM_BUILD_INSTRUMENTED without setting LLVM_PROFTDATA

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

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

vsk added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:2263 + } +} + joerg wrote: > vsk wrote: > > rjmccall wrote: > > > Is there a reason this only fails on x86? If LLVM doesn't have generic > > > wide-operation lowering, this probably needs

[PATCH] D38903: [ubsan] Only use indirect RTTI in prologues on Darwin

vsk created this revision. Using a layer of indirection to point to RTTI through function prologues is not supported on some setups. One reported error message is: error: Cannot represent a difference across sections This is a regression. This patch limits the indirect RTTI behavior to Darwin,

[PATCH] D38903: [ubsan] Only use indirect RTTI in prologues on Darwin

vsk added a comment. Sounds good. This doesn't seem too controversial, since it just takes us back to the old behavior on all platforms except Darwin. I'll wait an hour or so before committing in case there are any more comments. https://reviews.llvm.org/D38903 _

[PATCH] D38913: [ubsan] Don't emit function signatures for virtual methods

vsk created this revision. The function sanitizer only checks indirect calls through function pointers. This excludes all non-static member functions (constructor calls, calls through thunks, etc all use a separate code path). Don't emit function signatures for functions that won't be checked. Ap

[PATCH] D38903: [ubsan] Only use indirect RTTI in prologues on Darwin

vsk added a comment. @pcc made an alternate suggestion which led to https://reviews.llvm.org/D38913. We're still discussing whether the new patch is a sufficient fix. https://reviews.llvm.org/D38903 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D38903: [ubsan] Only use indirect RTTI in prologues on Darwin

vsk abandoned this revision. vsk added a comment. https://reviews.llvm.org/D38913 should make this unnecessary. https://reviews.llvm.org/D38903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cf

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

vsk added a comment. In https://reviews.llvm.org/D27607#901882, @fjricci wrote: > On platforms where `BOOL` == `signed char`, is it actually undefined behavior > (or is it just bad programming practice) to store a value other than 0 or 1 > in your `BOOL`? I can't find any language specs suggest

[PATCH] D51239: [ubsan] Enable -fsanitize=vptr on Apple devices and simulators

vsk created this revision. vsk added a reviewer: delcypher. It seems like an oversight that this check was not always enabled for on-device or device simulator targets. https://reviews.llvm.org/D51239 Files: clang/lib/Driver/ToolChains/Darwin.cpp clang/test/Driver/fsanitize.c Index: clang

[PATCH] D51239: [ubsan] Enable -fsanitize=vptr on Apple devices and simulators

vsk added inline comments. Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2254 Res |= SanitizerKind::Function; + if (!isTargetMacOS() || !isMacosxVersionLT(10, 9)) +Res |= SanitizerKind::Vptr; delcypher wrote: > Could we apply De'Morgan's rule here an

[PATCH] D51239: [ubsan] Enable -fsanitize=vptr on Apple devices and simulators

vsk updated this revision to Diff 162732. vsk added a comment. Address some review feedback. I'm not sure whether iOS 4 is really supported anymore. There are a handful of code paths in the driver which handle that target, so I've added in a version check for it. https://reviews.llvm.org/D512

[PATCH] D50927: [Sema] Remove location from implicit capture init expr

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

[PATCH] D50927: [Sema] Remove location from implicit capture init expr

vsk added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:10689 - S.DiagRuntimeBehavior(DRE->getBeginLoc(), DRE, + S.DiagRuntimeBehavior(DRE->getBeginOrDeclLoc(), DRE, S.PDiag(diag) rsmith wrote: > I'm a bit surpr

[PATCH] D50927: [Sema] Remove location from implicit capture init expr

vsk updated this revision to Diff 162764. vsk added a comment. - Partially address some of @rsmith's feedback. Instead of using the capture default location, I used the beginning location of the capture list. This results in smoother single-stepping when those two locations are on different lin

[PATCH] D50927: [Sema] Remove location from implicit capture init expr

vsk updated this revision to Diff 162918. vsk marked 2 inline comments as done. vsk added a comment. Address the latest round of review feedback. https://reviews.llvm.org/D50927 Files: clang/lib/Sema/SemaLambda.cpp clang/test/CXX/expr/expr.prim/expr.prim.lambda/p14.cpp clang/test/CXX/expr

[PATCH] D50927: [Sema] Remove location from implicit capture init expr

vsk added inline comments. Comment at: clang/lib/Sema/SemaLambda.cpp:1422-1424 auto Entity = InitializedEntity::InitializeLambdaCapture( Var->getIdentifier(), Field->getType(), Loc); InitializationKind InitKind = InitializationKind::CreateDirect(Loc, Loc, Loc);

[PATCH] D51945: [Clang] Fix test to followup https://reviews.llvm.org/rL341977

vsk added a comment. I took care of this in r341985. Repository: rC Clang https://reviews.llvm.org/D51945 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52034: [Clang] Add options -Xclang -coverage-filter and -Xclang -coverage-exclude to filter the files to instrument with gcov

vsk added a comment. Please document the filter behavior (see docs/UsersManual.rst). Comment at: include/clang/Driver/CC1Options.td:236 +def coverage_exclude_EQ : Joined<["-"], "coverage-exclude=">, + Alias; def coverage_exit_block_before_body : Flag<["-"], "coverage-exit-bl

[PATCH] D52064: [Sema] Add a note pointing to the first use of an implicit capture

vsk created this revision. vsk added reviewers: rsmith, erichkeane. When it's not possible to initialize an implicit capture, add a note pointing to the first use of the captured variable. Example (the `note` is new): lambda-expressions.cpp:81:15: error: no matching constructor for initializatio

[PATCH] D52064: [Sema] Add a note pointing to the first use of an implicit capture

vsk updated this revision to Diff 165403. vsk marked an inline comment as done. https://reviews.llvm.org/D52064 Files: clang/include/clang/Basic/DiagnosticSemaKinds.td clang/include/clang/Sema/Sema.h clang/lib/Frontend/FrontendActions.cpp clang/lib/Sema/SemaLambda.cpp clang/lib/Sema/Sem

[PATCH] D52064: [Sema] Add a note pointing to the first use of an implicit capture

vsk added inline comments. Comment at: clang/test/SemaCXX/lambda-expressions.cpp:87 +(void)^{ // expected-error@+1 {{no matching constructor for initialization of 'const G'}} + return [=]{ // expected-error@+1 {{no matching constructor for initialization of 'const G'}}

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

vsk added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:97 + is not equal to the original value before the downcast. This kind of issues + may often be caused by an implicit integer promotions. - ``-fsanitize=integer-divide-by-zero``: Integer divis

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

vsk added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:159 - ``-fsanitize=undefined``: All of the checks listed above other than ``unsigned-integer-overflow`` and the ``nullability-*`` checks. - ``-fsanitize=undefined-trap``: Deprecated alias of

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

vsk added inline comments. Comment at: lib/CodeGen/CodeGenFunction.h:383 + // This stack is used/maintained exclusively by the implicit cast sanitizer. + llvm::SmallVector CastExprStack; + lebedev.ri wrote: > vsk wrote: > > lebedev.ri wrote: > > > vsk wrote: >

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

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. LGTM, although I think it'd be helpful to have another +1 just to be safe. I did two small experiments with this using a Stage1 Rel+Asserts compiler: Stage2 Rel+Asserts build of llvm-tblgen: ninja l

[PATCH] D49760: [clang:sema] de-duplicate getDepthAndIndex helpers

vsk added a comment. Thanks for doing this! Comment at: include/clang/Sema/SemaInternal.h:120 + if (const TemplateTypeParmType *TTP = + UPP.first.dyn_cast()) +return std::make_pair(TTP->getDepth(), TTP->getIndex()); Perhaps 'const auto *TTP = ...'

[PATCH] D49760: [clang:sema] de-duplicate getDepthAndIndex helpers

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D49760#1174141, @nickdesaulniers wrote: > Thanks for the review. Today is my first day committing to clang! Welcome :). LGTM. Feel free to commit this yourself once you

[PATCH] D49589: [UBSan] Strengthen pointer checks in 'new' expressions

vsk added inline comments. Comment at: lib/CodeGen/CodeGenFunction.h:380 + /// True if sanitizer checks for current pointer value are generated. + bool PointerChecksAreEmitted = false; + rjmccall wrote: > sepavloff wrote: > > rjmccall wrote: > > > I don't think

[PATCH] D43787: Fix which Darwin versions have ObjC runtime with full subscripting support.

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks for digging into and addressing this! https://reviews.llvm.org/D43787 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.

[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] 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 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: include/__hash_tabl

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

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

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

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

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

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

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

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

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

[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 lib/CodeGen/CodeGenFunction.

[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 test/CodeGen

[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] D34299: [ubsan] Improve diagnostics for return value checks (clang)

vsk created this revision. This patch makes ubsan's nonnull return value diagnostics more precise, which makes the diagnostics more useful when there are multiple return statements in a function. Example: 1 |__attribute__((returns_nonnull)) char *foo() { 2 | if (...) { 3 |return expr_w

[PATCH] D34212: docs: Document binary compatibility issue due to bug in gcc

vsk added a comment. Could you add links to this document in index.rst and UsersManual.rst? Comment at: docs/BinaryCompatibilityWithOtherCompilers.rst:39 + +https://llvm.org/PR33161 This link is still showing up as 'insecure'. Mind using this?:

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

vsk added a comment. It looks Comment at: lib/Sema/SemaExpr.cpp:14715 +if (Method->isVirtual() && !(Method->hasAttr() || + Method->getParent()->hasAttr())) OdrUse = false; Do you think it makes sense to eliminate all c

[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)

vsk added a comment. In https://reviews.llvm.org/D34299#787795, @arphaman wrote: > It looks like if we have a function without the `return` (like the sample > below), we will pass in a `0` as the location pointer. This will prevent a > report of a runtime error as your compiler-rt change ignore

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

vsk updated this revision to Diff 103606. vsk added a comment. Fix a typo introduced in emitArraySubscriptGEP while refactoring /*isSubtraction=false*/ to CodeGenFunction::NotSubtraction, and add CHECK lines which catch the issue. https://reviews.llvm.org/D34121 Files: lib/CodeGen/CGExpr.cp

[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)

vsk added a comment. In https://reviews.llvm.org/D34299#788379, @filcab wrote: > In https://reviews.llvm.org/D34299#788152, @vsk wrote: > > > The source locations aren't constants. The ubsan runtime uses a bit inside > > of source location structures as a flag. When an issue is diagnosed at a >

[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)

vsk updated this revision to Diff 103632. vsk marked an inline comment as done. vsk added a comment. Handle functions without return statements correctly (fixing an issue pointed out by @arphaman). Now, the instrumentation always checks that we have a valid return location before calling the run

[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)

vsk added inline comments. Comment at: lib/CodeGen/CGStmt.cpp:1035 +assert(ReturnLocation.isValid() && "No valid return location"); +Builder.CreateStore(Builder.CreateBitCast(SLocPtr, Int8PtrTy), +ReturnLocation); filcab wrote: > C

[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)

vsk added a comment. In https://reviews.llvm.org/D34299#788908, @arphaman wrote: > Ok, so now the null check `return.sloc.load` won't call the checker in > compiler-rt and so the program won't `abort` and won't hit the `unreachable`. > I have one question tough: > > This patch changes the behav

[PATCH] D34563: [ubsan] Disable the object-size check at -O0

vsk created this revision. This is motivated by the thread: [cfe-dev] Disabling ubsan's object size check at -O0 I think the driver is the best place to disable a sanitizer check at particular optimization levels. Doing so in the frontend is messy, and makes it really hard to test IR generation

[PATCH] D34563: [ubsan] Disable the object-size check at -O0

vsk updated this revision to Diff 103778. vsk added a comment. Add a diagnostic for users who explicitly turn the object-size check on at -O0, and tighten up the test a bit. https://reviews.llvm.org/D34563 Files: include/clang/Basic/DiagnosticDriverKinds.td lib/Driver/SanitizerArgs.cpp t

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

vsk created this revision. On some targets, passing zero to the clz() or ctz() builtins has undefined behavior. I ran into this issue while debugging UB in __hash_table from libcxx: the bug I was seeing manifested itself differently under -O0 vs -Os, due to a UB call to clz() (see: libcxx/r304617)

[PATCH] D34591: [ubsan] Diagnose invalid uses of builtins (compiler-rt)

vsk created this revision. Herald added subscribers: dberris, kubamracek. Compiler-rt changes and tests to go along with: https://reviews.llvm.org/D34590 https://reviews.llvm.org/D34591 Files: lib/ubsan/ubsan_checks.inc lib/ubsan/ubsan_handlers.cc lib/ubsan/ubsan_handlers.h lib/ubsan/ub

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

vsk marked an inline comment as done. vsk added a comment. Thanks for the review! Comment at: lib/CodeGen/CGBuiltin.cpp:912 + auto IntMax = + llvm::APInt::getMaxValue(ResultInfo.Width).zextOrSelf(Op1Info.Width); + llvm::Value *TruncOverflow = CGF.Builder.Crea

[PATCH] D40720: No -fsanitize=function warning when calling noexcept function through non-noexcept pointer in C++17

vsk added a comment. Please add a test. Repository: rC Clang https://reviews.llvm.org/D40720 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40720: No -fsanitize=function warning when calling noexcept function through non-noexcept pointer in C++17

vsk added a comment. Copying my comment from the diffusion thread to keep things in one place: > You could make FunctionTypeMismatch an 'AlwaysRecoverable' check, just like > the Vptr check, and remove the _abort handlers from the ubsan runtimes. Repository: rC Clang https://reviews.llvm.or

[PATCH] D40720: No -fsanitize=function warning when calling noexcept function through non-noexcept pointer in C++17

vsk added a comment. In https://reviews.llvm.org/D40720#958697, @sberg wrote: > In https://reviews.llvm.org/D40720#958677, @vsk wrote: > > > Please add a test. > > > Note that the bot upon the first closing of this review changed the shown > diff from the combined cfe+compiler-rt diff to just th

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

vsk updated this revision to Diff 127424. vsk added a comment. - Make the patch cleaner by introducing an overload of EmitCall() which doesn't require a SourceLocation argument. https://reviews.llvm.org/D40698 Files: docs/UndefinedBehaviorSanitizer.rst lib/CodeGen/CGBuiltin.cpp lib/CodeG

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

vsk updated this revision to Diff 127426. vsk added a comment. - Tighten the IR test case. https://reviews.llvm.org/D40698 Files: docs/UndefinedBehaviorSanitizer.rst lib/CodeGen/CGBuiltin.cpp lib/CodeGen/CGCall.cpp lib/CodeGen/CGExpr.cpp lib/CodeGen/CGExprCXX.cpp lib/CodeGen/CodeGen

[PATCH] D41374: [Coverage] Fix use-after free in coverage emission

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, this lgtm as a stopgap. As I mentioned offline, I think the goal should be to have non-modules and modules-enabled builds of a project produce identical coverage reports. I'll look into wha

[PATCH] D40295: -fsanitize=vptr warnings on bad static types in dynamic_cast and typeid

vsk added a comment. I don't think any checks can be skipped in the newly-introduced calls to EmitTypeCheck. Clang uses EmitDynamicCast on arbitrary addresses, not just addresses which are known to be checked for alignment/etc. Regarding the test update, I think it makes sense to extend the run

[PATCH] D40295: -fsanitize=vptr warnings on bad static types in dynamic_cast and typeid

vsk added reviewers: vsk, thakis. vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, lgtm. Could you wait a day or two before committing? IIRC Richard or Nico have a -fsanitize=vptr Chromium bot, and they may have further comments. https://

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

vsk added a comment. I have some results from the development build of our kernel ('-O2 -g -flto'). According to dwarfdump -statistics, when compiled with -fextend-lifetimes, the percentage of covered scope bytes increases from 62% to 69%. The number of inlined scopes decreases by 4%, and (I th

[PATCH] D41717: [CGBuiltin] Handle unsigned mul overflow properly (PR35750)

vsk created this revision. vsk added reviewers: efriedma, arphaman. r320902 fixed the IRGen for some types of checked multiplications. It did not handle unsigned overflow correctly in the case where the signed operand is negative (PR35750). Eli pointed out that on overflow, the result must be equ

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

vsk created this revision. vsk added reviewers: rsmith, aaron.ballman, faisalv. When parsing C++ type construction expressions with list initialization, forward the locations of the braces to Sema. Without these locations, the code coverage pass crashes on the given test case, because the pass re

  1   2   3   4   5   6   7   >