[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

schittir added a comment. Landing this one fell through the cracks, hence the delay between accepting and committing the patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139148/new/ https://reviews.llvm.org/D139148 ___

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

This revision was automatically updated to reflect the committed changes. Closed by commit rG472232a80d03: [NFC] Fix potential dereferencing of nullptr (authored by schittir). Herald added a subscriber: wangpc. Changed prior to commit: https://reviews.llvm.org/D139148?vs=481989&id=539705#toc R

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. Builds passed now. I think this looks good. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139148/new/ https://reviews.llvm.org/D139148 _

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

tahonermann added a comment. The build failed again, but it looks like the cause is unrelated to these changes. I restarted the build. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139148/new/ https://reviews.llvm.org/D139148 ___ cfe-commits

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

schittir updated this revision to Diff 481989. schittir added a comment. Fix typos CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139148/new/ https://reviews.llvm.org/D139148 Files: clang/lib/Sema/SemaInit.cpp Index: clang/lib/Sema/SemaInit.cpp ==

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

schittir updated this revision to Diff 481974. schittir marked an inline comment as done. schittir added a comment. Fixed typo. Sorry! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139148/new/ https://reviews.llvm.org/D139148 Files: clang/lib/Sema/SemaInit.cpp Index: clang/lib/Sema

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

tahonermann added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:5966 (Context.hasSameUnqualifiedType(SourceType, DestType) || - S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType + (Initilializer && + S.IsDerivedF

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

schittir updated this revision to Diff 481732. schittir added a comment. Add `(Initializer && ` and assert to else branch per Tom's comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139148/new/ https://reviews.llvm.org/D139148 Files: clang/lib/Sema/SemaInit.cpp Index: clang/l

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

tahonermann added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:5959 if (DestType->isRecordType()) { +assert(Initializer && "Intializer must be non-null"); // - If the initialization is direct-initialization, or if it is It looks like t

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

schittir updated this revision to Diff 481475. schittir edited the summary of this revision. schittir added a comment. Add assert at the beginning of blocks pointed out by Coverity CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139148/new/ https://reviews.llvm.org/D139148 Files: clang

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

tahonermann added a comment. Here is a simple test case that fails the assertion in the current location. struct B { B(int, int); }; struct D : B { D() : B(0, 1) {} }; I spent a bit more time looking at the code and finally realized that `Initializer` is only assigned when `Args

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

schittir added a comment. Comment at: clang/lib/Sema/SemaInit.cpp:5947 if (TryOCLZeroOpaqueTypeInitialization(S, *this, DestType, Initializer)) return; #1/5 Coverity reported explicit null dereference of 'Initializer' Comment a

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

tahonermann requested changes to this revision. tahonermann added a comment. This revision now requires changes to proceed. Hmm, it seems the `assert` doesn't work in that location. The Linux and Windows builds both failed. It would be good to identify a simple test case for that. It seems we'll

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

tahonermann added a comment. > I don't see any after this point. You're right! I thought I had seen some, but upon checking, I agree. That is a good sign we're on the right path! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139148/new/ https://reviews.llvm.org/D139148 __

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

schittir added a comment. In D139148#3969383 , @tahonermann wrote: > When committing changes to address Coverity reported issues, I think it would > be useful to include the Coverity analysis in the commit message. A textual > representation can be obt

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

schittir added a comment. > Perhaps the` assert` should be added after line 5926 above. Done. > I think the checks for `Initializer` being non-null following the addition of > an `assert` should be removed. I don't see any after this point. CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

schittir updated this revision to Diff 480217. schittir added a comment. Move the assert to after the branches that handle the cases where Initializer may be null CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139148/new/ https://reviews.llvm.org/D139148 Files: clang/lib/Sema/SemaIni

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

tahonermann added a comment. > I am bit afraid about release builds: > > if (!Initializer) > > return; > > Is this semantically incorrect? I think so. And now I think I see a path where the proposed `assert` is incorrect as well. The `else` branches at lines 5919, 5921, and 5923 appear to han

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

tschuett added a comment. I am bit afraid about release builds: if (!Initializer) return; Is this semantically incorrect? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139148/new/ https://reviews.llvm.org/D139148 ___ cfe-commits maili

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

tahonermann added a comment. When committing changes to address Coverity reported issues, I think it would be useful to include the Coverity analysis in the commit message. A textual representation can be obtained using `cov-format-errors --emacs-style` from the command line with access to the

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

tahonermann added a comment. The change to add the assert looks good. I'm eager to see what the testsuite has to say about it :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139148/new/ https://reviews.llvm.org/D139148 ___ cfe-commits maili

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

schittir updated this revision to Diff 479731. schittir added a comment. Add assert instead of multiple checks in if conditions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139148/new/ https://reviews.llvm.org/D139148 Files: clang/lib/Sema/SemaInit.cpp Index: clang/lib/Sema/SemaI

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

tahonermann added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:5824-5828 // Handle default initialization. if (Kind.getKind() == InitializationKind::IK_Default) { TryDefaultInitialization(S, Entity, Kind, *this); return; } schittir

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

schittir added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:5824-5828 // Handle default initialization. if (Kind.getKind() == InitializationKind::IK_Default) { TryDefaultInitialization(S, Entity, Kind, *this); return; } tahonermann

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

tahonermann requested changes to this revision. tahonermann added a comment. This revision now requires changes to proceed. Per added comments, I think we should look for a guarantee that `Initializer` is non-null earlier in the function. If there is, then we could remove a bunch of the current

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

schittir updated this revision to Diff 479541. schittir added a comment. Add more nullptr checks per Shafik's comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139148/new/ https://reviews.llvm.org/D139148 Files: clang/lib/Sema/SemaInit.cpp Index: clang/lib/Sema/SemaInit.cpp ==

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

schittir added a comment. In D139148#3965404 , @shafik wrote: > This looks like the correct thing to do but I see further down there are also > unconditional uses of `Initializer` that should also be guarded with a check. > It would be good to fix those

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

shafik added a comment. This looks like the correct thing to do but I see further down there are also unconditional uses of `Initializer` that should also be guarded with a check. It would be good to fix those as well since we are here. I am guessing the answer is no since this was found using

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

schittir created this revision. Herald added a project: All. schittir requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Adding nullptr check for 'Initializer' Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D139148 Files: