[PATCH] D152977: [NFC] Fix potential dereferencing of null return value.

[Sindhu Chittireddy via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc2888cddd5d9: [NFC] Fix potential dereferencing of null 
return value. (authored by schittir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152977

Files:
  clang/lib/AST/ASTContext.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
  clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -291,6 +291,7 @@
   // Existence in DestroyRetVal ensures existence in LockMap.
   // Existence in Destroyed also ensures that the lock state for lockR is 
either
   // UntouchedAndPossiblyDestroyed or UnlockedAndPossiblyDestroyed.
+  assert(lstate);
   assert(lstate->isUntouchedAndPossiblyDestroyed() ||
  lstate->isUnlockedAndPossiblyDestroyed());
 
Index: clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
@@ -321,7 +321,9 @@
   if (!IvarRegion)
 return nullptr;
 
-  return IvarRegion->getSymbolicBase()->getSymbol();
+  const SymbolicRegion *SR = IvarRegion->getSymbolicBase();
+  assert(SR && "Symbolic base should not be nullptr");
+  return SR->getSymbol();
 }
 
 /// If we are in -dealloc or -dealloc is on the stack, handle the call if it is
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -2790,7 +2790,7 @@
 
   // Only support _BitInt elements with byte-sized power of 2 NumBits.
   if (T->isBitIntType()) {
-unsigned NumBits = T->getAs()->getNumBits();
+unsigned NumBits = T->castAs()->getNumBits();
 if (!llvm::isPowerOf2_32(NumBits) || NumBits < 8) {
   Diag(AttrLoc, diag::err_attribute_invalid_bitint_vector_type)
   << (NumBits < 8);
Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -458,6 +458,8 @@
   return;
 }
 
+assert(NamedCtx && "NamedCtx cannot be null");
+
 if (const auto *Decl = dyn_cast(NamedTemplate)) {
   OS << "unnamed function parameter " << Decl->getFunctionScopeIndex()
  << " ";
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -4141,8 +4141,8 @@
   assert(vecType->isBuiltinType() || vecType->isDependentType() ||
  (vecType->isBitIntType() &&
   // Only support _BitInt elements with byte-sized power of 2 NumBits.
-  llvm::isPowerOf2_32(vecType->getAs()->getNumBits()) &&
-  vecType->getAs()->getNumBits() >= 8));
+  llvm::isPowerOf2_32(vecType->castAs()->getNumBits()) &&
+  vecType->castAs()->getNumBits() >= 8));
 
   // Check if we've already instantiated a vector of this type.
   llvm::FoldingSetNodeID ID;


Index: clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -291,6 +291,7 @@
   // Existence in DestroyRetVal ensures existence in LockMap.
   // Existence in Destroyed also ensures that the lock state for lockR is either
   // UntouchedAndPossiblyDestroyed or UnlockedAndPossiblyDestroyed.
+  assert(lstate);
   assert(lstate->isUntouchedAndPossiblyDestroyed() ||
  lstate->isUnlockedAndPossiblyDestroyed());
 
Index: clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
@@ -321,7 +321,9 @@
   if (!IvarRegion)
 return nullptr;
 
-  return IvarRegion->getSymbolicBase()->getSymbol();
+  const SymbolicRegion *SR = IvarRegion->getSymbolicBase();
+  assert(SR && "Symbolic base should not be nullptr");
+  return SR->getSymbol();
 }
 
 /// If we are in -dealloc or -dealloc is on the stack, handle the call if it is
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -2790,7 +2790,7 @@
 
   // Only support _BitInt elements with byte-sized power of 2 NumBits.
   if (T->isBitIntType()) {
-unsigned NumBits 

[PATCH] D152977: [NFC] Fix potential dereferencing of null return value.

[Sindhu Chittireddy via Phabricator via cfe-commits]

schittir added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:294-295
   // UntouchedAndPossiblyDestroyed or UnlockedAndPossiblyDestroyed.
-  assert(lstate->isUntouchedAndPossiblyDestroyed() ||
- lstate->isUnlockedAndPossiblyDestroyed());
+  assert(lstate && (lstate->isUntouchedAndPossiblyDestroyed() ||
+lstate->isUnlockedAndPossiblyDestroyed()));
 

steakhal wrote:
> schittir wrote:
> > steakhal wrote:
> > > 
> > Wouldn't it be better to do an with a comment, like below?  
> > ```
> > assert(lstate && "lstate should not be null");
> > ```
> As a Static Analyzer dev I don't think its necessary. StateRefs are 
> ubiquitous and here we probably know it cannot be null. And if it turns out 
> to be null we would get a segfault. So that sense I don't think its necessary.
> 
> 
> And speaking of a comment like "it should not be null" I think the segfault 
> would sort of imply that.
That makes sense! Thanks!


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

https://reviews.llvm.org/D152977

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

[PATCH] D152977: [NFC] Fix potential dereferencing of null return value.

[Balázs Benics via Phabricator via cfe-commits]

steakhal accepted this revision.
steakhal added a comment.

Looks good. Thanks!




Comment at: clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:294-295
   // UntouchedAndPossiblyDestroyed or UnlockedAndPossiblyDestroyed.
-  assert(lstate->isUntouchedAndPossiblyDestroyed() ||
- lstate->isUnlockedAndPossiblyDestroyed());
+  assert(lstate && (lstate->isUntouchedAndPossiblyDestroyed() ||
+lstate->isUnlockedAndPossiblyDestroyed()));
 

schittir wrote:
> steakhal wrote:
> > 
> Wouldn't it be better to do an with a comment, like below?  
> ```
> assert(lstate && "lstate should not be null");
> ```
As a Static Analyzer dev I don't think its necessary. StateRefs are ubiquitous 
and here we probably know it cannot be null. And if it turns out to be null we 
would get a segfault. So that sense I don't think its necessary.


And speaking of a comment like "it should not be null" I think the segfault 
would sort of imply that.


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

https://reviews.llvm.org/D152977

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

[PATCH] D152977: [NFC] Fix potential dereferencing of null return value.

[Sindhu Chittireddy via Phabricator via cfe-commits]

schittir added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:294-295
   // UntouchedAndPossiblyDestroyed or UnlockedAndPossiblyDestroyed.
-  assert(lstate->isUntouchedAndPossiblyDestroyed() ||
- lstate->isUnlockedAndPossiblyDestroyed());
+  assert(lstate && (lstate->isUntouchedAndPossiblyDestroyed() ||
+lstate->isUnlockedAndPossiblyDestroyed()));
 

steakhal wrote:
> 
Wouldn't it be better to do an with a comment, like below?  
```
assert(lstate && "lstate should not be null");
```


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

https://reviews.llvm.org/D152977

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

[PATCH] D152977: [NFC] Fix potential dereferencing of null return value.

[Sindhu Chittireddy via Phabricator via cfe-commits]

schittir added inline comments.



Comment at: clang/lib/Frontend/FrontendActions.cpp:461
 
+assert(NamedCtx, "NamedCtx cannot be null");
+

aaron.ballman wrote:
> This doesn't build. ;-)
Thanks for the catch. Something was wrong with my workspace build setup, and 
didn't diagnose this at all! I changed this in the new patch. 


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

https://reviews.llvm.org/D152977

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

[PATCH] D152977: [NFC] Fix potential dereferencing of null return value.

[Sindhu Chittireddy via Phabricator via cfe-commits]

schittir updated this revision to Diff 531824.
schittir marked an inline comment as done.
schittir added a comment.

Fix the assert causing build failure and address another review comment.


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

https://reviews.llvm.org/D152977

Files:
  clang/lib/AST/ASTContext.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
  clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -291,6 +291,7 @@
   // Existence in DestroyRetVal ensures existence in LockMap.
   // Existence in Destroyed also ensures that the lock state for lockR is 
either
   // UntouchedAndPossiblyDestroyed or UnlockedAndPossiblyDestroyed.
+  assert(lstate);
   assert(lstate->isUntouchedAndPossiblyDestroyed() ||
  lstate->isUnlockedAndPossiblyDestroyed());
 
Index: clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
@@ -321,7 +321,9 @@
   if (!IvarRegion)
 return nullptr;
 
-  return IvarRegion->getSymbolicBase()->getSymbol();
+  const SymbolicRegion *SR = IvarRegion->getSymbolicBase();
+  assert(SR && "Symbolic base should not be nullptr");
+  return SR->getSymbol();
 }
 
 /// If we are in -dealloc or -dealloc is on the stack, handle the call if it is
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -2790,7 +2790,7 @@
 
   // Only support _BitInt elements with byte-sized power of 2 NumBits.
   if (T->isBitIntType()) {
-unsigned NumBits = T->getAs()->getNumBits();
+unsigned NumBits = T->castAs()->getNumBits();
 if (!llvm::isPowerOf2_32(NumBits) || NumBits < 8) {
   Diag(AttrLoc, diag::err_attribute_invalid_bitint_vector_type)
   << (NumBits < 8);
Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -458,6 +458,8 @@
   return;
 }
 
+assert(NamedCtx && "NamedCtx cannot be null");
+
 if (const auto *Decl = dyn_cast(NamedTemplate)) {
   OS << "unnamed function parameter " << Decl->getFunctionScopeIndex()
  << " ";
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -4141,8 +4141,8 @@
   assert(vecType->isBuiltinType() || vecType->isDependentType() ||
  (vecType->isBitIntType() &&
   // Only support _BitInt elements with byte-sized power of 2 NumBits.
-  llvm::isPowerOf2_32(vecType->getAs()->getNumBits()) &&
-  vecType->getAs()->getNumBits() >= 8));
+  llvm::isPowerOf2_32(vecType->castAs()->getNumBits()) &&
+  vecType->castAs()->getNumBits() >= 8));
 
   // Check if we've already instantiated a vector of this type.
   llvm::FoldingSetNodeID ID;


Index: clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -291,6 +291,7 @@
   // Existence in DestroyRetVal ensures existence in LockMap.
   // Existence in Destroyed also ensures that the lock state for lockR is either
   // UntouchedAndPossiblyDestroyed or UnlockedAndPossiblyDestroyed.
+  assert(lstate);
   assert(lstate->isUntouchedAndPossiblyDestroyed() ||
  lstate->isUnlockedAndPossiblyDestroyed());
 
Index: clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
@@ -321,7 +321,9 @@
   if (!IvarRegion)
 return nullptr;
 
-  return IvarRegion->getSymbolicBase()->getSymbol();
+  const SymbolicRegion *SR = IvarRegion->getSymbolicBase();
+  assert(SR && "Symbolic base should not be nullptr");
+  return SR->getSymbol();
 }
 
 /// If we are in -dealloc or -dealloc is on the stack, handle the call if it is
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -2790,7 +2790,7 @@
 
   // Only support _BitInt elements with byte-sized power of 2 NumBits.
   if (T->isBitIntType()) {
-unsigned NumBits = T->getAs()->getNumBits();
+

[PATCH] D152977: [NFC] Fix potential dereferencing of null return value.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:294-295
   // UntouchedAndPossiblyDestroyed or UnlockedAndPossiblyDestroyed.
-  assert(lstate->isUntouchedAndPossiblyDestroyed() ||
- lstate->isUnlockedAndPossiblyDestroyed());
+  assert(lstate && (lstate->isUntouchedAndPossiblyDestroyed() ||
+lstate->isUnlockedAndPossiblyDestroyed()));
 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152977

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

[PATCH] D152977: [NFC] Fix potential dereferencing of null return value.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM with the think-o fixed.




Comment at: clang/lib/Frontend/FrontendActions.cpp:461
 
+assert(NamedCtx, "NamedCtx cannot be null");
+

This doesn't build. ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152977

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

[PATCH] D152977: [NFC] Fix potential dereferencing of null return value.

[Sindhu Chittireddy via Phabricator via cfe-commits]

schittir created this revision.
schittir added reviewers: aaron.ballman, tahonermann, erichkeane.
Herald added subscribers: steakhal, martong.
Herald added a reviewer: NoQ.
Herald added a project: All.
schittir requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Replace getAs with castAs and add assert if needed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152977

Files:
  clang/lib/AST/ASTContext.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
  clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -291,8 +291,8 @@
   // Existence in DestroyRetVal ensures existence in LockMap.
   // Existence in Destroyed also ensures that the lock state for lockR is 
either
   // UntouchedAndPossiblyDestroyed or UnlockedAndPossiblyDestroyed.
-  assert(lstate->isUntouchedAndPossiblyDestroyed() ||
- lstate->isUnlockedAndPossiblyDestroyed());
+  assert(lstate && (lstate->isUntouchedAndPossiblyDestroyed() ||
+lstate->isUnlockedAndPossiblyDestroyed()));
 
   ConstraintManager  = state->getConstraintManager();
   ConditionTruthVal retZero = CMgr.isNull(state, *sym);
Index: clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
@@ -321,7 +321,9 @@
   if (!IvarRegion)
 return nullptr;
 
-  return IvarRegion->getSymbolicBase()->getSymbol();
+  const SymbolicRegion *SR = IvarRegion->getSymbolicBase();
+  assert(SR && "Symbolic base should not be nullptr");
+  return SR->getSymbol();
 }
 
 /// If we are in -dealloc or -dealloc is on the stack, handle the call if it is
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -2790,7 +2790,7 @@
 
   // Only support _BitInt elements with byte-sized power of 2 NumBits.
   if (T->isBitIntType()) {
-unsigned NumBits = T->getAs()->getNumBits();
+unsigned NumBits = T->castAs()->getNumBits();
 if (!llvm::isPowerOf2_32(NumBits) || NumBits < 8) {
   Diag(AttrLoc, diag::err_attribute_invalid_bitint_vector_type)
   << (NumBits < 8);
Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -458,6 +458,8 @@
   return;
 }
 
+assert(NamedCtx, "NamedCtx cannot be null");
+
 if (const auto *Decl = dyn_cast(NamedTemplate)) {
   OS << "unnamed function parameter " << Decl->getFunctionScopeIndex()
  << " ";
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -4141,8 +4141,8 @@
   assert(vecType->isBuiltinType() || vecType->isDependentType() ||
  (vecType->isBitIntType() &&
   // Only support _BitInt elements with byte-sized power of 2 NumBits.
-  llvm::isPowerOf2_32(vecType->getAs()->getNumBits()) &&
-  vecType->getAs()->getNumBits() >= 8));
+  llvm::isPowerOf2_32(vecType->castAs()->getNumBits()) &&
+  vecType->castAs()->getNumBits() >= 8));
 
   // Check if we've already instantiated a vector of this type.
   llvm::FoldingSetNodeID ID;


Index: clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -291,8 +291,8 @@
   // Existence in DestroyRetVal ensures existence in LockMap.
   // Existence in Destroyed also ensures that the lock state for lockR is either
   // UntouchedAndPossiblyDestroyed or UnlockedAndPossiblyDestroyed.
-  assert(lstate->isUntouchedAndPossiblyDestroyed() ||
- lstate->isUnlockedAndPossiblyDestroyed());
+  assert(lstate && (lstate->isUntouchedAndPossiblyDestroyed() ||
+lstate->isUnlockedAndPossiblyDestroyed()));
 
   ConstraintManager  = state->getConstraintManager();
   ConditionTruthVal retZero = CMgr.isNull(state, *sym);
Index: clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
@@ -321,7 +321,9 @@
   if (!IvarRegion)
 return nullptr;
 
-  return