Revert "Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (#68394)"

This reverts commit cd184c866e0aad1f957910b8c7a94f98a2b21ceb.

1 files changed, 26 insertions, 54 deletions

diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 54d0e95..58dd711 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1008,7 +1008,7 @@ class ThreadSafetyAnalyzer {
   threadSafety::SExprBuilder SxBuilder;
 
   ThreadSafetyHandler &Handler;
-  const FunctionDecl *CurrentFunction;
+  const CXXMethodDecl *CurrentMethod = nullptr;
   LocalVariableMap LocalVarMap;
   FactManager FactMan;
   std::vector<CFGBlockInfo> BlockInfo;
@@ -1243,10 +1243,10 @@ bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr &CapE) {
 
   // Members are in scope from methods of the same class.
   if (const auto *P = dyn_cast<til::Project>(SExp)) {
-    if (!isa_and_nonnull<CXXMethodDecl>(CurrentFunction))
+    if (!CurrentMethod)
       return false;
     const ValueDecl *VD = P->clangDecl();
-    return VD->getDeclContext() == CurrentFunction->getDeclContext();
+    return VD->getDeclContext() == CurrentMethod->getDeclContext();
   }
 
   return false;
@@ -1541,8 +1541,6 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
 
   ThreadSafetyAnalyzer *Analyzer;
   FactSet FSet;
-  // The fact set for the function on exit.
-  const FactSet &FunctionExitFSet;
   /// Maps constructed objects to `this` placeholder prior to initialization.
   llvm::SmallDenseMap<const Expr *, til::LiteralPtr *> ConstructedObjects;
   LocalVariableMap::Context LVarCtx;
@@ -1568,11 +1566,9 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
                         bool SkipFirstParam = false);
 
 public:
-  BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info,
-               const FactSet &FunctionExitFSet)
+  BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info)
       : ConstStmtVisitor<BuildLockset>(), Analyzer(Anlzr), FSet(Info.EntrySet),
-        FunctionExitFSet(FunctionExitFSet), LVarCtx(Info.EntryContext),
-        CtxIndex(Info.EntryIndex) {}
+        LVarCtx(Info.EntryContext), CtxIndex(Info.EntryIndex) {}
 
   void VisitUnaryOperator(const UnaryOperator *UO);
   void VisitBinaryOperator(const BinaryOperator *BO);
@@ -1581,7 +1577,6 @@ public:
   void VisitCXXConstructExpr(const CXXConstructExpr *Exp);
   void VisitDeclStmt(const DeclStmt *S);
   void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *Exp);
-  void VisitReturnStmt(const ReturnStmt *S);
 };
 
 } // namespace
@@ -1763,8 +1758,6 @@ void ThreadSafetyAnalyzer::checkPtAccess(const FactSet &FSet, const Expr *Exp,
   // Pass by reference warnings are under a different flag.
   ProtectedOperationKind PtPOK = POK_VarDereference;
   if (POK == POK_PassByRef) PtPOK = POK_PtPassByRef;
-  if (POK == POK_ReturnByRef)
-    PtPOK = POK_PtReturnByRef;
 
   const ValueDecl *D = getValueDecl(Exp);
   if (!D || !D->hasAttrs())
@@ -2149,25 +2142,6 @@ void BuildLockset::VisitMaterializeTemporaryExpr(
   }
 }
 
-void BuildLockset::VisitReturnStmt(const ReturnStmt *S) {
-  if (Analyzer->CurrentFunction == nullptr)
-    return;
-  const Expr *RetVal = S->getRetValue();
-  if (!RetVal)
-    return;
-
-  // If returning by reference, check that the function requires the appropriate
-  // capabilities.
-  const QualType ReturnType =
-      Analyzer->CurrentFunction->getReturnType().getCanonicalType();
-  if (ReturnType->isLValueReferenceType()) {
-    Analyzer->checkAccess(
-        FunctionExitFSet, RetVal,
-        ReturnType->getPointeeType().isConstQualified() ? AK_Read : AK_Written,
-        POK_ReturnByRef);
-  }
-}
-
 /// Given two facts merging on a join point, possibly warn and decide whether to
 /// keep or replace.
 ///
@@ -2277,7 +2251,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
 
   CFG *CFGraph = walker.getGraph();
   const NamedDecl *D = walker.getDecl();
-  CurrentFunction = dyn_cast<FunctionDecl>(D);
+  const auto *CurrentFunction = dyn_cast<FunctionDecl>(D);
+  CurrentMethod = dyn_cast<CXXMethodDecl>(D);
 
   if (D->hasAttr<NoThreadSafetyAnalysisAttr>())
     return;
@@ -2303,7 +2278,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
   PostOrderCFGView::CFGBlockSet VisitedBlocks(CFGraph);
 
   CFGBlockInfo &Initial = BlockInfo[CFGraph->getEntry().getBlockID()];
-  CFGBlockInfo &Final = BlockInfo[CFGraph->getExit().getBlockID()];
+  CFGBlockInfo &Final   = BlockInfo[CFGraph->getExit().getBlockID()];
 
   // Mark entry block as reachable
   Initial.Reachable = true;
@@ -2373,25 +2348,6 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
     }
   }
 
-  // Compute the expected exit set.
-  // By default, we expect all locks held on entry to be held on exit.
-  FactSet ExpectedFunctionExitSet = Initial.EntrySet;
-
-  // Adjust the expected exit set by adding or removing locks, as declared
-  // by *-LOCK_FUNCTION and UNLOCK_FUNCTION.  The intersect below will then
-  // issue the appropriate warning.
-  // FIXME: the location here is not quite right.
-  for (const auto &Lock : ExclusiveLocksAcquired)
-    ExpectedFunctionExitSet.addLock(
-        FactMan, std::make_unique<LockableFactEntry>(Lock, LK_Exclusive,
-                                                     D->getLocation()));
-  for (const auto &Lock : SharedLocksAcquired)
-    ExpectedFunctionExitSet.addLock(
-        FactMan,
-        std::make_unique<LockableFactEntry>(Lock, LK_Shared, D->getLocation()));
-  for (const auto &Lock : LocksReleased)
-    ExpectedFunctionExitSet.removeLock(FactMan, Lock);
-
   for (const auto *CurrBlock : *SortedGraph) {
     unsigned CurrBlockID = CurrBlock->getBlockID();
     CFGBlockInfo *CurrBlockInfo = &BlockInfo[CurrBlockID];
@@ -2451,7 +2407,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
     if (!CurrBlockInfo->Reachable)
       continue;
 
-    BuildLockset LocksetBuilder(this, *CurrBlockInfo, ExpectedFunctionExitSet);
+    BuildLockset LocksetBuilder(this, *CurrBlockInfo);
 
     // Visit all the statements in the basic block.
     for (const auto &BI : *CurrBlock) {
@@ -2527,8 +2483,24 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
   if (!Final.Reachable)
     return;
 
+  // By default, we expect all locks held on entry to be held on exit.
+  FactSet ExpectedExitSet = Initial.EntrySet;
+
+  // Adjust the expected exit set by adding or removing locks, as declared
+  // by *-LOCK_FUNCTION and UNLOCK_FUNCTION.  The intersect below will then
+  // issue the appropriate warning.
+  // FIXME: the location here is not quite right.
+  for (const auto &Lock : ExclusiveLocksAcquired)
+    ExpectedExitSet.addLock(FactMan, std::make_unique<LockableFactEntry>(
+                                         Lock, LK_Exclusive, D->getLocation()));
+  for (const auto &Lock : SharedLocksAcquired)
+    ExpectedExitSet.addLock(FactMan, std::make_unique<LockableFactEntry>(
+                                         Lock, LK_Shared, D->getLocation()));
+  for (const auto &Lock : LocksReleased)
+    ExpectedExitSet.removeLock(FactMan, Lock);
+
   // FIXME: Should we call this function for all blocks which exit the function?
-  intersectAndWarn(ExpectedFunctionExitSet, Final.ExitSet, Final.ExitLoc,
+  intersectAndWarn(ExpectedExitSet, Final.ExitSet, Final.ExitLoc,
                    LEK_LockedAtEndOfFunction, LEK_NotLockedAtEndOfFunction);
 
   Handler.leaveFunction(CurrentFunction);