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

[Sindhu Chittireddy via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sindhu Chittireddy via Phabricator via cfe-commits]

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=539705#toc

Repository:
  rG LLVM Github Monorepo

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
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6336,6 +6336,7 @@
   // We're at the end of the line for C: it's either a write-back conversion
   // or it's a C assignment. There's no need to check anything else.
   if (!S.getLangOpts().CPlusPlus) {
+assert(Initializer && "Initializer must be non-null");
 // If allowed, check whether this is an Objective-C writeback conversion.
 if (allowObjCWritebackConversion &&
 tryObjCWritebackConversion(S, *this, Entity, Initializer)) {
@@ -6362,7 +6363,8 @@
 if (Kind.getKind() == InitializationKind::IK_Direct ||
 (Kind.getKind() == InitializationKind::IK_Copy &&
  (Context.hasSameUnqualifiedType(SourceType, DestType) ||
-  S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType 
{
+  (Initializer && S.IsDerivedFrom(Initializer->getBeginLoc(),
+  SourceType, DestType) {
   TryConstructorInitialization(S, Entity, Kind, Args, DestType, DestType,
*this);
 
@@ -6406,6 +6408,7 @@
   //   function is used) to a derived class thereof are enumerated as
   //   described in 13.3.1.4, and the best one is chosen through
   //   overload resolution (13.3).
+  assert(Initializer && "Initializer must be non-null");
   TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
TopLevelOfInitList);
 }
@@ -6457,6 +6460,7 @@
   //- Otherwise, if the source type is a (possibly cv-qualified) class
   //  type, conversion functions are considered.
   if (!SourceType.isNull() && SourceType->isRecordType()) {
+assert(Initializer && "Initializer must be non-null");
 // For a conversion to _Atomic(T) from either T or a class type derived
 // from T, initialize the T object then convert to _Atomic type.
 bool NeedAtomicConversion = false;


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6336,6 +6336,7 @@
   // We're at the end of the line for C: it's either a write-back conversion
   // or it's a C assignment. There's no need to check anything else.
   if (!S.getLangOpts().CPlusPlus) {
+assert(Initializer && "Initializer must be non-null");
 // If allowed, check whether this is an Objective-C writeback conversion.
 if (allowObjCWritebackConversion &&
 tryObjCWritebackConversion(S, *this, Entity, Initializer)) {
@@ -6362,7 +6363,8 @@
 if (Kind.getKind() == InitializationKind::IK_Direct ||
 (Kind.getKind() == InitializationKind::IK_Copy &&
  (Context.hasSameUnqualifiedType(SourceType, DestType) ||
-  S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType {
+  (Initializer && S.IsDerivedFrom(Initializer->getBeginLoc(),
+  SourceType, DestType) {
   TryConstructorInitialization(S, Entity, Kind, Args, DestType, DestType,
*this);
 
@@ -6406,6 +6408,7 @@
   //   function is used) to a derived class thereof are enumerated as
   //   described in 13.3.1.4, and the best one is chosen through
   //   overload resolution (13.3).
+  assert(Initializer && "Initializer must be non-null");
   TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
TopLevelOfInitList);
 }
@@ -6457,6 +6460,7 @@
   //- Otherwise, if the source type is a (possibly cv-qualified) class
   //  type, conversion functions are considered.
   if (!SourceType.isNull() && SourceType->isRecordType()) {
+assert(Initializer && "Initializer must be non-null");
 // For a conversion to _Atomic(T) from either T or a class type derived
 // from T, initialize the T object then convert to _Atomic type.
 bool NeedAtomicConversion = false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tom Honermann via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tom Honermann via Phabricator via cfe-commits]

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 mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sindhu Chittireddy via Phabricator via cfe-commits]

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
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5936,6 +5936,7 @@
   // We're at the end of the line for C: it's either a write-back conversion
   // or it's a C assignment. There's no need to check anything else.
   if (!S.getLangOpts().CPlusPlus) {
+assert(Initializer && "Initializer must be non-null");
 // If allowed, check whether this is an Objective-C writeback conversion.
 if (allowObjCWritebackConversion &&
 tryObjCWritebackConversion(S, *this, Entity, Initializer)) {
@@ -5962,7 +5963,8 @@
 if (Kind.getKind() == InitializationKind::IK_Direct ||
 (Kind.getKind() == InitializationKind::IK_Copy &&
  (Context.hasSameUnqualifiedType(SourceType, DestType) ||
-  S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType
+  (Initializer &&
+   S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, 
DestType)
   TryConstructorInitialization(S, Entity, Kind, Args,
DestType, DestType, *this);
 // - Otherwise (i.e., for the remaining copy-initialization cases),
@@ -5971,9 +5973,11 @@
 //   used) to a derived class thereof are enumerated as described in
 //   13.3.1.4, and the best one is chosen through overload resolution
 //   (13.3).
-else
+else {
+  assert(Initializer && "Initializer must be non-null");
   TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
TopLevelOfInitList);
+}
 return;
   }
 
@@ -6022,6 +6026,7 @@
   //- Otherwise, if the source type is a (possibly cv-qualified) class
   //  type, conversion functions are considered.
   if (!SourceType.isNull() && SourceType->isRecordType()) {
+assert(Initializer && "Initializer must be non-null");
 // For a conversion to _Atomic(T) from either T or a class type derived
 // from T, initialize the T object then convert to _Atomic type.
 bool NeedAtomicConversion = false;


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5936,6 +5936,7 @@
   // We're at the end of the line for C: it's either a write-back conversion
   // or it's a C assignment. There's no need to check anything else.
   if (!S.getLangOpts().CPlusPlus) {
+assert(Initializer && "Initializer must be non-null");
 // If allowed, check whether this is an Objective-C writeback conversion.
 if (allowObjCWritebackConversion &&
 tryObjCWritebackConversion(S, *this, Entity, Initializer)) {
@@ -5962,7 +5963,8 @@
 if (Kind.getKind() == InitializationKind::IK_Direct ||
 (Kind.getKind() == InitializationKind::IK_Copy &&
  (Context.hasSameUnqualifiedType(SourceType, DestType) ||
-  S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType
+  (Initializer &&
+   S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType)
   TryConstructorInitialization(S, Entity, Kind, Args,
DestType, DestType, *this);
 // - Otherwise (i.e., for the remaining copy-initialization cases),
@@ -5971,9 +5973,11 @@
 //   used) to a derived class thereof are enumerated as described in
 //   13.3.1.4, and the best one is chosen through overload resolution
 //   (13.3).
-else
+else {
+  assert(Initializer && "Initializer must be non-null");
   TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
TopLevelOfInitList);
+}
 return;
   }
 
@@ -6022,6 +6026,7 @@
   //- Otherwise, if the source type is a (possibly cv-qualified) class
   //  type, conversion functions are considered.
   if (!SourceType.isNull() && SourceType->isRecordType()) {
+assert(Initializer && "Initializer must be non-null");
 // For a conversion to _Atomic(T) from either T or a class type derived
 // from T, initialize the T object then convert to _Atomic type.
 bool NeedAtomicConversion = false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sindhu Chittireddy via Phabricator via cfe-commits]

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/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5936,6 +5936,7 @@
   // We're at the end of the line for C: it's either a write-back conversion
   // or it's a C assignment. There's no need to check anything else.
   if (!S.getLangOpts().CPlusPlus) {
+assert(Initializer && "Intializer must be non-null");
 // If allowed, check whether this is an Objective-C writeback conversion.
 if (allowObjCWritebackConversion &&
 tryObjCWritebackConversion(S, *this, Entity, Initializer)) {
@@ -5962,7 +5963,8 @@
 if (Kind.getKind() == InitializationKind::IK_Direct ||
 (Kind.getKind() == InitializationKind::IK_Copy &&
  (Context.hasSameUnqualifiedType(SourceType, DestType) ||
-  S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType
+  (Initializer &&
+   S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, 
DestType)
   TryConstructorInitialization(S, Entity, Kind, Args,
DestType, DestType, *this);
 // - Otherwise (i.e., for the remaining copy-initialization cases),
@@ -5971,9 +5973,11 @@
 //   used) to a derived class thereof are enumerated as described in
 //   13.3.1.4, and the best one is chosen through overload resolution
 //   (13.3).
-else
+else {
+  assert(Initializer && "Intializer must be non-null");
   TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
TopLevelOfInitList);
+}
 return;
   }
 
@@ -6022,6 +6026,7 @@
   //- Otherwise, if the source type is a (possibly cv-qualified) class
   //  type, conversion functions are considered.
   if (!SourceType.isNull() && SourceType->isRecordType()) {
+assert(Initializer && "Intializer must be non-null");
 // For a conversion to _Atomic(T) from either T or a class type derived
 // from T, initialize the T object then convert to _Atomic type.
 bool NeedAtomicConversion = false;


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5936,6 +5936,7 @@
   // We're at the end of the line for C: it's either a write-back conversion
   // or it's a C assignment. There's no need to check anything else.
   if (!S.getLangOpts().CPlusPlus) {
+assert(Initializer && "Intializer must be non-null");
 // If allowed, check whether this is an Objective-C writeback conversion.
 if (allowObjCWritebackConversion &&
 tryObjCWritebackConversion(S, *this, Entity, Initializer)) {
@@ -5962,7 +5963,8 @@
 if (Kind.getKind() == InitializationKind::IK_Direct ||
 (Kind.getKind() == InitializationKind::IK_Copy &&
  (Context.hasSameUnqualifiedType(SourceType, DestType) ||
-  S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType
+  (Initializer &&
+   S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType)
   TryConstructorInitialization(S, Entity, Kind, Args,
DestType, DestType, *this);
 // - Otherwise (i.e., for the remaining copy-initialization cases),
@@ -5971,9 +5973,11 @@
 //   used) to a derived class thereof are enumerated as described in
 //   13.3.1.4, and the best one is chosen through overload resolution
 //   (13.3).
-else
+else {
+  assert(Initializer && "Intializer must be non-null");
   TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
TopLevelOfInitList);
+}
 return;
   }
 
@@ -6022,6 +6026,7 @@
   //- Otherwise, if the source type is a (possibly cv-qualified) class
   //  type, conversion functions are considered.
   if (!SourceType.isNull() && SourceType->isRecordType()) {
+assert(Initializer && "Intializer must be non-null");
 // For a conversion to _Atomic(T) from either T or a class type derived
 // from T, initialize the T object then convert to _Atomic type.
 bool NeedAtomicConversion = false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



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

Typo.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139148/new/

https://reviews.llvm.org/D139148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sindhu Chittireddy via Phabricator via cfe-commits]

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/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5936,6 +5936,7 @@
   // We're at the end of the line for C: it's either a write-back conversion
   // or it's a C assignment. There's no need to check anything else.
   if (!S.getLangOpts().CPlusPlus) {
+assert(Initializer && "Intializer must be non-null");
 // If allowed, check whether this is an Objective-C writeback conversion.
 if (allowObjCWritebackConversion &&
 tryObjCWritebackConversion(S, *this, Entity, Initializer)) {
@@ -5962,7 +5963,8 @@
 if (Kind.getKind() == InitializationKind::IK_Direct ||
 (Kind.getKind() == InitializationKind::IK_Copy &&
  (Context.hasSameUnqualifiedType(SourceType, DestType) ||
-  S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType
+  (Initilializer &&
+   S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, 
DestType)
   TryConstructorInitialization(S, Entity, Kind, Args,
DestType, DestType, *this);
 // - Otherwise (i.e., for the remaining copy-initialization cases),
@@ -5971,9 +5973,11 @@
 //   used) to a derived class thereof are enumerated as described in
 //   13.3.1.4, and the best one is chosen through overload resolution
 //   (13.3).
-else
+else {
+  assert(Initializer && "Intializer must be non-null");
   TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
TopLevelOfInitList);
+}
 return;
   }
 
@@ -6022,6 +6026,7 @@
   //- Otherwise, if the source type is a (possibly cv-qualified) class
   //  type, conversion functions are considered.
   if (!SourceType.isNull() && SourceType->isRecordType()) {
+assert(Initializer && "Intializer must be non-null");
 // For a conversion to _Atomic(T) from either T or a class type derived
 // from T, initialize the T object then convert to _Atomic type.
 bool NeedAtomicConversion = false;


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5936,6 +5936,7 @@
   // We're at the end of the line for C: it's either a write-back conversion
   // or it's a C assignment. There's no need to check anything else.
   if (!S.getLangOpts().CPlusPlus) {
+assert(Initializer && "Intializer must be non-null");
 // If allowed, check whether this is an Objective-C writeback conversion.
 if (allowObjCWritebackConversion &&
 tryObjCWritebackConversion(S, *this, Entity, Initializer)) {
@@ -5962,7 +5963,8 @@
 if (Kind.getKind() == InitializationKind::IK_Direct ||
 (Kind.getKind() == InitializationKind::IK_Copy &&
  (Context.hasSameUnqualifiedType(SourceType, DestType) ||
-  S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType
+  (Initilializer &&
+   S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType)
   TryConstructorInitialization(S, Entity, Kind, Args,
DestType, DestType, *this);
 // - Otherwise (i.e., for the remaining copy-initialization cases),
@@ -5971,9 +5973,11 @@
 //   used) to a derived class thereof are enumerated as described in
 //   13.3.1.4, and the best one is chosen through overload resolution
 //   (13.3).
-else
+else {
+  assert(Initializer && "Intializer must be non-null");
   TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
TopLevelOfInitList);
+}
 return;
   }
 
@@ -6022,6 +6026,7 @@
   //- Otherwise, if the source type is a (possibly cv-qualified) class
   //  type, conversion functions are considered.
   if (!SourceType.isNull() && SourceType->isRecordType()) {
+assert(Initializer && "Intializer must be non-null");
 // For a conversion to _Atomic(T) from either T or a class type derived
 // from T, initialize the T object then convert to _Atomic type.
 bool NeedAtomicConversion = false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tom Honermann via Phabricator via cfe-commits]

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 this assertion was triggered for both the Linux and Windows 
builds.

Since the `else` branch below unconditionally dereferences `Initializer`, I 
think the only way for `Initializer` to be null and for a crash not to occur is 
if the `then` branch is taken, but without `Initializer->getBeginLoc()` being 
evaluated due to short circuiting. I think we should do this:
1) Add a `Initializer &&` condition prior to the call to 
`S.IsDerivedFrom(Initializer->getBeginLoc(), ...)`. This will require yet more 
parenthesis for the conditional logic.
2) Add the assert to the `else` branch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139148/new/

https://reviews.llvm.org/D139148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sindhu Chittireddy via Phabricator via cfe-commits]

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/lib/Sema/SemaInit.cpp


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5936,6 +5936,7 @@
   // We're at the end of the line for C: it's either a write-back conversion
   // or it's a C assignment. There's no need to check anything else.
   if (!S.getLangOpts().CPlusPlus) {
+assert(Initializer && "Intializer must be non-null");
 // If allowed, check whether this is an Objective-C writeback conversion.
 if (allowObjCWritebackConversion &&
 tryObjCWritebackConversion(S, *this, Entity, Initializer)) {
@@ -5955,6 +5956,7 @@
 
   // - If the destination type is a (possibly cv-qualified) class type:
   if (DestType->isRecordType()) {
+assert(Initializer && "Intializer must be non-null");
 // - If the initialization is direct-initialization, or if it is
 //   copy-initialization where the cv-unqualified version of the
 //   source type is the same class as, or a derived class of, the
@@ -6022,6 +6024,7 @@
   //- Otherwise, if the source type is a (possibly cv-qualified) class
   //  type, conversion functions are considered.
   if (!SourceType.isNull() && SourceType->isRecordType()) {
+assert(Initializer && "Intializer must be non-null");
 // For a conversion to _Atomic(T) from either T or a class type derived
 // from T, initialize the T object then convert to _Atomic type.
 bool NeedAtomicConversion = false;


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5936,6 +5936,7 @@
   // We're at the end of the line for C: it's either a write-back conversion
   // or it's a C assignment. There's no need to check anything else.
   if (!S.getLangOpts().CPlusPlus) {
+assert(Initializer && "Intializer must be non-null");
 // If allowed, check whether this is an Objective-C writeback conversion.
 if (allowObjCWritebackConversion &&
 tryObjCWritebackConversion(S, *this, Entity, Initializer)) {
@@ -5955,6 +5956,7 @@
 
   // - If the destination type is a (possibly cv-qualified) class type:
   if (DestType->isRecordType()) {
+assert(Initializer && "Intializer must be non-null");
 // - If the initialization is direct-initialization, or if it is
 //   copy-initialization where the cv-unqualified version of the
 //   source type is the same class as, or a derived class of, the
@@ -6022,6 +6024,7 @@
   //- Otherwise, if the source type is a (possibly cv-qualified) class
   //  type, conversion functions are considered.
   if (!SourceType.isNull() && SourceType->isRecordType()) {
+assert(Initializer && "Intializer must be non-null");
 // For a conversion to _Atomic(T) from either T or a class type derived
 // from T, initialize the T object then convert to _Atomic type.
 bool NeedAtomicConversion = false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tom Honermann via Phabricator via cfe-commits]

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.size()` is exactly 1. So 
`Initializer` doesn't equate to whether or not an initializer is present; it 
equates to when exactly one initialization argument is present and in that case 
it aliases `Args[0]`. I don't like that, but deviating from it just to address 
a Coverity complaint doesn't seem justified.

I suggest we do this: add an assertion on `Initializer` at the beginning of 
each of the blocks that Coverity complained about. In my view of the source, 
that corresponds to lines 5939, 5958, and 6025.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139148/new/

https://reviews.llvm.org/D139148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sindhu Chittireddy via Phabricator via cfe-commits]

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 at: clang/lib/Sema/SemaInit.cpp:5967
  (Context.hasSameUnqualifiedType(SourceType, DestType) ||
   S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType
   TryConstructorInitialization(S, Entity, Kind, Args,

#2/5 Coverity reported explicit null dereference of 'Initializer'



Comment at: clang/lib/Sema/SemaInit.cpp:5977
 else
   TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
TopLevelOfInitList);

#3/5 Coverity reported explicit null dereference of 'Initializer'



Comment at: clang/lib/Sema/SemaInit.cpp:6032
   if (Context.hasSameUnqualifiedType(SourceType, Atomic->getValueType()) ||
   S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType,
   Atomic->getValueType())) {

#4/5 Coverity reported explicit null dereference of 'Initializer'



Comment at: clang/lib/Sema/SemaInit.cpp:6039
 
 TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
  TopLevelOfInitList);

#5/5 Coverity reported explicit null dereference of 'Initializer'


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139148/new/

https://reviews.llvm.org/D139148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tom Honermann via Phabricator via cfe-commits]

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 need to move the `assert` further down the function body.

  FAILED: tools/clang/lib/Tooling/ASTNodeAPI.json
  cmd.exe /C "cd /D 
C:\ws\w8\llvm-project\premerge-checks\build\tools\clang\lib\Tooling && 
C:\ws\w8\llvm-project\premerge-checks\build\bin\clang-ast-dump.exe 
--skip-processing=0 -I 
C:/ws/w8/llvm-project/premerge-checks/build/lib/clang/16/include -I 
C:/ws/w8/llvm-project/premerge-checks/clang/include -I 
C:/ws/w8/llvm-project/premerge-checks/build/tools/clang/include -I 
C:/ws/w8/llvm-project/premerge-checks/build/include -I 
C:/ws/w8/llvm-project/premerge-checks/llvm/include --json-output-path 
C:/ws/w8/llvm-project/premerge-checks/build/tools/clang/lib/Tooling/ASTNodeAPI.json"
  Assertion failed: Initializer && "Intializer must be non-null", file 
C:\ws\w8\llvm-project\premerge-checks\clang\lib\Sema\SemaInit.cpp, line 5928


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139148/new/

https://reviews.llvm.org/D139148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tom Honermann via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sindhu Chittireddy via Phabricator via cfe-commits]

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 obtained using `cov-format-errors --emacs-style` from 
> the command line with access to the intermediate directory. I'm not sure if a 
> textual representation can be obtained through the Coverity UI though.

Couldn't find a way to do this via the UI, so I am trying the command line way.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139148/new/

https://reviews.llvm.org/D139148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sindhu Chittireddy via Phabricator via cfe-commits]

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.org/D139148/new/

https://reviews.llvm.org/D139148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sindhu Chittireddy via Phabricator via cfe-commits]

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/SemaInit.cpp


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5925,6 +5925,8 @@
 return;
   }
 
+  assert(Initializer && "Intializer must be non-null");
+
   // Determine whether we should consider writeback conversions for
   // Objective-C ARC.
   bool allowObjCWritebackConversion = S.getLangOpts().ObjCAutoRefCount &&


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5925,6 +5925,8 @@
 return;
   }
 
+  assert(Initializer && "Intializer must be non-null");
+
   // Determine whether we should consider writeback conversions for
   // Objective-C ARC.
   bool allowObjCWritebackConversion = S.getLangOpts().ObjCAutoRefCount &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tom Honermann via Phabricator via cfe-commits]

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 
handle cases where `Initializer` may be null.

  clang/lib/Sema/SemaInit.cpp:
   5824   // Handle default initialization.
   5825   if (Kind.getKind() == InitializationKind::IK_Default) {
   5826 TryDefaultInitialization(S, Entity, Kind, *this);
   5827 return;
   5828   }
  
   
   5835   if (const ArrayType *DestAT = Context.getAsArrayType(DestType)) {
   5836 if (Initializer && isa(DestAT)) {
   
   5912 else if (S.getLangOpts().CPlusPlus &&
   5913  Entity.getKind() == InitializedEntity::EK_Member &&
   5914  Initializer && isa(Initializer)) {
   
   5918 } else if (DestAT->getElementType()->isCharType())
   5919   SetFailed(FK_ArrayNeedsInitListOrStringLiteral);
   5920 else if (IsWideCharCompatible(DestAT->getElementType(), Context))
   5921   SetFailed(FK_ArrayNeedsInitListOrWideStringLiteral);
   5922 else
   5923   SetFailed(FK_ArrayNeedsInitList);
   5924
   5925 return;
   5926   }

Perhaps the` assert` should be added after line 5926 above. I would be 
concerned about adding a return statement conditional on `Initializer` being 
null there though. If the intention is that an initializer must be present 
after that point, an early return could result in a miscompile; I'd rather have 
a crash.

I think the checks for `Initializer` being non-null following the addition of 
an `assert` should be removed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139148/new/

https://reviews.llvm.org/D139148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Thorsten via Phabricator via cfe-commits]

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 mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tom Honermann via Phabricator via cfe-commits]

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 intermediate directory. I'm not sure if a 
textual representation can be obtained through the Coverity UI though.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139148/new/

https://reviews.llvm.org/D139148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tom Honermann via Phabricator via cfe-commits]

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 mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sindhu Chittireddy via Phabricator via cfe-commits]

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/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5827,6 +5827,8 @@
 return;
   }
 
+  assert(Initializer && "Intializer must be non-null");
+
   // - If the destination type is an array of characters, an array of
   //   char16_t, an array of char32_t, or an array of wchar_t, and the
   //   initializer is a string literal, see 8.5.2.


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5827,6 +5827,8 @@
 return;
   }
 
+  assert(Initializer && "Intializer must be non-null");
+
   // - If the destination type is an array of characters, an array of
   //   char16_t, an array of char32_t, or an array of wchar_t, and the
   //   initializer is a string literal, see 8.5.2.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tom Honermann via Phabricator via cfe-commits]

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 wrote:
> tahonermann wrote:
> > This block handles default initialization and unconditionally performs a 
> > return. I wonder if this effectively guarantees that `Initializer` is 
> > non-null if this block is not entered.
> Thank you for the review. I am trying to find an explicit connection between 
> this block and `Initializer` being non-null, but it isn't clear to me yet. 
> How about adding the assert right after this block?
I think adding an `assert` here is a good thing to try. If any tests fail, then 
we'll know this isn't the right place. If no tests fail, then, well, the 
`assert` is good or we need more tests! ;)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139148/new/

https://reviews.llvm.org/D139148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sindhu Chittireddy via Phabricator via cfe-commits]

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 wrote:
> This block handles default initialization and unconditionally performs a 
> return. I wonder if this effectively guarantees that `Initializer` is 
> non-null if this block is not entered.
Thank you for the review. I am trying to find an explicit connection between 
this block and `Initializer` being non-null, but it isn't clear to me yet. 
How about adding the assert right after this block?



Comment at: clang/lib/Sema/SemaInit.cpp:5941
 if (allowObjCWritebackConversion &&
 tryObjCWritebackConversion(S, *this, Entity, Initializer)) {
   return;

tahonermann wrote:
> This use of `Initializer` is also questionable; 
> `tryObjCWritebackConversion()` will unconditionally dereference it.
Indeed. This set of calls should be addressed too. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139148/new/

https://reviews.llvm.org/D139148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tom Honermann via Phabricator via cfe-commits]

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 existence checks rather than adding more.




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

This block handles default initialization and unconditionally performs a 
return. I wonder if this effectively guarantees that `Initializer` is non-null 
if this block is not entered.



Comment at: clang/lib/Sema/SemaInit.cpp:5933
 
   if (TryOCLSamplerInitialization(S, *this, DestType, Initializer))
 return;

The use of `initializer` here looks questionable too; 
`TryOCLSamplerInitialization()` will dereference it without a check if both 
`S.getLangOpts().OpenCL` and `DestType->isSamplerT()` are both true.



Comment at: clang/lib/Sema/SemaInit.cpp:5941
 if (allowObjCWritebackConversion &&
 tryObjCWritebackConversion(S, *this, Entity, Initializer)) {
   return;

This use of `Initializer` is also questionable; `tryObjCWritebackConversion()` 
will unconditionally dereference it.



Comment at: clang/lib/Sema/SemaInit.cpp:5945
 
 if (TryOCLZeroOpaqueTypeInitialization(S, *this, DestType, Initializer))
   return;

This use of `Initializer` is also questionable; 
`TryOCLZeroOpaqueTypeInitialization()` will conditionally dereference it.



Comment at: clang/lib/Sema/SemaInit.cpp:5976
 else
   TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
TopLevelOfInitList);

This use of `Initializer` looks like it also needs to be protected; 
`TryUserDefinedConversion()` unconditionally dereferences it.



Comment at: clang/lib/Sema/SemaInit.cpp:6038-6039
 
 TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
  TopLevelOfInitList);
 MaybeProduceObjCObject(S, *this, Entity);

This use of `Initializer` looks like it also needs to be protected; 
`TryUserDefinedConversion()` unconditionally dereferences it.



Comment at: clang/lib/Sema/SemaInit.cpp:6066-6071
 = S.TryImplicitConversion(Initializer, DestType,
   /*SuppressUserConversions*/true,
   Sema::AllowedExplicit::None,
   /*InOverloadResolution*/ false,
   /*CStyle=*/Kind.isCStyleOrFunctionalCast(),
   allowObjCWritebackConversion);

`Initializer` is unconditionally dereferenced in 
`Sema::TryImplicitConversion()`.

I stopped analyzing other uses here. At this point (at least), it seems clear 
that the expectation is that `Initializer` is non-null. That makes me think 
that, rather than adding additional checks, we should look for an existing 
guarantee that `initializer` is in fact non-null (something that Coverity 
missed) or add one. If we need to add such a guarantee, we could add an 
`assert(Initializer)` somewhere earlier in the function, but I'm not sure where.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139148/new/

https://reviews.llvm.org/D139148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sindhu Chittireddy via Phabricator via cfe-commits]

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
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5962,9 +5962,10 @@
 if (Kind.getKind() == InitializationKind::IK_Direct ||
 (Kind.getKind() == InitializationKind::IK_Copy &&
  (Context.hasSameUnqualifiedType(SourceType, DestType) ||
-  S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType
-  TryConstructorInitialization(S, Entity, Kind, Args,
-   DestType, DestType, *this);
+  (Initializer &&
+   S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, 
DestType)
+  TryConstructorInitialization(S, Entity, Kind, Args, DestType, DestType,
+   *this);
 // - Otherwise (i.e., for the remaining copy-initialization cases),
 //   user-defined conversion sequences that can convert from the source
 //   type to the destination type or (when a conversion function is
@@ -6027,8 +6028,8 @@
 bool NeedAtomicConversion = false;
 if (const AtomicType *Atomic = DestType->getAs()) {
   if (Context.hasSameUnqualifiedType(SourceType, Atomic->getValueType()) ||
-  S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType,
-  Atomic->getValueType())) {
+  (Initializer && S.IsDerivedFrom(Initializer->getBeginLoc(),
+  SourceType, Atomic->getValueType())) 
{
 DestType = Atomic->getValueType();
 NeedAtomicConversion = true;
   }
@@ -6045,7 +6046,7 @@
   //- Otherwise, if the initialization is direct-initialization, the source
   //type is std::nullptr_t, and the destination type is bool, the initial
   //value of the object being initialized is false.
-  if (!SourceType.isNull() && SourceType->isNullPtrType() &&
+  if (!SourceType.isNull() && SourceType->isNullPtrType() && Initializer &&
   DestType->isBooleanType() &&
   Kind.getKind() == InitializationKind::IK_Direct) {
 AddConversionSequenceStep(
@@ -6095,11 +6096,11 @@
 DeclAccessPair dap;
 if (isLibstdcxxPointerReturnFalseHack(S, Entity, Initializer)) {
   AddZeroInitializationStep(Entity.getType());
-} else if (Initializer->getType() == Context.OverloadTy &&
+} else if (Initializer && Initializer->getType() == Context.OverloadTy &&
!S.ResolveAddressOfOverloadedFunction(Initializer, DestType,
  false, dap))
   SetFailed(InitializationSequence::FK_AddressOfOverloadFailed);
-else if (Initializer->getType()->isFunctionType() &&
+else if (Initializer && Initializer->getType()->isFunctionType() &&
  isExprAnUnaddressableFunction(S, Initializer))
   SetFailed(InitializationSequence::FK_AddressOfUnaddressableFunction);
 else


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5962,9 +5962,10 @@
 if (Kind.getKind() == InitializationKind::IK_Direct ||
 (Kind.getKind() == InitializationKind::IK_Copy &&
  (Context.hasSameUnqualifiedType(SourceType, DestType) ||
-  S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType
-  TryConstructorInitialization(S, Entity, Kind, Args,
-   DestType, DestType, *this);
+  (Initializer &&
+   S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType)
+  TryConstructorInitialization(S, Entity, Kind, Args, DestType, DestType,
+   *this);
 // - Otherwise (i.e., for the remaining copy-initialization cases),
 //   user-defined conversion sequences that can convert from the source
 //   type to the destination type or (when a conversion function is
@@ -6027,8 +6028,8 @@
 bool NeedAtomicConversion = false;
 if (const AtomicType *Atomic = DestType->getAs()) {
   if (Context.hasSameUnqualifiedType(SourceType, Atomic->getValueType()) ||
-  S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType,
-  Atomic->getValueType())) {
+  (Initializer && S.IsDerivedFrom(Initializer->getBeginLoc(),
+  SourceType, Atomic->getValueType())) {
 DestType = Atomic->getValueType();
 NeedAtomicConversion = true;
   }
@@ -6045,7 +6046,7 @@
   //- Otherwise, if the initialization is direct-initialization, the source
   //type is std::nullptr_t, and the destination type is bool, the initial
   //value of 

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

[Sindhu Chittireddy via Phabricator via cfe-commits]

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 as well since we are here.

Yes, it does make sense.

> I am guessing the answer is no since this was found using a static analysis 
> tool but do you have a test case that triggers this failure?

Indeed, the answer is no.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139148/new/

https://reviews.llvm.org/D139148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Shafik Yaghmour via Phabricator via cfe-commits]

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 a static analysis 
tool but do you have a test case that triggers this failure?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139148/new/

https://reviews.llvm.org/D139148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sindhu Chittireddy via Phabricator via cfe-commits]

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:
  clang/lib/Sema/SemaInit.cpp


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5962,9 +5962,10 @@
 if (Kind.getKind() == InitializationKind::IK_Direct ||
 (Kind.getKind() == InitializationKind::IK_Copy &&
  (Context.hasSameUnqualifiedType(SourceType, DestType) ||
-  S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType
-  TryConstructorInitialization(S, Entity, Kind, Args,
-   DestType, DestType, *this);
+  (Initializer &&
+   S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, 
DestType)
+  TryConstructorInitialization(S, Entity, Kind, Args, DestType, DestType,
+   *this);
 // - Otherwise (i.e., for the remaining copy-initialization cases),
 //   user-defined conversion sequences that can convert from the source
 //   type to the destination type or (when a conversion function is


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5962,9 +5962,10 @@
 if (Kind.getKind() == InitializationKind::IK_Direct ||
 (Kind.getKind() == InitializationKind::IK_Copy &&
  (Context.hasSameUnqualifiedType(SourceType, DestType) ||
-  S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType
-  TryConstructorInitialization(S, Entity, Kind, Args,
-   DestType, DestType, *this);
+  (Initializer &&
+   S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType)
+  TryConstructorInitialization(S, Entity, Kind, Args, DestType, DestType,
+   *this);
 // - Otherwise (i.e., for the remaining copy-initialization cases),
 //   user-defined conversion sequences that can convert from the source
 //   type to the destination type or (when a conversion function is
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits