[PATCH] D158775: [NFC] Initialize member pointer and avoid potential null dereference

[Sindhu Chittireddy via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbbcc7c5614cd: [NFC] Initialize member pointer and avoid 
potential null dereference (authored by schittir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158775

Files:
  clang/lib/AST/Interp/Interp.h
  clang/lib/Analysis/ThreadSafety.cpp


Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1008,7 +1008,7 @@
   threadSafety::SExprBuilder SxBuilder;
 
   ThreadSafetyHandler 
-  const CXXMethodDecl *CurrentMethod;
+  const CXXMethodDecl *CurrentMethod = nullptr;
   LocalVariableMap LocalVarMap;
   FactManager FactMan;
   std::vector BlockInfo;
Index: clang/lib/AST/Interp/Interp.h
===
--- clang/lib/AST/Interp/Interp.h
+++ clang/lib/AST/Interp/Interp.h
@@ -201,13 +201,14 @@
   return false;
   }
 
+  assert(S.Current);
   assert(S.Current->getFrameOffset() == S.Stk.size() && "Invalid frame");
   if (!S.checkingPotentialConstantExpression() || S.Current->Caller) {
 // Certain builtin functions are declared as func-name(...), so the
 // parameters are checked in Sema and only available through the CallExpr.
 // The interp::Function we create for them has 0 parameters, so we need to
 // remove them from the stack by checking the CallExpr.
-if (S.Current && S.Current->getFunction()->needsRuntimeArgPop(S.getCtx()))
+if (S.Current->getFunction()->needsRuntimeArgPop(S.getCtx()))
   popBuiltinArgs(S, PC);
 else
   S.Current->popArgs();


Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1008,7 +1008,7 @@
   threadSafety::SExprBuilder SxBuilder;
 
   ThreadSafetyHandler 
-  const CXXMethodDecl *CurrentMethod;
+  const CXXMethodDecl *CurrentMethod = nullptr;
   LocalVariableMap LocalVarMap;
   FactManager FactMan;
   std::vector BlockInfo;
Index: clang/lib/AST/Interp/Interp.h
===
--- clang/lib/AST/Interp/Interp.h
+++ clang/lib/AST/Interp/Interp.h
@@ -201,13 +201,14 @@
   return false;
   }
 
+  assert(S.Current);
   assert(S.Current->getFrameOffset() == S.Stk.size() && "Invalid frame");
   if (!S.checkingPotentialConstantExpression() || S.Current->Caller) {
 // Certain builtin functions are declared as func-name(...), so the
 // parameters are checked in Sema and only available through the CallExpr.
 // The interp::Function we create for them has 0 parameters, so we need to
 // remove them from the stack by checking the CallExpr.
-if (S.Current && S.Current->getFunction()->needsRuntimeArgPop(S.getCtx()))
+if (S.Current->getFunction()->needsRuntimeArgPop(S.getCtx()))
   popBuiltinArgs(S, PC);
 else
   S.Current->popArgs();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158775: [NFC] Initialize member pointer and avoid potential null dereference

[Sindhu Chittireddy via Phabricator via cfe-commits]

schittir added a comment.

Thank you for the reviews, Aaron and Aaron! 
All tests pass. Landing the patch now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158775

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

[PATCH] D158775: [NFC] Initialize member pointer and avoid potential null dereference

[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 though I can take or leave the changes to ThreadSafety.cpp.




Comment at: clang/lib/Analysis/ThreadSafety.cpp:2243
   const auto *CurrentFunction = dyn_cast(D);
   CurrentMethod = dyn_cast(D);
 

aaronpuchert wrote:
> It seems to me that `CurrentMethod` is unconditionally assigned a value here, 
> before it is being read anywhere.
It is, but this isn't the `ThreadSafetyAnalyzer` constructor, so there's a 
window for misuse. So I think this is a false positive, but still a reasonable 
enough change to make.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158775

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

[PATCH] D158775: [NFC] Initialize member pointer and avoid potential null dereference

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/lib/Analysis/ThreadSafety.cpp:2243
   const auto *CurrentFunction = dyn_cast(D);
   CurrentMethod = dyn_cast(D);
 

It seems to me that `CurrentMethod` is unconditionally assigned a value here, 
before it is being read anywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158775

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

[PATCH] D158775: [NFC] Initialize member pointer and avoid potential null dereference

[Sindhu Chittireddy via Phabricator via cfe-commits]

schittir created this revision.
schittir added reviewers: aaron.ballman, tahonermann.
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.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158775

Files:
  clang/lib/AST/Interp/Interp.h
  clang/lib/Analysis/ThreadSafety.cpp


Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1008,7 +1008,7 @@
   threadSafety::SExprBuilder SxBuilder;
 
   ThreadSafetyHandler 
-  const CXXMethodDecl *CurrentMethod;
+  const CXXMethodDecl *CurrentMethod = nullptr;
   LocalVariableMap LocalVarMap;
   FactManager FactMan;
   std::vector BlockInfo;
Index: clang/lib/AST/Interp/Interp.h
===
--- clang/lib/AST/Interp/Interp.h
+++ clang/lib/AST/Interp/Interp.h
@@ -201,13 +201,14 @@
   return false;
   }
 
+  assert(S.Current);
   assert(S.Current->getFrameOffset() == S.Stk.size() && "Invalid frame");
   if (!S.checkingPotentialConstantExpression() || S.Current->Caller) {
 // Certain builtin functions are declared as func-name(...), so the
 // parameters are checked in Sema and only available through the CallExpr.
 // The interp::Function we create for them has 0 parameters, so we need to
 // remove them from the stack by checking the CallExpr.
-if (S.Current && S.Current->getFunction()->needsRuntimeArgPop(S.getCtx()))
+if (S.Current->getFunction()->needsRuntimeArgPop(S.getCtx()))
   popBuiltinArgs(S, PC);
 else
   S.Current->popArgs();


Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1008,7 +1008,7 @@
   threadSafety::SExprBuilder SxBuilder;
 
   ThreadSafetyHandler 
-  const CXXMethodDecl *CurrentMethod;
+  const CXXMethodDecl *CurrentMethod = nullptr;
   LocalVariableMap LocalVarMap;
   FactManager FactMan;
   std::vector BlockInfo;
Index: clang/lib/AST/Interp/Interp.h
===
--- clang/lib/AST/Interp/Interp.h
+++ clang/lib/AST/Interp/Interp.h
@@ -201,13 +201,14 @@
   return false;
   }
 
+  assert(S.Current);
   assert(S.Current->getFrameOffset() == S.Stk.size() && "Invalid frame");
   if (!S.checkingPotentialConstantExpression() || S.Current->Caller) {
 // Certain builtin functions are declared as func-name(...), so the
 // parameters are checked in Sema and only available through the CallExpr.
 // The interp::Function we create for them has 0 parameters, so we need to
 // remove them from the stack by checking the CallExpr.
-if (S.Current && S.Current->getFunction()->needsRuntimeArgPop(S.getCtx()))
+if (S.Current->getFunction()->needsRuntimeArgPop(S.getCtx()))
   popBuiltinArgs(S, PC);
 else
   S.Current->popArgs();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits