[PATCH] D58065: [analyzer] Document the frontend library

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

High-level feedback: mixing of abstraction levels is wrong for the "bundled" 
documentation. This might also work better as a blogpost, if you want to jump 
from topic to topic.




Comment at: docs/analyzer/developer-docs/FrontendLibrary.rst:11-13
+This document will describe the frontend of the Static Analyzer, basically
+everything from compiling the analyzer from source, through it's invocation up
+to the beginning of the analysis. It will touch on topics such as

NoQ wrote:
> First of all, "frontend" is, as far as i understand, a weird word to use with 
> respect to this library in general. I think what they were trying to say was 
> something like "The Static Analyzer-specific part of the C Front End's 
> command-line flags" (as opposed to Driver flags), but calling this UI "The 
> Frontend" is a bit weird. We probablyshould try to somehow avoid confusion 
> with the "compiler frontend" concept throughout this document.
+1, not sure what the word "frontend" adds here.
IMO "frontend" in folder/library name is more of a relic in this case.



Comment at: docs/analyzer/developer-docs/FrontendLibrary.rst:57-88
+Following this, the compilation goes on as usual. The fastest way of obtaining
+the analyzer for development is by configuring CMake with the following 
options:
+
+* Use the `Ninja` build system
+* Build in `Release` with asserts enabled (Only recommended for slower
+  computers!)
+* Build shared libraries

NoQ wrote:
> For the above reason i think this text deserves a better document to be put 
> into; this is definitely important to know for a much wider audience than 
> developers of libStaticAnalyzerFrontend.
Strictly speaking for tests it's better to use `check-clang-analyzer`.
Also again strictly speaking it's not possible to compile analyzer without 
compiling `clang`.



Comment at: docs/analyzer/developer-docs/FrontendLibrary.rst:85
+-fuse-ld=lld \
+../llvm
+

Information on compiling LLVM IMO should not be here.
Also, why Sphinx? Why X86? Why LLD and not gold?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58065



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

[PATCH] D57261: [analyzer] [WIP] Opt-in C Style Cast Checker for OSObject pointers

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC353566: [analyzer] Opt-in C Style Cast Checker for OSObject 
pointers (authored by george.karpenkov, committed by ).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57261?vs=183630=186035#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57261

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp
  test/Analysis/osobjectcstylecastchecker_test.cpp

Index: test/Analysis/osobjectcstylecastchecker_test.cpp
===
--- test/Analysis/osobjectcstylecastchecker_test.cpp
+++ test/Analysis/osobjectcstylecastchecker_test.cpp
@@ -0,0 +1,39 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=optin.osx.OSObjectCStyleCast %s -verify
+#include "os_object_base.h"
+
+struct OSArray : public OSObject {
+  unsigned getCount();
+};
+
+struct A {
+  int x;
+};
+struct B : public A {
+  unsigned getCount();
+};
+
+unsigned warn_on_explicit_downcast(OSObject * obj) {
+  OSArray *a = (OSArray *) obj; // expected-warning{{C-style cast of OSObject. Use OSDynamicCast instead}}
+  return a->getCount();
+}
+
+void no_warn_on_upcast(OSArray *arr) {
+  OSObject *obj = (OSObject *) arr;
+  obj->retain();
+  obj->release();
+}
+
+unsigned no_warn_on_dynamic_cast(OSObject *obj) {
+  OSArray *a = OSDynamicCast(OSArray, obj);
+  return a->getCount();
+}
+
+unsigned long no_warn_on_primitive_conversion(OSArray *arr) {
+  return (unsigned long) arr;
+}
+
+unsigned no_warn_on_other_type_cast(A *a) {
+  B *b = (B *) a;
+  return b->getCount();
+}
+
Index: lib/StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp
===
--- lib/StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp
+++ lib/StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp
@@ -0,0 +1,90 @@
+//===- OSObjectCStyleCast.cpp *- C++ -*-==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines OSObjectCStyleCast checker, which checks for C-style casts
+// of OSObjects. Such casts almost always indicate a code smell,
+// as an explicit static or dynamic cast should be used instead.
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
+#include "llvm/Support/Debug.h"
+
+using namespace clang;
+using namespace ento;
+using namespace ast_matchers;
+
+namespace {
+
+const char *WarnAtNode = "OSObjCast";
+
+class OSObjectCStyleCastChecker : public Checker {
+public:
+  void checkASTCodeBody(const Decl *D,
+AnalysisManager ,
+BugReporter ) const;
+};
+
+static void emitDiagnostics(const BoundNodes ,
+BugReporter ,
+AnalysisDeclContext *ADC,
+const OSObjectCStyleCastChecker *Checker) {
+  const auto *CE = Nodes.getNodeAs(WarnAtNode);
+  assert(CE);
+
+  std::string Diagnostics;
+  llvm::raw_string_ostream OS(Diagnostics);
+  OS << "C-style cast of OSObject. Use OSDynamicCast instead.";
+
+  BR.EmitBasicReport(
+ADC->getDecl(),
+Checker,
+/*Name=*/"OSObject C-Style Cast",
+/*Category=*/"Security",
+OS.str(),
+PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), ADC),
+CE->getSourceRange());
+}
+
+static auto hasTypePointingTo(DeclarationMatcher DeclM)
+-> decltype(hasType(pointerType())) {
+  return hasType(pointerType(pointee(hasDeclaration(DeclM;
+}
+
+void OSObjectCStyleCastChecker::checkASTCodeBody(const Decl *D, AnalysisManager ,
+ BugReporter ) const {
+
+  AnalysisDeclContext *ADC = AM.getAnalysisDeclContext(D);
+
+  auto DynamicCastM = callExpr(callee(functionDecl(hasName("safeMetaCast";
+
+  auto OSObjTypeM = hasTypePointingTo(cxxRecordDecl(isDerivedFrom("OSMetaClassBase")));
+  auto OSObjSubclassM = hasTypePointingTo(
+cxxRecordDecl(isDerivedFrom("OSObject")));
+
+  auto CastM = cStyleCastExpr(
+  allOf(hasSourceExpression(allOf(OSObjTypeM, unless(DynamicCastM))),
+  OSObjSubclassM)).bind(WarnAtNode);
+
+  auto Matches = 

[PATCH] D54978: Move the SMT API to LLVM

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

> There is at least one other conflicting commit rL353465 
>  on top of this code already.

Sorry about that. I think it would be fine to revert both or to replay the 
other commit on top of reverted version of this one.

@mikhail.ramalho could you take a look?


Repository:
  rC Clang

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

https://reviews.llvm.org/D54978



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

[PATCH] D54978: Move the SMT API to LLVM

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

@mikhail.ramalho could you revert then?
In general, we should not use Z3 unless it's explicitly requested.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54978



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

[PATCH] D57782: [analyzer] [RetainCountChecker] Bugfix: in non-OSObject-mode, do not track CXX method calls

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
george.karpenkov marked an inline comment as done.
Closed by commit rC353227: [analyzer] [RetainCountChecker] Bugfix: in 
non-OSObject-mode, do not track CXX… (authored by george.karpenkov, committed 
by ).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57782?vs=185392=185407#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57782

Files:
  lib/Analysis/RetainSummaryManager.cpp
  test/Analysis/retain-release.mm


Index: lib/Analysis/RetainSummaryManager.cpp
===
--- lib/Analysis/RetainSummaryManager.cpp
+++ lib/Analysis/RetainSummaryManager.cpp
@@ -499,19 +499,19 @@
 if (const RetainSummary *S = getSummaryForOSObject(FD, FName, RetTy))
   return S;
 
-  if (TrackObjCAndCFObjects)
-if (const RetainSummary *S =
-getSummaryForObjCOrCFObject(FD, FName, RetTy, FT, 
AllowAnnotations))
-  return S;
-
   if (const auto *MD = dyn_cast(FD))
-if (!(TrackOSObjects && isOSObjectRelated(MD)))
+if (!isOSObjectRelated(MD))
   return getPersistentSummary(RetEffect::MakeNoRet(),
   ArgEffects(AF.getEmptyMap()),
   ArgEffect(DoNothing),
   ArgEffect(StopTracking),
   ArgEffect(DoNothing));
 
+  if (TrackObjCAndCFObjects)
+if (const RetainSummary *S =
+getSummaryForObjCOrCFObject(FD, FName, RetTy, FT, 
AllowAnnotations))
+  return S;
+
   return getDefaultSummary();
 }
 
Index: test/Analysis/retain-release.mm
===
--- test/Analysis/retain-release.mm
+++ test/Analysis/retain-release.mm
@@ -471,7 +471,6 @@
   void* x = IOBSDNameMatching(); // no-warning
 }
 
-
 namespace member_CFRetains {
 class Foo {
 public:
@@ -485,3 +484,22 @@
   foo.CFRetain(0); // no-warning
 }
 }
+
+namespace cxx_method_escaping {
+
+struct S {
+  static CFArrayRef testGetNoTracking();
+  CFArrayRef testGetNoTrackingMember();
+};
+
+void test_cxx_static_method_escaping() {
+  CFArrayRef arr = S::testGetNoTracking();
+  CFRelease(arr);
+}
+
+void test_cxx_method_escaping(S *s) {
+  CFArrayRef arr = s->testGetNoTrackingMember();
+  CFRelease(arr);
+}
+
+}


Index: lib/Analysis/RetainSummaryManager.cpp
===
--- lib/Analysis/RetainSummaryManager.cpp
+++ lib/Analysis/RetainSummaryManager.cpp
@@ -499,19 +499,19 @@
 if (const RetainSummary *S = getSummaryForOSObject(FD, FName, RetTy))
   return S;
 
-  if (TrackObjCAndCFObjects)
-if (const RetainSummary *S =
-getSummaryForObjCOrCFObject(FD, FName, RetTy, FT, AllowAnnotations))
-  return S;
-
   if (const auto *MD = dyn_cast(FD))
-if (!(TrackOSObjects && isOSObjectRelated(MD)))
+if (!isOSObjectRelated(MD))
   return getPersistentSummary(RetEffect::MakeNoRet(),
   ArgEffects(AF.getEmptyMap()),
   ArgEffect(DoNothing),
   ArgEffect(StopTracking),
   ArgEffect(DoNothing));
 
+  if (TrackObjCAndCFObjects)
+if (const RetainSummary *S =
+getSummaryForObjCOrCFObject(FD, FName, RetTy, FT, AllowAnnotations))
+  return S;
+
   return getDefaultSummary();
 }
 
Index: test/Analysis/retain-release.mm
===
--- test/Analysis/retain-release.mm
+++ test/Analysis/retain-release.mm
@@ -471,7 +471,6 @@
   void* x = IOBSDNameMatching(); // no-warning
 }
 
-
 namespace member_CFRetains {
 class Foo {
 public:
@@ -485,3 +484,22 @@
   foo.CFRetain(0); // no-warning
 }
 }
+
+namespace cxx_method_escaping {
+
+struct S {
+  static CFArrayRef testGetNoTracking();
+  CFArrayRef testGetNoTrackingMember();
+};
+
+void test_cxx_static_method_escaping() {
+  CFArrayRef arr = S::testGetNoTracking();
+  CFRelease(arr);
+}
+
+void test_cxx_method_escaping(S *s) {
+  CFArrayRef arr = s->testGetNoTrackingMember();
+  CFRelease(arr);
+}
+
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57433: [analyzer] [RetainCountChecker] Bugfix for tracking top-level parameters of Objective-C methods

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352588: [analyzer] [RetainCountChecker] Bugfix for tracking 
top-level parameters of… (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57433?vs=184221=184225#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57433

Files:
  include/clang/Analysis/RetainSummaryManager.h
  lib/Analysis/RetainSummaryManager.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  test/Analysis/Inputs/expected-plists/retain-release.m.objc.plist
  test/Analysis/Inputs/expected-plists/retain-release.m.objcpp.plist
  test/Analysis/retain-release.m



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

[PATCH] D57201: [analyzer] [RetainSummaryManager] [NFC] Split one function into two, as it's really doing two things

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352533: [analyzer] [RetainSummaryManager] [NFC] Split one 
function into two, as its… (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57201?vs=183426=184140#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57201

Files:
  include/clang/Analysis/RetainSummaryManager.h
  lib/Analysis/RetainSummaryManager.cpp

Index: lib/Analysis/RetainSummaryManager.cpp
===
--- lib/Analysis/RetainSummaryManager.cpp
+++ lib/Analysis/RetainSummaryManager.cpp
@@ -557,64 +557,47 @@
   llvm_unreachable("Unknown ArgEffect kind");
 }
 
-void RetainSummaryManager::updateSummaryForCall(const RetainSummary *,
-AnyCall C,
-bool HasNonZeroCallbackArg,
-bool IsReceiverUnconsumedSelf) {
-
-  if (HasNonZeroCallbackArg) {
-ArgEffect RecEffect =
-  getStopTrackingHardEquivalent(S->getReceiverEffect());
-ArgEffect DefEffect =
-  getStopTrackingHardEquivalent(S->getDefaultArgEffect());
-
-ArgEffects ScratchArgs(AF.getEmptyMap());
-ArgEffects CustomArgEffects = S->getArgEffects();
-for (ArgEffects::iterator I = CustomArgEffects.begin(),
-  E = CustomArgEffects.end();
- I != E; ++I) {
-  ArgEffect Translated = getStopTrackingHardEquivalent(I->second);
-  if (Translated.getKind() != DefEffect.getKind())
-ScratchArgs = AF.add(ScratchArgs, I->first, Translated);
-}
+const RetainSummary *
+RetainSummaryManager::updateSummaryForNonZeroCallbackArg(const RetainSummary *S,
+ AnyCall ) {
+  ArgEffect RecEffect = getStopTrackingHardEquivalent(S->getReceiverEffect());
+  ArgEffect DefEffect = getStopTrackingHardEquivalent(S->getDefaultArgEffect());
 
-RetEffect RE = RetEffect::MakeNoRetHard();
+  ArgEffects ScratchArgs(AF.getEmptyMap());
+  ArgEffects CustomArgEffects = S->getArgEffects();
+  for (ArgEffects::iterator I = CustomArgEffects.begin(),
+E = CustomArgEffects.end();
+   I != E; ++I) {
+ArgEffect Translated = getStopTrackingHardEquivalent(I->second);
+if (Translated.getKind() != DefEffect.getKind())
+  ScratchArgs = AF.add(ScratchArgs, I->first, Translated);
+  }
 
-// Special cases where the callback argument CANNOT free the return value.
-// This can generally only happen if we know that the callback will only be
-// called when the return value is already being deallocated.
-if (C.getKind() == AnyCall::Function) {
-  if (const IdentifierInfo *Name = C.getIdentifier()) {
-// When the CGBitmapContext is deallocated, the callback here will free
-// the associated data buffer.
-// The callback in dispatch_data_create frees the buffer, but not
-// the data object.
-if (Name->isStr("CGBitmapContextCreateWithData") ||
-Name->isStr("dispatch_data_create"))
-  RE = S->getRetEffect();
-  }
-}
+  RetEffect RE = RetEffect::MakeNoRetHard();
 
-S = getPersistentSummary(RE, ScratchArgs, RecEffect, DefEffect);
+  // Special cases where the callback argument CANNOT free the return value.
+  // This can generally only happen if we know that the callback will only be
+  // called when the return value is already being deallocated.
+  if (const IdentifierInfo *Name = C.getIdentifier()) {
+// When the CGBitmapContext is deallocated, the callback here will free
+// the associated data buffer.
+// The callback in dispatch_data_create frees the buffer, but not
+// the data object.
+if (Name->isStr("CGBitmapContextCreateWithData") ||
+Name->isStr("dispatch_data_create"))
+  RE = S->getRetEffect();
   }
 
-  // Special case '[super init];' and '[self init];'
-  //
-  // Even though calling '[super init]' without assigning the result to self
-  // and checking if the parent returns 'nil' is a bad pattern, it is common.
-  // Additionally, our Self Init checker already warns about it. To avoid
-  // overwhelming the user with messages from both checkers, we model the case
-  // of '[super init]' in cases when it is not consumed by another expression
-  // as if the call preserves the value of 'self'; essentially, assuming it can
-  // never fail and return 'nil'.
-  // Note, we don't want to just stop tracking the value since we want the
-  // RetainCount checker to report leaks and use-after-free if SelfInit checker
-  // is turned off.
-  if (IsReceiverUnconsumedSelf) {
-RetainSummaryTemplate ModifiableSummaryTemplate(S, *this);
-ModifiableSummaryTemplate->setReceiverEffect(ArgEffect(DoNothing));
- 

[PATCH] D57344: Extend AnyCall to handle callable declarations without the call expressions

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352531: Extend AnyCall to handle callable declarations 
without the call expressions (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57344?vs=183975=184138#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57344

Files:
  include/clang/Analysis/AnyCall.h
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp

Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -347,7 +347,7 @@
   const Expr *CE = Call.getOriginExpr();
   AnyCall C =
   CE ? *AnyCall::forExpr(CE)
- : AnyCall::forDestructorCall(cast(Call.getDecl()));
+ : AnyCall(cast(Call.getDecl()));
   return Summaries.getSummary(C, Call.hasNonZeroCallbackArg(),
   isReceiverUnconsumedSelf(Call), ReceiverType);
 }
Index: include/clang/Analysis/AnyCall.h
===
--- include/clang/Analysis/AnyCall.h
+++ include/clang/Analysis/AnyCall.h
@@ -19,7 +19,9 @@
 
 namespace clang {
 
-/// An instance of this class corresponds to a 'callable' call.
+/// An instance of this class corresponds to a call.
+/// It might be a syntactically-concrete call, done as a part of evaluating an
+/// expression, or it may be an abstract callee with no associated expression.
 class AnyCall {
 public:
   enum Kind {
@@ -48,7 +50,11 @@
   };
 
 private:
-  /// Call expression, remains null iff the call is an implicit destructor call.
+  /// Either expression or declaration (but not both at the same time)
+  /// can be null.
+
+  /// Call expression, is null when is not known (then declaration is non-null),
+  /// or for implicit destructor calls (when no expression exists.)
   const Expr *E = nullptr;
 
   /// Corresponds to a statically known declaration of the called function,
@@ -56,8 +62,6 @@
   const Decl *D = nullptr;
   Kind K;
 
-  AnyCall(const Expr *E, const Decl *D, Kind K) : E(E), D(D), K(K) {}
-
 public:
   AnyCall(const CallExpr *CE) : E(CE) {
 D = CE->getCalleeDecl();
@@ -80,6 +84,23 @@
   AnyCall(const CXXConstructExpr *NE)
   : E(NE), D(NE->getConstructor()), K(Constructor) {}
 
+  AnyCall(const CXXDestructorDecl *D) : E(nullptr), D(D), K(Destructor) {}
+
+  AnyCall(const CXXConstructorDecl *D) : E(nullptr), D(D), K(Constructor) {}
+
+  AnyCall(const ObjCMethodDecl *D) : E(nullptr), D(D), K(ObjCMethod) {}
+
+  AnyCall(const FunctionDecl *D) : E(nullptr), D(D) {
+if (isa(D)) {
+  K = Constructor;
+} else if (isa (D)) {
+  K = Destructor;
+} else {
+  K = Function;
+}
+
+  }
+
   /// If {@code E} is a generic call (to ObjC method /function/block/etc),
   /// return a constructed {@code AnyCall} object. Return None otherwise.
   static Optional forExpr(const Expr *E) {
@@ -98,8 +119,16 @@
 }
   }
 
-  static AnyCall forDestructorCall(const CXXDestructorDecl *D) {
-return AnyCall(/*E=*/nullptr, D, Destructor);
+  /// If {@code D} is a callable (Objective-C method or a function), return
+  /// a constructed {@code AnyCall} object. Return None otherwise.
+  // FIXME: block support.
+  static Optional forDecl(const Decl *D) {
+if (const auto *FD = dyn_cast(D)) {
+  return AnyCall(FD);
+} else if (const auto *MD = dyn_cast(D)) {
+  return AnyCall(MD);
+}
+return None;
   }
 
   /// \returns formal parameters for direct calls (including virtual calls)
@@ -111,8 +140,6 @@
   return FD->parameters();
 } else if (const auto *MD = dyn_cast(D)) {
   return MD->parameters();
-} else if (const auto *CD = dyn_cast(D)) {
-  return CD->parameters();
 } else if (const auto *BD = dyn_cast(D)) {
   return BD->parameters();
 } else {
@@ -129,10 +156,17 @@
   QualType getReturnType(ASTContext ) const {
 switch (K) {
 case Function:
+  if (E)
+return cast(E)->getCallReturnType(Ctx);
+  return cast(D)->getReturnType();
+case ObjCMethod:
+  if (E)
+return cast(E)->getCallReturnType(Ctx);
+  return cast(D)->getReturnType();
 case Block:
+  // FIXME: BlockDecl does not know its return type,
+  // hence the asymmetry with the function and method cases above.
   return cast(E)->getCallReturnType(Ctx);
-case ObjCMethod:
-  return cast(E)->getCallReturnType(Ctx);
 case Destructor:
 case Constructor:
 case Allocator:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57346: [analyzer] [ARCMT] [NFC] Unify entry point into RetainSummaryManager

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352532: [analyzer] [ARCMT] [NFC] Unify entry point into 
RetainSummaryManager (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57346?vs=183916=184139#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57346

Files:
  include/clang/Analysis/RetainSummaryManager.h
  lib/ARCMigrate/ObjCMT.cpp
  lib/Analysis/RetainSummaryManager.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp

Index: lib/Analysis/RetainSummaryManager.cpp
===
--- lib/Analysis/RetainSummaryManager.cpp
+++ lib/Analysis/RetainSummaryManager.cpp
@@ -635,11 +635,14 @@
 // FIXME: These calls are currently unsupported.
 return getPersistentStopSummary();
   case AnyCall::ObjCMethod: {
-const auto *ME = cast(C.getExpr());
-if (ME->isInstanceMessage())
+const auto *ME = cast_or_null(C.getExpr());
+if (!ME) {
+  return getMethodSummary(cast(C.getDecl()));
+} else if (ME->isInstanceMessage()) {
   Summ = getInstanceMethodSummary(ME, ReceiverType);
-else
+} else {
   Summ = getClassMethodSummary(ME);
+}
 break;
   }
   }
@@ -1238,31 +1241,3 @@
 
   return getMethodSummary(S, ID, MD, ResultTy, *CachedSummaries);
 }
-
-CallEffects CallEffects::getEffect(const ObjCMethodDecl *MD) {
-  ASTContext  = MD->getASTContext();
-  RetainSummaryManager M(Ctx,
- /*TrackNSAndCFObjects=*/true,
- /*TrackOSObjects=*/false);
-  const RetainSummary *S = M.getMethodSummary(MD);
-  CallEffects CE(S->getRetEffect(), S->getReceiverEffect());
-  unsigned N = MD->param_size();
-  for (unsigned i = 0; i < N; ++i) {
-CE.Args.push_back(S->getArg(i));
-  }
-  return CE;
-}
-
-CallEffects CallEffects::getEffect(const FunctionDecl *FD) {
-  ASTContext  = FD->getASTContext();
-  RetainSummaryManager M(Ctx,
- /*TrackNSAndCFObjects=*/true,
- /*TrackOSObjects=*/false);
-  const RetainSummary *S = M.getFunctionSummary(FD);
-  CallEffects CE(S->getRetEffect());
-  unsigned N = FD->param_size();
-  for (unsigned i = 0; i < N; ++i) {
-CE.Args.push_back(S->getArg(i));
-  }
-  return CE;
-}
Index: lib/ARCMigrate/ObjCMT.cpp
===
--- lib/ARCMigrate/ObjCMT.cpp
+++ lib/ARCMigrate/ObjCMT.cpp
@@ -64,9 +64,11 @@
 ObjCInstanceTypeFamily OIT_Family = OIT_None);
 
   void migrateCFAnnotation(ASTContext , const Decl *Decl);
-  void AddCFAnnotations(ASTContext , const CallEffects ,
+  void AddCFAnnotations(ASTContext ,
+const RetainSummary *RS,
 const FunctionDecl *FuncDecl, bool ResultAnnotated);
-  void AddCFAnnotations(ASTContext , const CallEffects ,
+  void AddCFAnnotations(ASTContext ,
+const RetainSummary *RS,
 const ObjCMethodDecl *MethodDecl, bool ResultAnnotated);
 
   void AnnotateImplicitBridging(ASTContext );
@@ -84,6 +86,8 @@
 
   bool InsertFoundation(ASTContext , SourceLocation Loc);
 
+  std::unique_ptr Summaries;
+
 public:
   std::string MigrateDir;
   unsigned ASTMigrateActions;
@@ -102,6 +106,14 @@
   llvm::SmallVector CFFunctionIBCandidates;
   llvm::StringSet<> WhiteListFilenames;
 
+  RetainSummaryManager (ASTContext ) {
+if (!Summaries)
+  Summaries.reset(new RetainSummaryManager(Ctx,
+   /*TrackNSCFObjects=*/true,
+   /*TrackOSObjects=*/false));
+return *Summaries;
+  }
+
   ObjCMigrateASTConsumer(StringRef migrateDir,
  unsigned astMigrateActions,
  FileRemapper ,
@@ -1452,12 +1464,12 @@
 }
 
 void ObjCMigrateASTConsumer::AddCFAnnotations(ASTContext ,
-  const CallEffects ,
+  const RetainSummary *RS,
   const FunctionDecl *FuncDecl,
   bool ResultAnnotated) {
   // Annotate function.
   if (!ResultAnnotated) {
-RetEffect Ret = CE.getReturnValue();
+RetEffect Ret = RS->getRetEffect();
 const char *AnnotationString = nullptr;
 if (Ret.getObjKind() == ObjKind::CF) {
   if (Ret.isOwned() && NSAPIObj->isMacroDefined("CF_RETURNS_RETAINED"))
@@ -1477,12 +1489,11 @@
   Editor->commit(commit);
 }
   }
-  ArrayRef AEArgs = CE.getArgs();
   unsigned i = 0;
   for (FunctionDecl::param_const_iterator pi = FuncDecl->param_begin(),
pe = FuncDecl->param_end(); pi != pe; ++pi, ++i) {
 const ParmVarDecl *pd = *pi;
-ArgEffect AE = AEArgs[i];
+ArgEffect 

[PATCH] D57211: [analyzer] [RetainCountChecker] Support 'taggedRetain' and 'taggedRelease'

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352530: [analyzer] [RetainCountChecker] Support 
taggedRetain and taggedRelease (authored by 
george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57211?vs=183449=184137#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57211

Files:
  lib/Analysis/RetainSummaryManager.cpp
  test/Analysis/os_object_base.h
  test/Analysis/os_smart_ptr.h
  test/Analysis/osobject-retain-release.cpp

Index: test/Analysis/os_smart_ptr.h
===
--- test/Analysis/os_smart_ptr.h
+++ test/Analysis/os_smart_ptr.h
@@ -51,7 +51,6 @@
   }
 
   T * operator->() const {
-OSPTR_LOG("Dereference smart_ptr with %p\n", pointer);
 return pointer;
   }
 
@@ -84,6 +83,6 @@
 
   T *pointer;
 };
-}
+} // namespace os
 
 #endif /* _OS_SMART_POINTER_H */
Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -593,12 +593,12 @@
// expected-note@-1{{Returning from constructor for 'smart_ptr'}}
 // expected-note@os_smart_ptr.h:13{{Taking true branch}}
 // expected-note@os_smart_ptr.h:14{{Calling 'smart_ptr::_retain'}}
-// expected-note@os_smart_ptr.h:72{{Reference count incremented. The object now has a +2 retain count}}
+// expected-note@os_smart_ptr.h:71{{Reference count incremented. The object now has a +2 retain count}}
 // expected-note@os_smart_ptr.h:14{{Returning from 'smart_ptr::_retain'}}
   } // expected-note{{Calling '~smart_ptr'}}
   // expected-note@os_smart_ptr.h:35{{Taking true branch}}
   // expected-note@os_smart_ptr.h:36{{Calling 'smart_ptr::_release'}}
-  // expected-note@os_smart_ptr.h:77{{Reference count decremented. The object now has a +1 retain count}}
+  // expected-note@os_smart_ptr.h:76{{Reference count decremented. The object now has a +1 retain count}}
   // expected-note@os_smart_ptr.h:36{{Returning from 'smart_ptr::_release'}}
  // expected-note@-5{{Returning from '~smart_ptr'}}
   obj->release(); // expected-note{{Object released}}
@@ -613,12 +613,12 @@
// expected-note@-1{{Returning from constructor for 'smart_ptr'}}
 // expected-note@os_smart_ptr.h:13{{Taking true branch}}
 // expected-note@os_smart_ptr.h:14{{Calling 'smart_ptr::_retain'}}
-// expected-note@os_smart_ptr.h:72{{Reference count incremented. The object now has a +2 retain count}}
+// expected-note@os_smart_ptr.h:71{{Reference count incremented. The object now has a +2 retain count}}
 // expected-note@os_smart_ptr.h:14{{Returning from 'smart_ptr::_retain'}}
   } // expected-note{{Calling '~smart_ptr'}}
   // expected-note@os_smart_ptr.h:35{{Taking true branch}}
   // expected-note@os_smart_ptr.h:36{{Calling 'smart_ptr::_release'}}
-  // expected-note@os_smart_ptr.h:77{{Reference count decremented. The object now has a +1 retain count}}
+  // expected-note@os_smart_ptr.h:76{{Reference count decremented. The object now has a +1 retain count}}
   // expected-note@os_smart_ptr.h:36{{Returning from 'smart_ptr::_release'}}
  // expected-note@-5{{Returning from '~smart_ptr'}}
 } // expected-warning{{Potential leak of an object stored into 'obj'}}
@@ -648,3 +648,15 @@
   // expected-warning@-1{{'free' called on an object that may be referenced elsewhere}}
 }
 
+void test_tagged_retain_no_leak() {
+  OSObject *obj = new OSObject;
+  obj->taggedRelease();
+}
+
+void test_tagged_retain_no_uaf() {
+  OSObject *obj = new OSObject;
+  obj->taggedRetain();
+  obj->release();
+  obj->release();
+}
+
Index: test/Analysis/os_object_base.h
===
--- test/Analysis/os_object_base.h
+++ test/Analysis/os_object_base.h
@@ -27,6 +27,10 @@
 
   virtual void retain() const;
   virtual void release() const;
+
+  virtual void taggedRetain(const void * tag = nullptr) const;
+  virtual void taggedRelease(const void * tag = nullptr) const;
+
   virtual void free();
   virtual ~OSMetaClassBase(){};
 };
Index: lib/Analysis/RetainSummaryManager.cpp
===
--- lib/Analysis/RetainSummaryManager.cpp
+++ lib/Analysis/RetainSummaryManager.cpp
@@ -242,10 +242,10 @@
   if (const auto *MD = dyn_cast(FD)) {
 const CXXRecordDecl *Parent = MD->getParent();
 if (TrackOSObjects && Parent && isOSObjectSubclass(Parent)) {
-  if (FName == "release")
+  if (FName == "release" || FName == "taggedRelease")
 return getOSSummaryReleaseRule(FD);
 
-  if (FName == "retain")
+  if (FName == "retain" || FName == "taggedRetain")
 return getOSSummaryRetainRule(FD);
 
   if (FName == "free")
___
cfe-commits 

[PATCH] D54438: [analyzer] Reimplement dependencies between checkers

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

After this landed, options for RetainCountChecker stopped working - e.g. I 
can't use `osx.cocoa.RetainCount:blah=X`.
Do you know why is this the case / how to fix it?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54438



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

[PATCH] D52790: [analyzer][PlistMacroExpansion] New flag to convert macro expansions to events

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

I personally do use HTML output quite a lot (and we do have non-Xcode 
projects), and complex macros in HTML are currently totally unusable.
I'm not sure whether this is a right approach to handling this, but it's 
definitely a problem for us.
In general - thanks for working on this anyway.


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

https://reviews.llvm.org/D52790



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

[PATCH] D57230: [analyzer] Toning down invalidation a bit

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

I'm in favor of this change, I never understood how invalidating a field 
invalidates entire structure.


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

https://reviews.llvm.org/D57230



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

[PATCH] D57127: [analyzer] Port RetainSummaryManager to the new GenericCall interface, decouple ARCMT from the analyzer

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

@thakis Sorry I don't understand your point. At the end of the day, 
ProgramPoint and BodyFarm are only used by the static analyzer.
If you disagree, what is your proposed directory structure? What would be the 
benefits?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57127



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

[PATCH] D57127: [analyzer] Port RetainSummaryManager to the new GenericCall interface, decouple ARCMT from the analyzer

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

> Also, isn't lib/Analysis a strange place for this? lib/Analysis is used by 
> the compiler proper (CodeGen, Sema, …), while RetainSummaryManager is only 
> used by tooling-like things (static analyzer, arcmigrate).

Not only, it has utilities used by the static analyzer which are too generic to 
be in the static analyzer itself - ProgramPoint.cpp, BodyFarm.cpp, 
CocoaConventions.cpp to name a few.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57127



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

[PATCH] D57127: [analyzer] Port RetainSummaryManager to the new GenericCall interface, decouple ARCMT from the analyzer

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

> Can you elaborate a bit in what sense this decouples ARCMT from the analyzer?

ARCMT now does not need to link against the analyzer

> Can CLANG_ENABLE_STATIC_ANALYZER now be set independently of 
> CLANG_ENABLE_ARCMT?

Yes


Repository:
  rC Clang

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

https://reviews.llvm.org/D57127



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

[PATCH] D57204: [AST] Add a method to get a call type from an ObjCMessageExpr

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352147: [AST] Add a method to get a call type from an 
ObjCMessageExpr (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57204?vs=183439=183453#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57204

Files:
  include/clang/AST/ExprObjC.h
  lib/AST/ExprObjC.cpp


Index: include/clang/AST/ExprObjC.h
===
--- include/clang/AST/ExprObjC.h
+++ include/clang/AST/ExprObjC.h
@@ -1180,6 +1180,13 @@
   /// sent to.
   ReceiverKind getReceiverKind() const { return (ReceiverKind)Kind; }
 
+  /// \return the return type of the message being sent.
+  /// This is not always the type of the message expression itself because
+  /// of references (the expression would not have a reference type).
+  /// It is also not always the declared return type of the method because
+  /// of `instancetype` (in that case it's an expression type).
+  QualType getCallReturnType(ASTContext ) const;
+
   /// Source range of the receiver.
   SourceRange getReceiverRange() const;
 
Index: lib/AST/ExprObjC.cpp
===
--- lib/AST/ExprObjC.cpp
+++ lib/AST/ExprObjC.cpp
@@ -292,6 +292,31 @@
 SelLocs.push_back(getSelectorLoc(i));
 }
 
+
+QualType ObjCMessageExpr::getCallReturnType(ASTContext ) const {
+  if (const ObjCMethodDecl *MD = getMethodDecl()) {
+QualType QT = MD->getReturnType();
+if (QT == Ctx.getObjCInstanceType()) {
+  // instancetype corresponds to expression types.
+  return getType();
+}
+return QT;
+  }
+
+  // Expression type might be different from an expected call return type,
+  // as expression type would never be a reference even if call returns a
+  // reference. Reconstruct the original expression type.
+  QualType QT = getType();
+  switch (getValueKind()) {
+  case VK_LValue:
+return Ctx.getLValueReferenceType(QT);
+  case VK_XValue:
+return Ctx.getRValueReferenceType(QT);
+  case VK_RValue:
+return QT;
+  }
+}
+
 SourceRange ObjCMessageExpr::getReceiverRange() const {
   switch (getReceiverKind()) {
   case Instance:


Index: include/clang/AST/ExprObjC.h
===
--- include/clang/AST/ExprObjC.h
+++ include/clang/AST/ExprObjC.h
@@ -1180,6 +1180,13 @@
   /// sent to.
   ReceiverKind getReceiverKind() const { return (ReceiverKind)Kind; }
 
+  /// \return the return type of the message being sent.
+  /// This is not always the type of the message expression itself because
+  /// of references (the expression would not have a reference type).
+  /// It is also not always the declared return type of the method because
+  /// of `instancetype` (in that case it's an expression type).
+  QualType getCallReturnType(ASTContext ) const;
+
   /// Source range of the receiver.
   SourceRange getReceiverRange() const;
 
Index: lib/AST/ExprObjC.cpp
===
--- lib/AST/ExprObjC.cpp
+++ lib/AST/ExprObjC.cpp
@@ -292,6 +292,31 @@
 SelLocs.push_back(getSelectorLoc(i));
 }
 
+
+QualType ObjCMessageExpr::getCallReturnType(ASTContext ) const {
+  if (const ObjCMethodDecl *MD = getMethodDecl()) {
+QualType QT = MD->getReturnType();
+if (QT == Ctx.getObjCInstanceType()) {
+  // instancetype corresponds to expression types.
+  return getType();
+}
+return QT;
+  }
+
+  // Expression type might be different from an expected call return type,
+  // as expression type would never be a reference even if call returns a
+  // reference. Reconstruct the original expression type.
+  QualType QT = getType();
+  switch (getValueKind()) {
+  case VK_LValue:
+return Ctx.getLValueReferenceType(QT);
+  case VK_XValue:
+return Ctx.getRValueReferenceType(QT);
+  case VK_RValue:
+return QT;
+  }
+}
+
 SourceRange ObjCMessageExpr::getReceiverRange() const {
   switch (getReceiverKind()) {
   case Instance:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57062: [analyzer] Re-enable the "System is over constrained" assertion on optimized builds.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov requested changes to this revision.
george.karpenkov added a comment.
This revision now requires changes to proceed.

We would start getting crashes from `TrustNonnullChecker` if we enable it. 
Shouldn't those be fixed first?
An input which crashes with this assertion is 
`test/Analysis/trustnonnullchecker_test.m`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57062



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

[PATCH] D55400: [analyzer] Move out tracking retain count for OSObjects into a separate checker

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

> Hmmm, does this mess with options that bad? Could you please clarify?

`registerChecker` gets-or-creates a checker object. A checker name (used for 
getting the options) is set the first time it's created.
The checker which was created first "wins" and gets to name the resulting 
checker.
In practice it basically means that options and checkers reusing the same class 
do not work.
Do you have better ideas on how this could be arranged?

I think the current situation is a mess - ideally I would prefer to be able to 
access the options in the constructor, but we can't even do that,
since `registerChecker` sets the checker name and is called after the object 
has been constructed.
It seems that it would only make sense if the checker name is known at the time 
the checker is constructed: probably the function `registerXChecker` should get 
it as an argument.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55400



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

[PATCH] D55400: [analyzer] Move out tracking retain count for OSObjects into a separate checker

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

> Deal with the consequences of this, and just correct all plist files to now 
> refer to the new base checker.

What does it mean?

> Each time a report is emitted from these checkers, create a ProgramPointTag, 
> and "manually" make sure the emitted checker name doesn't change.

I'm not sure what do you propose exactly, but sounds pretty bad.

> Rename osx.cocoa.RetainCount to something else. This one is clearly 
> nonsensical, but let's put it here for the sake of completeness.

I don't think we can rename the old checker, as older Xcode versions have 
"osx.cocoa.RetainCount" hardcoded in them.

TBH I'm not really sold on the way we "bundle" multiple checkers (from the 
tablegen) into a single class.
For one, that means options don't work at all, and essentially the checker name 
is defined by the class which was registered by the tablegen first (which 
should not even be visible, and should be an internal implementation detail).
Do you have better proposals on how, conceptually, grouped checkers should be 
organized?

Essentially, we have a single checker class with two checks, which should be 
toggle-able orthogonally from each other.
For legacy reasons, we can't quite get rid of `osx.cocoa.RetainCount` (but even 
if we could, what then?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D55400



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

[PATCH] D56899: [analyzer] pr37688: Fix a crash on trying to evaluate a deleted destructor of a union.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov accepted this revision.
george.karpenkov added a comment.
This revision is now accepted and ready to land.

The code is fine, but I obviously would prefer a proper fix in a CFG


Repository:
  rC Clang

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

https://reviews.llvm.org/D56899



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

[PATCH] D56891: [analyzer] Introduce proper diagnostic for freeing unowned object

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC351514: [analyzer] Introduce proper diagnostic for freeing 
unowned object (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56891?vs=182431=182445#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D56891

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
  test/Analysis/osobject-retain-release.cpp

Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
@@ -30,6 +30,7 @@
 UseAfterRelease,
 ReleaseNotOwned,
 DeallocNotOwned,
+FreeNotOwned,
 OverAutorelease,
 ReturnNotOwnedForOwned,
 LeakWithinFunction,
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
@@ -255,6 +255,7 @@
   RefCountBug useAfterRelease{this, RefCountBug::UseAfterRelease};
   RefCountBug releaseNotOwned{this, RefCountBug::ReleaseNotOwned};
   RefCountBug deallocNotOwned{this, RefCountBug::DeallocNotOwned};
+  RefCountBug freeNotOwned{this, RefCountBug::FreeNotOwned};
   RefCountBug overAutorelease{this, RefCountBug::OverAutorelease};
   RefCountBug returnNotOwnedForOwned{this, RefCountBug::ReturnNotOwnedForOwned};
   RefCountBug leakWithinFunction{this, RefCountBug::LeakWithinFunction};
@@ -336,8 +337,8 @@
RefVal V, ArgEffect E, RefVal::Kind ,
CheckerContext ) const;
 
-
-  const RefCountBug (RefVal::Kind ErrorKind) const;
+  const RefCountBug (RefVal::Kind ErrorKind,
+SymbolRef Sym) const;
 
   void processNonLeakError(ProgramStateRef St, SourceRange ErrorRange,
RefVal::Kind ErrorKind, SymbolRef Sym,
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
@@ -27,6 +27,8 @@
 return "Bad release";
   case DeallocNotOwned:
 return "-dealloc sent to non-exclusively owned object";
+  case FreeNotOwned:
+return "freeing non-exclusively owned object";
   case OverAutorelease:
 return "Object autoreleased too many times";
   case ReturnNotOwnedForOwned:
@@ -47,6 +49,8 @@
"not owned at this point by the caller";
   case DeallocNotOwned:
 return "-dealloc sent to object that may be referenced elsewhere";
+  case FreeNotOwned:
+return  "'free' called on an object that may be referenced elsewhere";
   case OverAutorelease:
 return "Object autoreleased too many times";
   case ReturnNotOwnedForOwned:
@@ -86,7 +90,8 @@
 /// Write information about the type state change to {@code os},
 /// return whether the note should be generated.
 static bool shouldGenerateNote(llvm::raw_string_ostream ,
-   const RefVal *PrevT, const RefVal ,
+   const RefVal *PrevT,
+   const RefVal ,
bool DeallocSent) {
   // Get the previous type state.
   RefVal PrevV = *PrevT;
@@ -416,6 +421,11 @@
 RefCountReportVisitor::VisitNode(const ExplodedNode *N,
   BugReporterContext , BugReport ) {
 
+  const auto  = static_cast(BR.getBugType());
+
+  bool IsFreeUnowned = BT.getBugType() == RefCountBug::FreeNotOwned ||
+   BT.getBugType() == RefCountBug::DeallocNotOwned;
+
   const SourceManager  = BRC.getSourceManager();
   CallEventManager  = BRC.getStateManager().getCallEventManager();
   if (auto CE = N->getLocationAs())
@@ -434,7 +444,8 @@
   const LocationContext *LCtx = N->getLocationContext();
 
   const RefVal* CurrT = getRefBinding(CurrSt, Sym);
-  if (!CurrT) return nullptr;
+  if (!CurrT)
+return nullptr;
 
   const RefVal  = *CurrT;
   const RefVal *PrevT = getRefBinding(PrevSt, Sym);
@@ -444,6 +455,12 @@
   std::string sbuf;
   llvm::raw_string_ostream os(sbuf);
 
+  if (PrevT && IsFreeUnowned && CurrV.isNotOwned() && PrevT->isOwned()) {
+os << "Object is now not exclusively owned";
+auto Pos = 

[PATCH] D56885: [analyzer] const-ify reference to bug type used in BugReporter

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC351511: [analyzer] const-ify reference to bug type used in 
BugReporter (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56885?vs=182412=182443#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D56885

Files:
  include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
  lib/StaticAnalyzer/Core/BugReporter.cpp

Index: include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
===
--- include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
+++ include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -95,7 +95,7 @@
   friend class BugReportEquivClass;
   friend class BugReporter;
 
-  BugType& BT;
+  const BugType& BT;
   const Decl *DeclWithIssue = nullptr;
   std::string ShortDescription;
   std::string Description;
@@ -164,15 +164,15 @@
   void popInterestingSymbolsAndRegions();
 
 public:
-  BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode)
+  BugReport(const BugType& bt, StringRef desc, const ExplodedNode *errornode)
   : BT(bt), Description(desc), ErrorNode(errornode) {}
 
-  BugReport(BugType& bt, StringRef shortDesc, StringRef desc,
+  BugReport(const BugType& bt, StringRef shortDesc, StringRef desc,
 const ExplodedNode *errornode)
   : BT(bt), ShortDescription(shortDesc), Description(desc),
 ErrorNode(errornode) {}
 
-  BugReport(BugType , StringRef desc, PathDiagnosticLocation l)
+  BugReport(const BugType , StringRef desc, PathDiagnosticLocation l)
   : BT(bt), Description(desc), Location(l) {}
 
   /// Create a BugReport with a custom uniqueing location.
@@ -190,7 +190,7 @@
   virtual ~BugReport();
 
   const BugType& getBugType() const { return BT; }
-  BugType& getBugType() { return BT; }
+  //BugType& getBugType() { return BT; }
 
   /// True when the report has an execution path associated with it.
   ///
@@ -481,7 +481,7 @@
 return {};
   }
 
-  void Register(BugType *BT);
+  void Register(const BugType *BT);
 
   /// Add the given report to the set of reports tracked by BugReporter.
   ///
Index: lib/StaticAnalyzer/Core/BugReporter.cpp
===
--- lib/StaticAnalyzer/Core/BugReporter.cpp
+++ lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -1247,7 +1247,7 @@
 
 static std::unique_ptr
 generateEmptyDiagnosticForReport(BugReport *R, SourceManager ) {
-  BugType  = R->getBugType();
+  const BugType  = R->getBugType();
   return llvm::make_unique(
   R->getBugType().getCheckName(), R->getDeclWithIssue(),
   R->getBugType().getName(), R->getDescription(),
@@ -2684,7 +2684,7 @@
   return Out;
 }
 
-void BugReporter::Register(BugType *BT) {
+void BugReporter::Register(const BugType *BT) {
   BugTypes = F.add(BugTypes, BT);
 }
 
@@ -2718,7 +2718,7 @@
   R->Profile(ID);
 
   // Lookup the equivance class.  If there isn't one, create it.
-  BugType& BT = R->getBugType();
+  const BugType& BT = R->getBugType();
   Register();
   void *InsertPos;
   BugReportEquivClass* EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos);
@@ -2836,7 +2836,7 @@
  SmallVectorImpl ) {
   BugReportEquivClass::iterator I = EQ.begin(), E = EQ.end();
   assert(I != E);
-  BugType& BT = I->getBugType();
+  const BugType& BT = I->getBugType();
 
   // If we don't need to suppress any of the nodes because they are
   // post-dominated by a sink, simply add all the nodes in the equivalence class
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
@@ -51,7 +51,7 @@
   StringRef endText);
 
   llvm::iterator_range getRanges() override {
-const RefCountBug& BugTy = static_cast(getBugType());
+const RefCountBug& BugTy = static_cast(getBugType());
 if (!BugTy.isLeak())
   return BugReport::getRanges();
 return llvm::make_range(ranges_iterator(), ranges_iterator());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56884: [analyzer] Extend BugType constructor to accept "SuppressOnSink" as a parameter

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC351510: [analyzer] Extend BugType constructor to accept 
SuppressOnSink as a parameter (authored by george.karpenkov, 
committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56884?vs=182434=182442#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D56884

Files:
  include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
  lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
  lib/StaticAnalyzer/Checkers/ValistChecker.cpp

Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
@@ -26,8 +26,10 @@
 
 class RefCountBug : public BugType {
 protected:
-  RefCountBug(const CheckerBase *checker, StringRef name)
-  : BugType(checker, name, categories::MemoryRefCount) {}
+  RefCountBug(const CheckerBase *checker, StringRef name,
+  bool SuppressOnSink=false)
+  : BugType(checker, name, categories::MemoryRefCount,
+SuppressOnSink) {}
 
 public:
   virtual const char *getDescription() const = 0;
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -92,10 +92,10 @@
 
 class Leak : public RefCountBug {
 public:
-  Leak(const CheckerBase *checker, StringRef name) : RefCountBug(checker, name) {
-// Leaks should not be reported if they are post-dominated by a sink.
-setSuppressOnSink(true);
-  }
+  // Leaks should not be reported if they are post-dominated by a sink.
+  Leak(const CheckerBase *checker, StringRef name)
+  : RefCountBug(checker, name,
+/*SuppressOnSink=*/true) {}
 
   const char *getDescription() const override { return ""; }
 
Index: lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
+++ lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
@@ -109,10 +109,10 @@
   DoubleCloseBugType.reset(
   new BugType(this, "Double fclose", "Unix Stream API Error"));
 
-  LeakBugType.reset(
-  new BugType(this, "Resource Leak", "Unix Stream API Error"));
   // Sinks are higher importance bugs as well as calls to assert() or exit(0).
-  LeakBugType->setSuppressOnSink(true);
+  LeakBugType.reset(
+  new BugType(this, "Resource Leak", "Unix Stream API Error",
+  /*SuppressOnSink=*/true));
 }
 
 void SimpleStreamChecker::checkPostCall(const CallEvent ,
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -399,14 +399,14 @@
 
 IteratorChecker::IteratorChecker() {
   OutOfRangeBugType.reset(
-  new BugType(this, "Iterator out of range", "Misuse of STL APIs"));
-  OutOfRangeBugType->setSuppressOnSink(true);
+  new BugType(this, "Iterator out of range", "Misuse of STL APIs",
+  /*SuppressOnSink=*/true));
   MismatchedBugType.reset(
-  new BugType(this, "Iterator(s) mismatched", "Misuse of STL APIs"));
-  MismatchedBugType->setSuppressOnSink(true);
+  new BugType(this, "Iterator(s) mismatched", "Misuse of STL APIs",
+  /*SuppressOnSink=*/true));
   InvalidatedBugType.reset(
-  new BugType(this, "Iterator invalidated", "Misuse of STL APIs"));
-  InvalidatedBugType->setSuppressOnSink(true);
+  new BugType(this, "Iterator invalidated", "Misuse of STL APIs",
+  /*SuppressOnSink=*/true));
 }
 
 void IteratorChecker::checkPreCall(const CallEvent ,
Index: lib/StaticAnalyzer/Checkers/ValistChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ValistChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ValistChecker.cpp
@@ -276,8 +276,8 @@
   new BugType(CheckNames[CK_Unterminated].getName().empty()
   ? CheckNames[CK_Uninitialized]
   : CheckNames[CK_Unterminated],
-  "Leaked va_list", categories::MemoryError));
-  BT_leakedvalist->setSuppressOnSink(true);
+  "Leaked va_list", categories::MemoryError,
+  

[PATCH] D56820: [analyzer] [RetainCountChecker] Produce a correct message when OSTypeAlloc is used

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC351509: [analyzer] [RetainCountChecker] Produce a correct 
message when OSTypeAlloc is… (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56820?vs=182194=182441#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D56820

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  test/Analysis/os_object_base.h
  test/Analysis/osobject-retain-release.cpp


Index: test/Analysis/os_object_base.h
===
--- test/Analysis/os_object_base.h
+++ test/Analysis/os_object_base.h
@@ -47,7 +47,7 @@
 };
 
 struct OSMetaClass : public OSMetaClassBase {
-  virtual OSObject * alloc();
+  virtual OSObject * alloc() const;
   virtual ~OSMetaClass(){}
 };
 
Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -627,3 +627,10 @@
   }
   obj->release();
 }
+
+void test_ostypealloc_correct_diagnostic_name() {
+  OSArray *arr = OSTypeAlloc(OSArray); // expected-note{{Call to method 
'OSMetaClass::alloc' returns an OSObject of type 'OSArray' with a +1 retain 
count}}
+  arr->retain(); // expected-note{{Reference count incremented. The object now 
has a +2 retain count}}
+  arr->release(); // expected-note{{Reference count decremented. The object 
now has a +1 retain count}}
+} // expected-note{{Object leaked: object allocated and stored into 'arr' is 
not referenced later in this execution path and has a retain count of +1}}
+  // expected-warning@-1{{Potential leak of an object stored into 'arr'}}
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
@@ -132,6 +132,32 @@
   return None;
 }
 
+Optional findMetaClassAlloc(const Expr *Callee) {
+  if (const auto *ME = dyn_cast(Callee)) {
+if (ME->getMemberDecl()->getNameAsString() != "alloc")
+  return None;
+const Expr *This = ME->getBase()->IgnoreParenImpCasts();
+if (const auto *DRE = dyn_cast(This)) {
+  const ValueDecl *VD = DRE->getDecl();
+  if (VD->getNameAsString() != "metaClass")
+return None;
+
+  if (const auto *RD = dyn_cast(VD->getDeclContext()))
+return RD->getNameAsString();
+
+}
+  }
+  return None;
+}
+
+std::string findAllocatedObjectName(const Stmt *S,
+QualType QT) {
+  if (const auto *CE = dyn_cast(S))
+if (auto Out = findMetaClassAlloc(CE->getCallee()))
+  return *Out;
+  return getPrettyTypeName(QT);
+}
+
 static void generateDiagnosticsForCallLike(ProgramStateRef CurrSt,
const LocationContext *LCtx,
const RefVal , SymbolRef ,
@@ -189,7 +215,7 @@
 os << "a Core Foundation object of type '"
<< Sym->getType().getAsString() << "' with a ";
   } else if (CurrV.getObjKind() == ObjKind::OS) {
-os << "an OSObject of type '" << getPrettyTypeName(Sym->getType())
+os << "an OSObject of type '" << findAllocatedObjectName(S, Sym->getType())
<< "' with a ";
   } else if (CurrV.getObjKind() == ObjKind::Generalized) {
 os << "an object of type '" << Sym->getType().getAsString()


Index: test/Analysis/os_object_base.h
===
--- test/Analysis/os_object_base.h
+++ test/Analysis/os_object_base.h
@@ -47,7 +47,7 @@
 };
 
 struct OSMetaClass : public OSMetaClassBase {
-  virtual OSObject * alloc();
+  virtual OSObject * alloc() const;
   virtual ~OSMetaClass(){}
 };
 
Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -627,3 +627,10 @@
   }
   obj->release();
 }
+
+void test_ostypealloc_correct_diagnostic_name() {
+  OSArray *arr = OSTypeAlloc(OSArray); // expected-note{{Call to method 'OSMetaClass::alloc' returns an OSObject of type 'OSArray' with a +1 retain count}}
+  arr->retain(); // expected-note{{Reference count incremented. The object now has a +2 retain count}}
+  arr->release(); // expected-note{{Reference count decremented. The object now has a +1 retain count}}
+} // expected-note{{Object leaked: object allocated and stored into 'arr' is not referenced later in this execution path and has a retain count of +1}}
+  // expected-warning@-1{{Potential leak of an object stored into 'arr'}}
Index: 

[PATCH] D56823: [analyzer] Do not try to body-farm bodies for Objective-C properties with custom accessors.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

LG, but it sounds like something which can skew results a lot (?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D56823



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

[PATCH] D56759: [analyzer] Another RetainCountChecker cleanup

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC351394: [analyzer] Another RetainCountChecker cleanup 
(authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56759?vs=182100=182170#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D56759

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  test/Analysis/osobject-retain-release.cpp

Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -529,12 +529,24 @@
   C.addTransition(state);
 }
 
+/// A value escapes in these possible cases:
+///
+/// - binding to something that is not a memory region.
+/// - binding to a memregion that does not have stack storage
+/// - binding to a variable that has a destructor attached using CleanupAttr
+///
+/// We do not currently model what happens when a symbol is
+/// assigned to a struct field, so be conservative here and let the symbol go.
+/// FIXME: This could definitely be improved upon.
 static bool shouldEscapeRegion(const MemRegion *R) {
+  const auto *VR = dyn_cast(R);
+  if (!R->hasStackStorage() || !VR)
+return true;
 
-  // We do not currently model what happens when a symbol is
-  // assigned to a struct field, so be conservative here and let the symbol
-  // go. TODO: This could definitely be improved upon.
-  return !R->hasStackStorage() || !isa(R);
+  const VarDecl *VD = VR->getDecl();
+  if (!VD->hasAttr())
+return false; // CleanupAttr attaches destructors, which cause escaping.
+  return true;
 }
 
 static SmallVector
@@ -1145,39 +1157,15 @@
 
 void RetainCountChecker::checkBind(SVal loc, SVal val, const Stmt *S,
CheckerContext ) const {
-  // Are we storing to something that causes the value to "escape"?
-  bool escapes = true;
-
-  // A value escapes in three possible cases (this may change):
-  //
-  // (1) we are binding to something that is not a memory region.
-  // (2) we are binding to a memregion that does not have stack storage
   ProgramStateRef state = C.getState();
+  const MemRegion *MR = loc.getAsRegion();
 
-  if (auto regionLoc = loc.getAs()) {
-escapes = shouldEscapeRegion(regionLoc->getRegion());
-  }
-
-  // If we are storing the value into an auto function scope variable annotated
-  // with (__attribute__((cleanup))), stop tracking the value to avoid leak
-  // false positives.
-  if (const auto *LVR = dyn_cast_or_null(loc.getAsRegion())) {
-const VarDecl *VD = LVR->getDecl();
-if (VD->hasAttr()) {
-  escapes = true;
-}
-  }
-
-  // If our store can represent the binding and we aren't storing to something
-  // that doesn't have local storage then just return and have the simulation
-  // state continue as is.
-  if (!escapes)
-  return;
-
-  // Otherwise, find all symbols referenced by 'val' that we are tracking
+  // Find all symbols referenced by 'val' that we are tracking
   // and stop tracking them.
-  state = state->scanReachableSymbols(val).getState();
-  C.addTransition(state);
+  if (MR && shouldEscapeRegion(MR)) {
+state = state->scanReachableSymbols(val).getState();
+C.addTransition(state);
+  }
 }
 
 ProgramStateRef RetainCountChecker::evalAssume(ProgramStateRef state,
@@ -1196,14 +1184,14 @@
 
   bool changed = false;
   RefBindingsTy::Factory  = state->get_context();
+  ConstraintManager  = state->getConstraintManager();
 
-  for (RefBindingsTy::iterator I = B.begin(), E = B.end(); I != E; ++I) {
+  for (auto  : B) {
 // Check if the symbol is null stop tracking the symbol.
-ConstraintManager  = state->getConstraintManager();
-ConditionTruthVal AllocFailed = CMgr.isNull(state, I.getKey());
+ConditionTruthVal AllocFailed = CMgr.isNull(state, I.first);
 if (AllocFailed.isConstrainedTrue()) {
   changed = true;
-  B = RefBFactory.remove(B, I.getKey());
+  B = RefBFactory.remove(B, I.first);
 }
   }
 
@@ -1424,9 +1412,9 @@
 return;
   }
 
-  for (RefBindingsTy::iterator I = B.begin(), E = B.end(); I != E; ++I) {
+  for (auto  : B) {
 state = handleAutoreleaseCounts(state, Pred, /*Tag=*/nullptr, Ctx,
-I->first, I->second);
+I.first, I.second);
 if (!state)
   return;
   }
@@ -1441,8 +1429,8 @@
   B = state->get();
   SmallVector Leaked;
 
-  for (RefBindingsTy::iterator I = B.begin(), E = B.end(); I != E; ++I)
-state = handleSymbolDeath(state, I->first, I->second, Leaked);
+  for (auto  : B)
+state = handleSymbolDeath(state, I.first, I.second, Leaked);
 
   processLeaks(state, Leaked, Ctx, Pred);
 }
@@ 

[PATCH] D55226: [Fix][StaticAnalyzer] Bug 39792 - False positive on strcpy targeting struct member

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL351097: [analyzer] [PR39792] false positive on strcpy 
targeting struct members (authored by george.karpenkov, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55226?vs=176676=181603#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55226

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  cfe/trunk/test/Analysis/security-syntax-checks.m


Index: cfe/trunk/test/Analysis/security-syntax-checks.m
===
--- cfe/trunk/test/Analysis/security-syntax-checks.m
+++ cfe/trunk/test/Analysis/security-syntax-checks.m
@@ -177,6 +177,11 @@
   strcpy(x, "abcd");
 }
 
+void test_strcpy_safe_2() {
+  struct {char s1[100];} s;
+  strcpy(s.s1, "hello");
+}
+
 //===--===
 // strcat()
 //===--===
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -651,14 +651,14 @@
 
   const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
  *Source = CE->getArg(1)->IgnoreImpCasts();
-  if (const auto *DeclRef = dyn_cast(Target))
-if (const auto *Array = dyn_cast(DeclRef->getType())) {
-  uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
-  if (const auto *String = dyn_cast(Source)) {
-if (ArraySize >= String->getLength() + 1)
-  return;
-  }
+
+  if (const auto *Array = dyn_cast(Target->getType())) {
+uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
+if (const auto *String = dyn_cast(Source)) {
+  if (ArraySize >= String->getLength() + 1)
+return;
 }
+  }
 
   // Issue a warning.
   PathDiagnosticLocation CELoc =


Index: cfe/trunk/test/Analysis/security-syntax-checks.m
===
--- cfe/trunk/test/Analysis/security-syntax-checks.m
+++ cfe/trunk/test/Analysis/security-syntax-checks.m
@@ -177,6 +177,11 @@
   strcpy(x, "abcd");
 }
 
+void test_strcpy_safe_2() {
+  struct {char s1[100];} s;
+  strcpy(s.s1, "hello");
+}
+
 //===--===
 // strcat()
 //===--===
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -651,14 +651,14 @@
 
   const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
  *Source = CE->getArg(1)->IgnoreImpCasts();
-  if (const auto *DeclRef = dyn_cast(Target))
-if (const auto *Array = dyn_cast(DeclRef->getType())) {
-  uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
-  if (const auto *String = dyn_cast(Source)) {
-if (ArraySize >= String->getLength() + 1)
-  return;
-  }
+
+  if (const auto *Array = dyn_cast(Target->getType())) {
+uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
+if (const auto *String = dyn_cast(Source)) {
+  if (ArraySize >= String->getLength() + 1)
+return;
 }
+  }
 
   // Issue a warning.
   PathDiagnosticLocation CELoc =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55226: [Fix][StaticAnalyzer] Bug 39792 - False positive on strcpy targeting struct member

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

@Pierre-vh The patch does not compile due to unmatched braces. Please do test 
and compile before submitting!


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

https://reviews.llvm.org/D55226



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

[PATCH] D55226: [Fix][StaticAnalyzer] Bug 39792 - False positive on strcpy targeting struct member

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

Whoops, sorry. There were holidays, and then I did forget about this patch. 
I'll commit this now.


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

https://reviews.llvm.org/D55226



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

[PATCH] D55616: Emit ASM input in a constant context

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

Apologies - the value seems to indeed overflow, but I'm still very confused how 
this was affected by this change.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55616



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

[PATCH] D55616: Emit ASM input in a constant context

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

@void @efriedma I can't build XNU anymore after this commit, with an error 
message:

  osfmk/i386/genassym.c:298:2: error: value '18446743523953737728' out of range 
for constraint 'n'
  DECLAREULL("KERNEL_BASE", KERNEL_BASE);
  ^~
  osfmk/i386/genassym.c:107:65: note: expanded from macro 'DECLAREULL'
  __asm("DEFINITION__define__" SYM ":\t .ascii \"%0\"" : : "n"  
((unsigned long long)(VAL)))
 
^

The error message looks wrong: the value is in range to fit in `unsigned long 
long` (but I'm not an inline assembly expert).

At a minimum, could we add a test case to this change to show in which cases an 
extra diagnostics would appear?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55616



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

[PATCH] D56402: [analyzer] [NFC] [RetainCountChecker] Remove dead unused map

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC350868: [analyzer] [NFC] [RetainCountChecker] Remove dead 
unused map (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56402?vs=180534=181098#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D56402

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h


Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
@@ -256,11 +256,6 @@
   mutable std::unique_ptr overAutorelease, returnNotOwnedForOwned;
   mutable std::unique_ptr leakWithinFunction, leakAtReturn;
 
-  typedef llvm::DenseMap 
SymbolTagMap;
-
-  // This map is only used to ensure proper deletion of any allocated tags.
-  mutable SymbolTagMap DeadSymbolTags;
-
   mutable std::unique_ptr Summaries;
 public:
   static constexpr const char *DeallocTagDescription = "DeallocSent";
@@ -273,7 +268,6 @@
 
   RetainCountChecker() {}
 
-  ~RetainCountChecker() override { DeleteContainerSeconds(DeadSymbolTags); }
 
   CFRefBug *getLeakWithinFunctionBug(const LangOptions ) const;
 


Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
@@ -256,11 +256,6 @@
   mutable std::unique_ptr overAutorelease, returnNotOwnedForOwned;
   mutable std::unique_ptr leakWithinFunction, leakAtReturn;
 
-  typedef llvm::DenseMap SymbolTagMap;
-
-  // This map is only used to ensure proper deletion of any allocated tags.
-  mutable SymbolTagMap DeadSymbolTags;
-
   mutable std::unique_ptr Summaries;
 public:
   static constexpr const char *DeallocTagDescription = "DeallocSent";
@@ -273,7 +268,6 @@
 
   RetainCountChecker() {}
 
-  ~RetainCountChecker() override { DeleteContainerSeconds(DeadSymbolTags); }
 
   CFRefBug *getLeakWithinFunctionBug(const LangOptions ) const;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55875: [analyzer] pr38668: RegionStore: Do not attempt to cast loaded values of non-scalar types.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov accepted this revision.
george.karpenkov added a comment.

Seems good, but a comment explaining why this is necessary and why the crash 
follows otherwise would be great.


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

https://reviews.llvm.org/D55875



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

[PATCH] D55978: [gn build] Add build files for clang/lib/{ASTMatchers, CrossTU}, clang/lib/StaticAnalyzer/{Checkers, Core, Frontend}

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

> I might want to default to clang_enable_static_analyzer=false when I add the 
> clang/lib/FrontendTool build file.

I don't think that quite makes sense, since by default clang does have the 
analyzer built in.


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

https://reviews.llvm.org/D55978



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

[PATCH] D55978: [gn build] Add build files for clang/lib/{ASTMatchers, CrossTU}, clang/lib/StaticAnalyzer/{Checkers, Core, Frontend}

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

Looks reasonable, what about linking with Z3? Or is your goal just to get a 
minimally working functionality?


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

https://reviews.llvm.org/D55978



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

[PATCH] D55976: [analyzer] Perform escaping in RetainCountChecker on type mismatch even for inlined functions

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC349876: [analyzer] Perform escaping in RetainCountChecker on 
type mismatch even for… (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55976?vs=179220=179227#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55976

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  test/Analysis/osobject-retain-release.cpp

Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -90,7 +90,10 @@
   static OSObject *safeMetaCast(const OSObject *inst, const OSMetaClass *meta);
 };
 
+typedef unsigned long MYTYPE;
+
 void escape(void *);
+void escape_with_source(MYTYPE p) {}
 bool coin();
 
 bool os_consume_violation_two_args(OS_CONSUME OSObject *obj, bool extra) {
@@ -139,6 +142,13 @@
   escape(obj);
 }
 
+void test_escape_has_source() {
+  OSObject *obj = new OSObject;
+  if (obj)
+escape_with_source((MYTYPE)obj);
+  return;
+}
+
 void test_no_infinite_check_recursion(MyArray *arr) {
   OSObject *input = new OSObject;
   OSObject *o = arr->generateObject(input);
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -502,6 +502,25 @@
   return None;
 }
 
+static bool isPointerToObject(QualType QT) {
+  QualType PT = QT->getPointeeType();
+  if (!PT.isNull())
+if (PT->getAsCXXRecordDecl())
+  return true;
+  return false;
+}
+
+/// Whether the tracked value should be escaped on a given call.
+/// OSObjects are escaped when passed to void * / etc.
+static bool shouldEscapeArgumentOnCall(const CallEvent , unsigned ArgIdx,
+   const RefVal *TrackedValue) {
+  if (TrackedValue->getObjKind() != RetEffect::OS)
+return false;
+  if (ArgIdx >= CE.parameters().size())
+return false;
+  return !isPointerToObject(CE.parameters()[ArgIdx]->getType());
+}
+
 // We don't always get the exact modeling of the function with regards to the
 // retain count checker even when the function is inlined. For example, we need
 // to stop tracking the symbols which were marked with StopTrackingHard.
@@ -512,11 +531,16 @@
 
   // Evaluate the effect of the arguments.
   for (unsigned idx = 0, e = CallOrMsg.getNumArgs(); idx != e; ++idx) {
-if (Summ.getArg(idx) == StopTrackingHard) {
-  SVal V = CallOrMsg.getArgSVal(idx);
-  if (SymbolRef Sym = V.getAsLocSymbol()) {
+SVal V = CallOrMsg.getArgSVal(idx);
+
+if (SymbolRef Sym = V.getAsLocSymbol()) {
+  bool ShouldRemoveBinding = Summ.getArg(idx) == StopTrackingHard;
+  if (const RefVal *T = getRefBinding(state, Sym))
+if (shouldEscapeArgumentOnCall(CallOrMsg, idx, T))
+  ShouldRemoveBinding = true;
+
+  if (ShouldRemoveBinding)
 state = removeRefBinding(state, Sym);
-  }
 }
   }
 
@@ -574,25 +598,6 @@
   return State;
 }
 
-static bool isPointerToObject(QualType QT) {
-  QualType PT = QT->getPointeeType();
-  if (!PT.isNull())
-if (PT->getAsCXXRecordDecl())
-  return true;
-  return false;
-}
-
-/// Whether the tracked value should be escaped on a given call.
-/// OSObjects are escaped when passed to void * / etc.
-static bool shouldEscapeArgumentOnCall(const CallEvent , unsigned ArgIdx,
-   const RefVal *TrackedValue) {
-  if (TrackedValue->getObjKind() != RetEffect::OS)
-return false;
-  if (ArgIdx >= CE.parameters().size())
-return false;
-  return !isPointerToObject(CE.parameters()[ArgIdx]->getType());
-}
-
 void RetainCountChecker::checkSummary(const RetainSummary ,
   const CallEvent ,
   CheckerContext ) const {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55907: [analyzer] RetainCount: Bluntly suppress the CFRetain detection heuristic on a couple of CM functions.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

In general, I'm against hardcoding, and I think we should use the annotations 
indicating that the function does retain.
(or use a naming convention, and use an annotation to indicate that the 
function does not retain).

However, if this is prohibitive for some reason, we could use this approach as 
well.


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

https://reviews.llvm.org/D55907



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

[PATCH] D55823: [analyzer] Fix backward compatibility issue after D53280 'Emit an error for invalid -analyzer-config inputs'

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:3700
+  // through them.
+  for (size_t Index = 0; Index < Args.size(); ++Index) {
+if (StringRef(Args.getArgString(Index)).contains("-analyzer-config")) {

NoQ wrote:
> Needs an LLVM-style loop!~ :)
Shouldn't this be under else- for the previous branch? Otherwise it seems that 
the option would be added twice.


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

https://reviews.llvm.org/D55823



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

[PATCH] D55792: Allow direct navigation to static analysis checker documentation through SARIF exports

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

This is amazing, huge thanks for doing this!


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

https://reviews.llvm.org/D55792



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

[PATCH] D54921: [analyzer] Remove memoization from RunLoopAutoreleaseLeakChecker

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

@NoQ thanks, interesting! But I thought the underlying map is not actually 
modified while the reference is alive?


Repository:
  rC Clang

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

https://reviews.llvm.org/D54921



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

[PATCH] D54921: [analyzer] Remove memoization from RunLoopAutoreleaseLeakChecker

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348822: [analyzer] Remove memoization from 
RunLoopAutoreleaseLeakChecker (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54921?vs=175351=177635#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D54921

Files:
  lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp


Index: lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
@@ -58,13 +58,12 @@
 
 } // end anonymous namespace
 
-
-using TriBoolTy = Optional;
-using MemoizationMapTy = llvm::DenseMap>;
-
-static TriBoolTy
-seenBeforeRec(const Stmt *Parent, const Stmt *A, const Stmt *B,
-  MemoizationMapTy ) {
+/// \return Whether {@code A} occurs before {@code B} in traversal of
+/// {@code Parent}.
+/// Conceptually a very incomplete/unsound approximation of happens-before
+/// relationship (A is likely to be evaluated before B),
+/// but useful enough in this case.
+static bool seenBefore(const Stmt *Parent, const Stmt *A, const Stmt *B) {
   for (const Stmt *C : Parent->children()) {
 if (!C) continue;
 
@@ -74,26 +73,9 @@
 if (C == B)
   return false;
 
-Optional  = Memoization[C];
-if (!Cached)
-  Cached = seenBeforeRec(C, A, B, Memoization);
-
-if (Cached->hasValue())
-  return Cached->getValue();
+return seenBefore(C, A, B);
   }
-
-  return None;
-}
-
-/// \return Whether {@code A} occurs before {@code B} in traversal of
-/// {@code Parent}.
-/// Conceptually a very incomplete/unsound approximation of happens-before
-/// relationship (A is likely to be evaluated before B),
-/// but useful enough in this case.
-static bool seenBefore(const Stmt *Parent, const Stmt *A, const Stmt *B) {
-  MemoizationMapTy Memoization;
-  TriBoolTy Val = seenBeforeRec(Parent, A, B, Memoization);
-  return Val.getValue();
+  return false;
 }
 
 static void emitDiagnostics(BoundNodes ,


Index: lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
@@ -58,13 +58,12 @@
 
 } // end anonymous namespace
 
-
-using TriBoolTy = Optional;
-using MemoizationMapTy = llvm::DenseMap>;
-
-static TriBoolTy
-seenBeforeRec(const Stmt *Parent, const Stmt *A, const Stmt *B,
-  MemoizationMapTy ) {
+/// \return Whether {@code A} occurs before {@code B} in traversal of
+/// {@code Parent}.
+/// Conceptually a very incomplete/unsound approximation of happens-before
+/// relationship (A is likely to be evaluated before B),
+/// but useful enough in this case.
+static bool seenBefore(const Stmt *Parent, const Stmt *A, const Stmt *B) {
   for (const Stmt *C : Parent->children()) {
 if (!C) continue;
 
@@ -74,26 +73,9 @@
 if (C == B)
   return false;
 
-Optional  = Memoization[C];
-if (!Cached)
-  Cached = seenBeforeRec(C, A, B, Memoization);
-
-if (Cached->hasValue())
-  return Cached->getValue();
+return seenBefore(C, A, B);
   }
-
-  return None;
-}
-
-/// \return Whether {@code A} occurs before {@code B} in traversal of
-/// {@code Parent}.
-/// Conceptually a very incomplete/unsound approximation of happens-before
-/// relationship (A is likely to be evaluated before B),
-/// but useful enough in this case.
-static bool seenBefore(const Stmt *Parent, const Stmt *A, const Stmt *B) {
-  MemoizationMapTy Memoization;
-  TriBoolTy Val = seenBeforeRec(Parent, A, B, Memoization);
-  return Val.getValue();
+  return false;
 }
 
 static void emitDiagnostics(BoundNodes ,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55528: [analyzer] Resolve another bug where the name of the leaked object was not printed properly

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348819: [analyzer] Resolve another bug where the name of the 
leaked object was not… (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55528?vs=177601=177631#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55528

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  test/Analysis/osobject-retain-release.cpp


Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
@@ -405,11 +405,11 @@
 
 if (FB) {
   const MemRegion *R = FB.getRegion();
-  const VarRegion *VR = R->getBaseRegion()->getAs();
   // Do not show local variables belonging to a function other than
   // where the error is reported.
-  if (!VR || VR->getStackFrame() == LeakContext->getStackFrame())
-FirstBinding = R;
+  if (auto MR = dyn_cast(R->getMemorySpace()))
+if (MR->getStackFrame() == LeakContext->getStackFrame())
+  FirstBinding = R;
 }
 
 // AllocationNode is the last node in which the symbol was tracked.
Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -153,8 +153,8 @@
 
 unsigned int check_leak_explicit_new() {
   OSArray *arr = new OSArray; // expected-note{{Operator new returns an 
OSObject of type OSArray with a +1 retain count}}
-  return arr->getCount(); // expected-note{{Object leaked: allocated object of 
type OSArray is not referenced later in this execution path and has a retain 
count of +1}}
-  // expected-warning@-1{{Potential leak of an object 
of type OSArray}}
+  return arr->getCount(); // expected-note{{Object leaked: object allocated 
and stored into 'arr' is not referenced later in this execution path and has a 
retain count of +1}}
+  // expected-warning@-1{{Potential leak of an object 
stored into 'arr'}}
 }
 
 unsigned int check_leak_factory() {


Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
@@ -405,11 +405,11 @@
 
 if (FB) {
   const MemRegion *R = FB.getRegion();
-  const VarRegion *VR = R->getBaseRegion()->getAs();
   // Do not show local variables belonging to a function other than
   // where the error is reported.
-  if (!VR || VR->getStackFrame() == LeakContext->getStackFrame())
-FirstBinding = R;
+  if (auto MR = dyn_cast(R->getMemorySpace()))
+if (MR->getStackFrame() == LeakContext->getStackFrame())
+  FirstBinding = R;
 }
 
 // AllocationNode is the last node in which the symbol was tracked.
Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -153,8 +153,8 @@
 
 unsigned int check_leak_explicit_new() {
   OSArray *arr = new OSArray; // expected-note{{Operator new returns an OSObject of type OSArray with a +1 retain count}}
-  return arr->getCount(); // expected-note{{Object leaked: allocated object of type OSArray is not referenced later in this execution path and has a retain count of +1}}
-  // expected-warning@-1{{Potential leak of an object of type OSArray}}
+  return arr->getCount(); // expected-note{{Object leaked: object allocated and stored into 'arr' is not referenced later in this execution path and has a retain count of +1}}
+  // expected-warning@-1{{Potential leak of an object stored into 'arr'}}
 }
 
 unsigned int check_leak_factory() {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55465: Stop tracking retain count of OSObject after escape to void * / other primitive types

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348675: Stop tracking retain count of OSObject after escape 
to void * / other primitive… (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55465?vs=177361=177362#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55465

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  test/Analysis/osobject-retain-release.cpp


Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -574,6 +574,25 @@
   return State;
 }
 
+static bool isPointerToObject(QualType QT) {
+  QualType PT = QT->getPointeeType();
+  if (!PT.isNull())
+if (PT->getAsCXXRecordDecl())
+  return true;
+  return false;
+}
+
+/// Whether the tracked value should be escaped on a given call.
+/// OSObjects are escaped when passed to void * / etc.
+static bool shouldEscapeArgumentOnCall(const CallEvent , unsigned ArgIdx,
+   const RefVal *TrackedValue) {
+  if (TrackedValue->getObjKind() != RetEffect::OS)
+return false;
+  if (ArgIdx >= CE.parameters().size())
+return false;
+  return !isPointerToObject(CE.parameters()[ArgIdx]->getType());
+}
+
 void RetainCountChecker::checkSummary(const RetainSummary ,
   const CallEvent ,
   CheckerContext ) const {
@@ -592,6 +611,10 @@
   state = updateOutParameter(state, V, Effect);
 } else if (SymbolRef Sym = V.getAsLocSymbol()) {
   if (const RefVal *T = getRefBinding(state, Sym)) {
+
+if (shouldEscapeArgumentOnCall(CallOrMsg, idx, T))
+  Effect = StopTrackingHard;
+
 state = updateSymbol(state, Sym, *T, Effect, hasErr, C);
 if (hasErr) {
   ErrorRange = CallOrMsg.getArgSourceRange(idx);
Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -89,6 +89,13 @@
   static OSObject *safeMetaCast(const OSObject *inst, const OSMetaClass *meta);
 };
 
+void escape(void *);
+
+void test_escaping_into_voidstar() {
+  OSObject *obj = new OSObject;
+  escape(obj);
+}
+
 void test_no_infinite_check_recursion(MyArray *arr) {
   OSObject *input = new OSObject;
   OSObject *o = arr->generateObject(input);


Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -574,6 +574,25 @@
   return State;
 }
 
+static bool isPointerToObject(QualType QT) {
+  QualType PT = QT->getPointeeType();
+  if (!PT.isNull())
+if (PT->getAsCXXRecordDecl())
+  return true;
+  return false;
+}
+
+/// Whether the tracked value should be escaped on a given call.
+/// OSObjects are escaped when passed to void * / etc.
+static bool shouldEscapeArgumentOnCall(const CallEvent , unsigned ArgIdx,
+   const RefVal *TrackedValue) {
+  if (TrackedValue->getObjKind() != RetEffect::OS)
+return false;
+  if (ArgIdx >= CE.parameters().size())
+return false;
+  return !isPointerToObject(CE.parameters()[ArgIdx]->getType());
+}
+
 void RetainCountChecker::checkSummary(const RetainSummary ,
   const CallEvent ,
   CheckerContext ) const {
@@ -592,6 +611,10 @@
   state = updateOutParameter(state, V, Effect);
 } else if (SymbolRef Sym = V.getAsLocSymbol()) {
   if (const RefVal *T = getRefBinding(state, Sym)) {
+
+if (shouldEscapeArgumentOnCall(CallOrMsg, idx, T))
+  Effect = StopTrackingHard;
+
 state = updateSymbol(state, Sym, *T, Effect, hasErr, C);
 if (hasErr) {
   ErrorRange = CallOrMsg.getArgSourceRange(idx);
Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -89,6 +89,13 @@
   static OSObject *safeMetaCast(const OSObject *inst, const OSMetaClass *meta);
 };
 
+void escape(void *);
+
+void test_escaping_into_voidstar() {
+  OSObject *obj = new OSObject;
+  escape(obj);
+}
+
 void test_no_infinite_check_recursion(MyArray *arr) {
   OSObject *input = new OSObject;
   OSObject *o = arr->generateObject(input);
___

[PATCH] D55385: [analyzer] RetainCountChecker: remove untested, unused, incorrect option IncludeAllocationLine

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348637: [analyzer] RetainCountChecker: remove untested, 
unused, incorrect option… (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55385?vs=177055=177275#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55385

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h


Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
@@ -634,8 +634,7 @@
   UniqueingDecl = AllocNode->getLocationContext()->getDecl();
 }
 
-void CFRefLeakReport::createDescription(CheckerContext ,
-bool IncludeAllocationLine) {
+void CFRefLeakReport::createDescription(CheckerContext ) {
   assert(Location.isValid() && UniqueingDecl && UniqueingLocation.isValid());
   Description.clear();
   llvm::raw_string_ostream os(Description);
@@ -644,10 +643,6 @@
   Optional RegionDescription = describeRegion(AllocBinding);
   if (RegionDescription) {
 os << " stored into '" << *RegionDescription << '\'';
-if (IncludeAllocationLine) {
-  FullSourceLoc SL(AllocStmt->getBeginLoc(), Ctx.getSourceManager());
-  os << " (allocated on line " << SL.getSpellingLineNumber() << ")";
-}
   } else {
 
 // If we can't figure out the name, just supply the type information.
@@ -658,15 +653,14 @@
 CFRefLeakReport::CFRefLeakReport(CFRefBug , const LangOptions ,
  const SummaryLogTy ,
  ExplodedNode *n, SymbolRef sym,
- CheckerContext ,
- bool IncludeAllocationLine)
+ CheckerContext )
   : CFRefReport(D, LOpts, Log, n, sym, false) {
 
   deriveAllocLocation(Ctx, sym);
   if (!AllocBinding)
 deriveParamLocation(Ctx, sym);
 
-  createDescription(Ctx, IncludeAllocationLine);
+  createDescription(Ctx);
 
   addVisitor(llvm::make_unique(sym, Log));
 }
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -1059,8 +1059,7 @@
 if (N) {
   const LangOptions  = C.getASTContext().getLangOpts();
   auto R = llvm::make_unique(
-  *getLeakAtReturnBug(LOpts), LOpts, SummaryLog, N, Sym, C,
-  IncludeAllocationLine);
+  *getLeakAtReturnBug(LOpts), LOpts, SummaryLog, N, Sym, C);
   C.emitReport(std::move(R));
 }
 return N;
@@ -1346,7 +1345,7 @@
   assert(BT && "BugType not initialized.");
 
   Ctx.emitReport(llvm::make_unique(
-  *BT, LOpts, SummaryLog, N, *I, Ctx, IncludeAllocationLine));
+  *BT, LOpts, SummaryLog, N, *I, Ctx));
 }
   }
 
@@ -1508,8 +1507,6 @@
 
   AnalyzerOptions  = Mgr.getAnalyzerOptions();
 
-  Chk->IncludeAllocationLine = Options.getCheckerBooleanOption(
-   "leak-diagnostics-reference-allocation", false, 
Chk);
   Chk->ShouldCheckOSObjectRetainCount = Options.getCheckerBooleanOption(
 "CheckOSObject", true, 
Chk);
 }
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
@@ -71,13 +71,12 @@
   // Finds the location where a leak warning for 'sym' should be raised.
   void deriveAllocLocation(CheckerContext , SymbolRef sym);
   // Produces description of a leak warning which is printed on the console.
-  void createDescription(CheckerContext , bool IncludeAllocationLine);
+  void createDescription(CheckerContext );
 
 public:
   CFRefLeakReport(CFRefBug , const LangOptions ,
   const SummaryLogTy , ExplodedNode *n, SymbolRef sym,
-  CheckerContext ,
-  bool IncludeAllocationLine);
+  CheckerContext );
 
   PathDiagnosticLocation getLocation(const SourceManager ) const override {
 assert(Location.isValid());


Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

[PATCH] D55400: [analyzer] Move out tracking retain count for OSObjects into a separate checker

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348638: [analyzer] Move out tracking retain count for 
OSObjects into a separate checker (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55400?vs=177271=177276#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55400

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
  lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
  test/Analysis/osobject-retain-release.cpp
  test/Analysis/test-separate-retaincount.cpp

Index: lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
===
--- lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
+++ lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
@@ -24,6 +24,31 @@
 using namespace clang;
 using namespace ento;
 
+template 
+constexpr static bool isOneOf() {
+  return false;
+}
+
+/// Helper function to check whether the class is one of the
+/// rest of varargs.
+template 
+constexpr static bool isOneOf() {
+  return std::is_same::value || isOneOf();
+}
+
+template  bool RetainSummaryManager::isAttrEnabled() {
+  if (isOneOf()) {
+return TrackObjCAndCFObjects;
+  } else if (isOneOf()) {
+return TrackOSObjects;
+  }
+  llvm_unreachable("Unexpected attribute passed");
+}
+
 ArgEffects RetainSummaryManager::getArgEffects() {
   ArgEffects AE = ScratchArgs;
   ScratchArgs = AF.getEmptyMap();
@@ -116,30 +141,60 @@
 }
 
 const RetainSummary *
-RetainSummaryManager::generateSummary(const FunctionDecl *FD,
-  bool ) {
-  // We generate "stop" summaries for implicitly defined functions.
-  if (FD->isImplicit()) {
-return getPersistentStopSummary();
+RetainSummaryManager::getSummaryForOSObject(const FunctionDecl *FD,
+StringRef FName, QualType RetTy) {
+  if (RetTy->isPointerType()) {
+const CXXRecordDecl *PD = RetTy->getPointeeType()->getAsCXXRecordDecl();
+if (PD && isOSObjectSubclass(PD)) {
+  if (const IdentifierInfo *II = FD->getIdentifier()) {
+if (isOSObjectDynamicCast(II->getName()))
+  return getDefaultSummary();
+
+// All objects returned with functions *not* starting with
+// get, or iterators, are returned at +1.
+if ((!II->getName().startswith("get") &&
+ !II->getName().startswith("Get")) ||
+isOSIteratorSubclass(PD)) {
+  return getOSSummaryCreateRule(FD);
+} else {
+  return getOSSummaryGetRule(FD);
+}
+  }
+}
   }
 
-  const IdentifierInfo *II = FD->getIdentifier();
+  if (const auto *MD = dyn_cast(FD)) {
+const CXXRecordDecl *Parent = MD->getParent();
+if (TrackOSObjects && Parent && isOSObjectSubclass(Parent)) {
+  if (FName == "release")
+return getOSSummaryReleaseRule(FD);
 
-  StringRef FName = II ? II->getName() : "";
+  if (FName == "retain")
+return getOSSummaryRetainRule(FD);
 
-  // Strip away preceding '_'.  Doing this here will effect all the checks
-  // down below.
-  FName = FName.substr(FName.find_first_not_of('_'));
+  if (FName == "free")
+return getOSSummaryFreeRule(FD);
+
+  if (MD->getOverloadedOperator() == OO_New)
+return getOSSummaryCreateRule(MD);
+}
+  }
+
+  return nullptr;
+}
+
+const RetainSummary *RetainSummaryManager::getSummaryForObjCOrCFObject(
+const FunctionDecl *FD,
+StringRef FName,
+QualType RetTy,
+const FunctionType *FT,
+bool ) {
 
-  // Inspect the result type. Strip away any typedefs.
-  const auto *FT = FD->getType()->getAs();
-  QualType RetTy = FT->getReturnType();
   std::string RetTyName = RetTy.getAsString();
 
   // FIXME: This should all be refactored into a chain of "summary lookup"
   //  filters.
   assert(ScratchArgs.isEmpty());
-
   if (FName == "pthread_create" || FName == "pthread_setspecific") {
 // Part of:  and .
 // This will be addressed better with IPA.
@@ -230,30 +285,11 @@
 
   if (RetTy->isPointerType()) {
 
-const CXXRecordDecl *PD = RetTy->getPointeeType()->getAsCXXRecordDecl();
-if (TrackOSObjects && PD && isOSObjectSubclass(PD)) {
-  if (const IdentifierInfo *II = FD->getIdentifier()) {
-
-if (isOSObjectDynamicCast(II->getName()))
-  return getDefaultSummary();
-
-// All objects returned with functions *not* starting with
-// get, or iterators, are returned at +1.
-if ((!II->getName().startswith("get") &&
- !II->getName().startswith("Get")) ||
-isOSIteratorSubclass(PD)) {
-  return getOSSummaryCreateRule(FD);
- 

[PATCH] D55158: [analyzer] Rely on os_consumes_this attribute to signify that the method call consumes a reference for "this"

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348533: [analyzer] Rely on os_consumes_this attribute to 
signify that the method call… (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55158?vs=176234=177044#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55158

Files:
  include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
  lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
  test/Analysis/osobject-retain-release.cpp


Index: include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
===
--- include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
+++ include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
@@ -369,8 +369,12 @@
   ///  This is only meaningful if the summary applies to an ObjCMessageExpr*.
   ArgEffect getReceiverEffect() const { return Receiver; }
 
+  /// \return the effect on the "this" receiver of the method call.
   ArgEffect getThisEffect() const { return This; }
 
+  /// Set the effect of the method on "this".
+  void setThisEffect(ArgEffect e) { This = e; }
+
   bool isNoop() const {
 return Ret == RetEffect::MakeNoRet() && Receiver == DoNothing
   && DefaultArgEffect == MayEscape && This == DoNothing
Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -5,6 +5,7 @@
 #define OS_CONSUME __attribute__((os_consumed))
 #define OS_RETURNS_RETAINED __attribute__((os_returns_retained))
 #define OS_RETURNS_NOT_RETAINED __attribute__((os_returns_not_retained))
+#define OS_CONSUMES_THIS __attribute__((os_consumes_this))
 
 #define OSTypeID(type)   (type::metaClass)
 
@@ -49,6 +50,11 @@
 
   virtual void consumeReference(OS_CONSUME OSArray *other);
 
+  void putIntoArray(OSArray *array) OS_CONSUMES_THIS;
+
+  template 
+  void putIntoT(T *owner) OS_CONSUMES_THIS;
+
   static OSArray *generateArrayHasCode() {
 return new OSArray;
   }
@@ -112,6 +118,16 @@
   return 0;
 }
 
+void check_consumes_this(OSArray *owner) {
+  OSArray *arr = new OSArray;
+  arr->putIntoArray(owner);
+}
+
+void check_consumes_this_with_template(OSArray *owner) {
+  OSArray *arr = new OSArray;
+  arr->putIntoT(owner);
+}
+
 void check_free_no_error() {
   OSArray *arr = OSArray::withCapacity(10);
   arr->retain();
Index: lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
===
--- lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
+++ lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
@@ -759,6 +759,9 @@
   QualType RetTy = FD->getReturnType();
   if (Optional RetE = getRetEffectFromAnnotations(RetTy, FD))
 Template->setRetEffect(*RetE);
+
+  if (FD->hasAttr())
+Template->setThisEffect(DecRef);
 }
 
 void


Index: include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
===
--- include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
+++ include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
@@ -369,8 +369,12 @@
   ///  This is only meaningful if the summary applies to an ObjCMessageExpr*.
   ArgEffect getReceiverEffect() const { return Receiver; }
 
+  /// \return the effect on the "this" receiver of the method call.
   ArgEffect getThisEffect() const { return This; }
 
+  /// Set the effect of the method on "this".
+  void setThisEffect(ArgEffect e) { This = e; }
+
   bool isNoop() const {
 return Ret == RetEffect::MakeNoRet() && Receiver == DoNothing
   && DefaultArgEffect == MayEscape && This == DoNothing
Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -5,6 +5,7 @@
 #define OS_CONSUME __attribute__((os_consumed))
 #define OS_RETURNS_RETAINED __attribute__((os_returns_retained))
 #define OS_RETURNS_NOT_RETAINED __attribute__((os_returns_not_retained))
+#define OS_CONSUMES_THIS __attribute__((os_consumes_this))
 
 #define OSTypeID(type)   (type::metaClass)
 
@@ -49,6 +50,11 @@
 
   virtual void consumeReference(OS_CONSUME OSArray *other);
 
+  void putIntoArray(OSArray *array) OS_CONSUMES_THIS;
+
+  template 
+  void putIntoT(T *owner) OS_CONSUMES_THIS;
+
   static OSArray *generateArrayHasCode() {
 return new OSArray;
   }
@@ -112,6 +118,16 @@
   return 0;
 }
 
+void check_consumes_this(OSArray *owner) {
+  OSArray *arr = new OSArray;
+  arr->putIntoArray(owner);
+}
+
+void check_consumes_this_with_template(OSArray *owner) {
+  OSArray *arr = new OSArray;
+  arr->putIntoT(owner);
+}
+
 void check_free_no_error() {
   OSArray *arr = OSArray::withCapacity(10);
   arr->retain();

[PATCH] D55155: [attributes] Add an attribute "os_consumes_this" with similar semantics to "ns_consumes_self"

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348532: [attributes] Add an attribute os_consumes_this, with 
similar semantics to… (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55155?vs=177041=177043#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55155

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/attr-osobject.cpp

Index: include/clang/Basic/AttrDocs.td
===
--- include/clang/Basic/AttrDocs.td
+++ include/clang/Basic/AttrDocs.td
@@ -862,6 +862,9 @@
 Similiarly, the annotation ``__attribute__((ns_returns_not_retained))``
 specifies that the object is returned at ``+0`` and the ownership remains with
 the callee.
+The annotation ``__attribute__((ns_consumes_self))`` specifies that
+the Objective-C method call consumes the reference to ``self``, e.g. by
+attaching it to a supplied parameter.
 Additionally, parameters can have an annotation
 ``__attribute__((ns_consumed))``, which specifies that passing an owned object
 as that parameter effectively transfers the ownership, and the caller is no
@@ -881,7 +884,11 @@
 ``__attribute__((os_returns_not_retained))``,
 ``__attribute__((os_returns_retained))`` and ``__attribute__((os_consumed))``,
 with the same respective semantics.
-These attributes are also used by the Clang Static Analyzer.
+Similar to ``__attribute__((ns_consumes_self))``,
+``__attribute__((os_consumes_this))`` specifies that the method call consumes
+the reference to "this" (e.g., when attaching it to a different object supplied
+as a parameter).
+These attributes are only used by the Clang Static Analyzer.
 
 The family of attributes ``X_returns_X_retained`` can be added to functions,
 C++ methods, and Objective-C methods and properties.
Index: include/clang/Basic/Attr.td
===
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -90,6 +90,10 @@
 [{!S->isBitField()}],
 "non-bit-field non-static data members">;
 
+def NonStaticCXXMethod : SubsetSubjectisStatic()}],
+   "non-static member functions">;
+
 def NonStaticNonConstCXXMethod
 : SubsetSubjectisStatic() && !S->isConst()}],
@@ -846,6 +850,12 @@
   let Documentation = [RetainBehaviorDocs];
 }
 
+def OSConsumesThis : InheritableAttr {
+  let Spellings = [Clang<"os_consumes_this">];
+  let Subjects = SubjectList<[NonStaticCXXMethod]>;
+  let Documentation = [RetainBehaviorDocs];
+}
+
 def Cleanup : InheritableAttr {
   let Spellings = [GCC<"cleanup">];
   let Args = [FunctionArgument<"FunctionDecl">];
@@ -1649,13 +1659,13 @@
 def NSConsumesSelf : InheritableAttr {
   let Spellings = [Clang<"ns_consumes_self">];
   let Subjects = SubjectList<[ObjCMethod]>;
-  let Documentation = [Undocumented];
+  let Documentation = [RetainBehaviorDocs];
 }
 
 def NSConsumed : InheritableParamAttr {
   let Spellings = [Clang<"ns_consumed">];
   let Subjects = SubjectList<[ParmVar]>;
-  let Documentation = [Undocumented];
+  let Documentation = [RetainBehaviorDocs];
 }
 
 def ObjCException : InheritableAttr {
Index: test/Sema/attr-osobject.cpp
===
--- test/Sema/attr-osobject.cpp
+++ test/Sema/attr-osobject.cpp
@@ -4,6 +4,10 @@
   __attribute__((os_returns_retained)) S* method_returns_retained() {
 return nullptr;
   }
+
+  __attribute__((os_consumes_this)) void method_consumes_this();
+
+  __attribute__((os_consumes_this)) static void rejected_on_static(); // expected-warning{{'os_consumes_this' attribute only applies to non-static member functions}}
 };
 __attribute__((os_returns_retained)) S *ret_retained() {
   return nullptr;
@@ -37,6 +41,8 @@
 
 __attribute__((os_returns_not_retained(10))) S* os_returns_no_retained_no_extra_args( S *arg) { // expected-error{{'os_returns_not_retained' attribute takes no arguments}}
   return nullptr;
-} 
+}
 
 struct __attribute__((os_returns_not_retained)) NoNotRetainedAttrOnStruct {}; // expected-warning{{'os_returns_not_retained' attribute only applies to functions, Objective-C methods, and Objective-C properties}}
+
+__attribute__((os_consumes_this)) void no_consumes_this_on_function() {} // expected-warning{{'os_consumes_this' attribute only applies to non-static member functions}}
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -6408,6 +6408,9 @@
   case ParsedAttr::AT_NSConsumesSelf:
 handleSimpleAttribute(S, D, AL);
 break;
+  case ParsedAttr::AT_OSConsumesThis:
+handleSimpleAttribute(S, D, AL);
+break;

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

> Well,, that seems unfortunate if we have the only supported interface for the 
> static analyzer be an internal interface. Perhaps it can be given a different 
> option? Even discounting this change, I that seems like it would be 
> appropriate.

Officially there is, it's called `-Xanalyzer`, but in practice it does the same 
as `-Xclang`, and users use the two interchangeably.


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

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added reviewers: dcoughlin, NoQ.
george.karpenkov requested changes to this revision.
george.karpenkov added a comment.

Using `-Xclang` is the only way to pass options to the static analyzer, I don't 
think we should warn on it.


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

https://reviews.llvm.org/D55150



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

[PATCH] D55321: Do not check for parameters shadowing fields in function declarations

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov accepted this revision.
george.karpenkov added a comment.
This revision is now accepted and ready to land.

Thanks!


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

https://reviews.llvm.org/D55321



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

[PATCH] D55226: [Fix][StaticAnalyzer] Bug 39792 - False positive on strcpy targeting struct member

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov accepted this revision.
george.karpenkov added a comment.
This revision is now accepted and ready to land.

Thank you for the fix, but how far can the pattern matching go? Seems easy 
enough to think of cases not covered by the above.
In any case, the fix looks good.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55226



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

[PATCH] D55076: [analyzer] RetainCountChecker: recognize that OSObject can be created directly using an operator "new"

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov marked an inline comment as done.
george.karpenkov added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp:483-497
   case CE_Function:
 Summ = getFunctionSummary(cast(Call).getDecl());
 break;
   case CE_CXXMember:
 Summ = getFunctionSummary(cast(Call).getDecl());
 break;
   case CE_CXXMemberOperator:

NoQ wrote:
> It's actually just `Call.getDecl()` and you can turn this into a fall-through.
Call.getDecl() returns a Decl (gotta love Obj-C methods!).

I guess we can group all those cases, and cast the returned decl to 
FunctionDecl instead of casting the call.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55076



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

[PATCH] D55076: [analyzer] RetainCountChecker: recognize that OSObject can be created directly using an operator "new"

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC347949: [analyzer] RetainCountChecker: recognize that 
OSObject can be created directly… (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55076?vs=175949=176021#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55076

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
  test/Analysis/osobject-retain-release.cpp


Index: lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
===
--- lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
+++ lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
@@ -124,10 +124,8 @@
   }
 
   const IdentifierInfo *II = FD->getIdentifier();
-  if (!II)
-return getDefaultSummary();
 
-  StringRef FName = II->getName();
+  StringRef FName = II ? II->getName() : "";
 
   // Strip away preceding '_'.  Doing this here will effect all the checks
   // down below.
@@ -304,6 +302,9 @@
 
   if (FName == "retain")
 return getOSSummaryRetainRule(FD);
+
+  if (MD->getOverloadedOperator() == OO_New)
+return getOSSummaryCreateRule(MD);
 }
   }
 
@@ -491,9 +492,11 @@
   case CE_CXXConstructor:
 Summ = getFunctionSummary(cast(Call).getDecl());
 break;
+  case CE_CXXAllocator:
+Summ = getFunctionSummary(cast(Call).getDecl());
+break;
   case CE_Block:
   case CE_CXXDestructor:
-  case CE_CXXAllocator:
 // FIXME: These calls are currently unsupported.
 return getPersistentStopSummary();
   case CE_ObjCMessage: {
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
@@ -137,6 +137,8 @@
 } else {
   os << "function call";
 }
+  } else if (const auto *NE = dyn_cast(S)){
+os << "Operator new";
   } else {
 assert(isa(S));
 CallEventManager  = CurrSt->getStateManager().getCallEventManager();
Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -23,6 +23,9 @@
   static OSObject *getObject();
   static OSObject *GetObject();
 
+
+  static void * operator new(unsigned long size);
+
   static const OSMetaClass * const metaClass;
 };
 
@@ -62,6 +65,18 @@
   static OSObject *safeMetaCast(const OSObject *inst, const OSMetaClass *meta);
 };
 
+unsigned int check_leak_explicit_new() {
+  OSArray *arr = new OSArray; // expected-note{{Operator new returns an 
OSObject of type struct OSArray * with a +1 retain count}}
+  return arr->getCount(); // expected-note{{Object leaked: allocated object of 
type struct OSArray * is not referenced later in this execution path and has a 
retain count of +1}}
+  // expected-warning@-1{{Potential leak of an object 
of type struct OSArray *}}
+}
+
+unsigned int check_leak_factory() {
+  OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to function 
'OSArray::withCapacity' returns an OSObject of type struct OSArray * with a +1 
retain count}}
+  return arr->getCount(); // expected-note{{Object leaked: object allocated 
and stored into 'arr' is not referenced later in this execution path and has a 
retain count of +1}}
+  // expected-warning@-1{{Potential leak of an object 
stored into 'arr'}}
+}
+
 void check_get_object() {
   OSObject::getObject();
 }


Index: lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
===
--- lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
+++ lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
@@ -124,10 +124,8 @@
   }
 
   const IdentifierInfo *II = FD->getIdentifier();
-  if (!II)
-return getDefaultSummary();
 
-  StringRef FName = II->getName();
+  StringRef FName = II ? II->getName() : "";
 
   // Strip away preceding '_'.  Doing this here will effect all the checks
   // down below.
@@ -304,6 +302,9 @@
 
   if (FName == "retain")
 return getOSSummaryRetainRule(FD);
+
+  if (MD->getOverloadedOperator() == OO_New)
+return getOSSummaryCreateRule(MD);
 }
   }
 
@@ -491,9 +492,11 @@
   case CE_CXXConstructor:
 Summ = getFunctionSummary(cast(Call).getDecl());
 break;
+  case CE_CXXAllocator:
+Summ = getFunctionSummary(cast(Call).getDecl());
+break;
   case CE_Block:
   case CE_CXXDestructor:
-  case CE_CXXAllocator:
 // FIXME: These calls are currently unsupported.
 return getPersistentStopSummary();
   case 

[PATCH] D55041: [analyzer] Switch retain count checker for OSObject to use OS_* attributes

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC347948: [analyzer] Switch retain count checker for OSObject 
to use OS_* attributes (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55041?vs=175798=176020#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55041

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
  test/Analysis/osobject-retain-release.cpp


Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -2,9 +2,9 @@
 
 struct OSMetaClass;
 
-#define OS_CONSUME __attribute__((annotate("rc_ownership_consumed")))
-#define OS_RETURNS_RETAINED 
__attribute__((annotate("rc_ownership_returns_retained")))
-#define OS_RETURNS_NOT_RETAINED 
__attribute__((annotate("rc_ownership_returns_not_retained")))
+#define OS_CONSUME __attribute__((os_consumed))
+#define OS_RETURNS_RETAINED __attribute__((os_returns_retained))
+#define OS_RETURNS_NOT_RETAINED __attribute__((os_returns_not_retained))
 
 #define OSTypeID(type)   (type::metaClass)
 
Index: lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
===
--- lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
+++ lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
@@ -664,15 +664,21 @@
 return None;
   }
 
-  if (D->hasAttr())
+  if (D->hasAttr()) {
 return RetEffect::MakeOwned(RetEffect::CF);
-  else if (hasRCAnnotation(D, "rc_ownership_returns_retained"))
+  } else if (D->hasAttr()) {
+return RetEffect::MakeOwned(RetEffect::OS);
+  } else if (hasRCAnnotation(D, "rc_ownership_returns_retained")) {
 return RetEffect::MakeOwned(RetEffect::Generalized);
+  }
 
-  if (D->hasAttr())
+  if (D->hasAttr()) {
 return RetEffect::MakeNotOwned(RetEffect::CF);
-  else if (hasRCAnnotation(D, "rc_ownership_returns_not_retained"))
+  } else if (D->hasAttr()) {
+return RetEffect::MakeNotOwned(RetEffect::OS);
+  } else if (hasRCAnnotation(D, "rc_ownership_returns_not_retained")) {
 return RetEffect::MakeNotOwned(RetEffect::Generalized);
+  }
 
   return None;
 }
@@ -688,15 +694,16 @@
 
   // Effects on the parameters.
   unsigned parm_idx = 0;
-  for (FunctionDecl::param_const_iterator pi = FD->param_begin(),
+  for (auto pi = FD->param_begin(),
  pe = FD->param_end(); pi != pe; ++pi, ++parm_idx) {
 const ParmVarDecl *pd = *pi;
-if (pd->hasAttr())
+if (pd->hasAttr()) {
   Template->addArg(AF, parm_idx, DecRefMsg);
-else if (pd->hasAttr() ||
- hasRCAnnotation(pd, "rc_ownership_consumed"))
+} else if (pd->hasAttr() ||
+ pd->hasAttr() ||
+ hasRCAnnotation(pd, "rc_ownership_consumed")) {
   Template->addArg(AF, parm_idx, DecRef);
-else if (pd->hasAttr() ||
+} else if (pd->hasAttr() ||
  hasRCAnnotation(pd, "rc_ownership_returns_retained")) {
   QualType PointeeTy = pd->getType()->getPointeeType();
   if (!PointeeTy.isNull())
@@ -734,9 +741,9 @@
  pi=MD->param_begin(), pe=MD->param_end();
pi != pe; ++pi, ++parm_idx) {
 const ParmVarDecl *pd = *pi;
-if (pd->hasAttr())
+if (pd->hasAttr()) {
   Template->addArg(AF, parm_idx, DecRefMsg);
-else if (pd->hasAttr()) {
+} else if (pd->hasAttr() || pd->hasAttr()) 
{
   Template->addArg(AF, parm_idx, DecRef);
 } else if (pd->hasAttr()) {
   QualType PointeeTy = pd->getType()->getPointeeType();
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
@@ -526,7 +526,8 @@
   os << "that is annotated as CF_RETURNS_NOT_RETAINED";
 } else if (D->hasAttr()) {
   os << "that is annotated as NS_RETURNS_NOT_RETAINED";
-  // TODO: once the patch is ready, insert a case for 
OS_RETURNS_NOT_RETAINED
+} else if (D->hasAttr()) {
+  os << "that is annotated as OS_RETURNS_NOT_RETAINED";
 } else {
   if (const ObjCMethodDecl *MD = dyn_cast(D)) {
 if (BRC.getASTContext().getLangOpts().ObjCAutoRefCount) {


Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -2,9 +2,9 @@
 
 struct OSMetaClass;
 
-#define OS_CONSUME __attribute__((annotate("rc_ownership_consumed")))
-#define OS_RETURNS_RETAINED __attribute__((annotate("rc_ownership_returns_retained")))
-#define 

[PATCH] D55033: [analyzer] Add the type of the leaked object to the diagnostic message

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC347943: [analyzer] Add the type of the leaked object to the 
diagnostic message (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55033?vs=176005=176012#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55033

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
  test/Analysis/Inputs/expected-plists/objc-arc.m.plist
  test/Analysis/Inputs/expected-plists/retain-release-path-notes.m.plist
  test/Analysis/objc-radar17039661.m
  test/Analysis/osobject-retain-release.cpp
  test/Analysis/retain-release-path-notes.m
  test/Analysis/retaincountchecker-compoundregion.m

Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
@@ -141,12 +141,14 @@
 };
 
 class CFRefReport : public BugReport {
+protected:
+  SymbolRef Sym;
 
 public:
   CFRefReport(CFRefBug , const LangOptions ,
   const SummaryLogTy , ExplodedNode *n, SymbolRef sym,
   bool registerVisitor = true)
-: BugReport(D, D.getDescription(), n) {
+: BugReport(D, D.getDescription(), n), Sym(sym) {
 if (registerVisitor)
   addVisitor(llvm::make_unique(sym, Log));
   }
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
@@ -28,6 +28,17 @@
  isa(E);
 }
 
+/// If type represents a pointer to CXXRecordDecl,
+/// and is not a typedef, return the decl name.
+/// Otherwise, return the serialization of type.
+static StringRef getPrettyTypeName(QualType QT) {
+  QualType PT = QT->getPointeeType();
+  if (!PT.isNull() && !QT->getAs())
+if (const auto *RD = PT->getAsCXXRecordDecl())
+  return RD->getName();
+  return QT.getAsString();
+}
+
 /// Write information about the type state change to {@code os},
 /// return whether the note should be generated.
 static bool shouldGenerateNote(llvm::raw_string_ostream ,
@@ -193,7 +204,7 @@
<< Sym->getType().getAsString() << " with a ";
   } else if (CurrV.getObjKind() == RetEffect::OS) {
 os << " returns an OSObject of type "
-   << Sym->getType().getAsString() << " with a ";
+   << getPrettyTypeName(Sym->getType()) << " with a ";
   } else if (CurrV.getObjKind() == RetEffect::Generalized) {
 os << " returns an object of type " << Sym->getType().getAsString()
<< " with a ";
@@ -432,7 +443,7 @@
   if (RegionDescription) {
 os << "object allocated and stored into '" << *RegionDescription << '\'';
   } else {
-os << "allocated object";
+os << "allocated object of type " << getPrettyTypeName(Sym->getType());
   }
 
   // Get the retain count.
@@ -472,10 +483,10 @@
   " Foundation";
   }
 }
-  }
-  else
+  } else {
 os << " is not referenced later in this execution path and has a retain "
   "count of +" << RV->getCount();
+  }
 
   return std::make_shared(L, os.str());
 }
@@ -555,6 +566,10 @@
   FullSourceLoc SL(AllocStmt->getBeginLoc(), Ctx.getSourceManager());
   os << " (allocated on line " << SL.getSpellingLineNumber() << ")";
 }
+  } else {
+
+// If we can't figure out the name, just supply the type information.
+os << " of type " << getPrettyTypeName(Sym->getType());
   }
 }
 
Index: test/Analysis/retain-release-path-notes.m
===
--- test/Analysis/retain-release-path-notes.m
+++ test/Analysis/retain-release-path-notes.m
@@ -227,7 +227,7 @@
 // expected-note@-1 {{Method returns an instance of MyObj with a +1 retain count}}
 // expected-note@-2 {{Calling 'initX'}}
 // expected-note@-3 {{Returning from 'initX'}}
-// expected-note@-4 {{Object leaked: allocated object is not referenced later in this execution path and has a retain count of +1}}
+// expected-note@-4 {{Object leaked: allocated object of type MyObj * is not referenced later in this execution path and has a retain count of +1}}
   // initI is inlined because the allocation happens within initY
   id y = [[MyObj alloc] initY];
 // expected-note@-1 {{Calling 'initY'}}
Index: 

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov requested changes to this revision.
george.karpenkov added inline comments.
This revision now requires changes to proceed.



Comment at: test/Analysis/uninit-vals.m:222
   testObj->origin = makeIntPoint(1, 2);
-  if (testObj->size > 0) { ; } // expected-note{{Taking false branch}}
+  if (testObj->size > 0) { ; } // expected-note{{Assuming 'testObj->size' is 
<= 0}}
// expected-note@-1{{Taking false branch}}

Same here: we should know that `testObj->size == 0`



Comment at: test/Analysis/uninit-vals.m:324
   testObj->origin = makeIntPoint2D(1, 2);
-  if (testObj->size > 0) { ; } // expected-note{{Taking false branch}}
+  if (testObj->size > 0) { ; } // expected-note{{Assuming 'testObj->size' is 
<= 0}}
// expected-note@-1{{Taking false branch}}

That does not seem right: from `calloc` the analyzer should know that the 
`testObj->size` is actually zero.



Comment at: test/Analysis/virtualcall.cpp:170
+   // expected-note-re@-4 ^}}Assuming 'i' is <= 0}}
+   // expected-note-re@-5 ^}}Taking false branch}}
 #endif

Could you describe what happens here?
Why the `assuming` notes weren't there before?


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

https://reviews.llvm.org/D53076



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

This looks more reasonable, thanks!
What about just dropping the `Knowing` prefix?
Just having `arr is null, taking true branch` seems considerably more readable.


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

https://reviews.llvm.org/D53076



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

[PATCH] D54457: [AST] Generate unique identifiers for CXXCtorInitializer objects.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov accepted this revision.
george.karpenkov added inline comments.
This revision is now accepted and ready to land.



Comment at: lib/AST/DeclCXX.cpp:2249
 
+int64_t CXXCtorInitializer::getID(const ASTContext ) const {
+  Optional Out = Context.getAllocator().identifyObject(this);

Should we factor out this code? It's a fourth duplication now, probably a 
generic form should be in the allocator.


Repository:
  rC Clang

https://reviews.llvm.org/D54457



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

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

In the future, for macOS-specific changes I think it would be better to wait 
for a sign-off from at least one maintainer who is an expert on Apple tools.


Repository:
  rL LLVM

https://reviews.llvm.org/D54310



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

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

I suspect it broke 
http://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/14090/console as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D54310



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

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

I don't quite understand the need for this patch.
If we are talking about a binary built from source, wouldn't it make more sense 
to build libcxx from source as well?
If libcxx is built from source in the same repo, clang-based tools do find it.


Repository:
  rL LLVM

https://reviews.llvm.org/D54310



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

[PATCH] D54429: [analyzer] Creating standard Sphinx documentation

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

From a first glance, it's missing screenshots and an introduction section.

What do you propose we should do with other pages on the existing website? 
(OpenProjects / etc.)


Repository:
  rC Clang

https://reviews.llvm.org/D54429



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

[PATCH] D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

> Also, remove diags::note_suggest_disabling_all_checkers.

Isn't that a separate revision? Given that removing existing options is often 
questionable, I'd much rather see this patch separated.


Repository:
  rC Clang

https://reviews.llvm.org/D54401



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

[PATCH] D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added inline comments.



Comment at: include/clang/Basic/DiagnosticFrontendKinds.td:177
 "no analyzer checkers are associated with '%0'">;
-def note_suggest_disabling_all_checkers : Note<
-"use -analyzer-disable-all-checks to disable all static analyzer 
checkers">;

@NoQ what does this option even do? Can you think of some legitimate uses?


Repository:
  rC Clang

https://reviews.llvm.org/D54401



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

[PATCH] D54132: [CodeGenCXX] XFAIL test for ASAN on Darwin.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov accepted this revision.
george.karpenkov added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/test/CodeGenCXX/castexpr-basepathsize-threshold.cpp:8
+// recursive template instantiation limit.
+// XFAIL: darwin && asan
+

vsapsai wrote:
> george.karpenkov wrote:
> > Do we actually want UNSUPPORTED here? We don't want to fail if ASAN stack 
> > usage decreases?
> If ASAN stack usage decreases or template instantiation stack usage 
> decreases, I'd like to know that and to remove XFAIL. My reason to prefer 
> XFAIL over UNSUPPORTED is that currently the test fails due to specific 
> implementation of Clang and ASAN, not due to conceptual incompatibility. But 
> I don't have any evidence to show that my suggestion is actually better, so 
> if my argument doesn't look convincing, most likely UNSUPPORTED would be 
> better.
Either one works for me.


https://reviews.llvm.org/D54132



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

[PATCH] D54132: [CodeGenCXX] XFAIL test for ASAN on Darwin.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added inline comments.



Comment at: clang/test/CodeGenCXX/castexpr-basepathsize-threshold.cpp:8
+// recursive template instantiation limit.
+// XFAIL: darwin && asan
+

Do we actually want UNSUPPORTED here? We don't want to fail if ASAN stack usage 
decreases?


https://reviews.llvm.org/D54132



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

[PATCH] D50050: [AST] CastExpr: BasePathSize is not large enough.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a subscriber: vsapsai.
george.karpenkov added a comment.

@lebedev.ri yeah ASAN is making stack frame size larger.

It seems @vsapsai is working on disabling this test under ASAN.


Repository:
  rC Clang

https://reviews.llvm.org/D50050



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

[PATCH] D50050: [AST] CastExpr: BasePathSize is not large enough.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

@lebedev.ri @erichkeane The test fails for me on macOS whenever asan and ubsan 
are both enabled.
The failure is stack overflow at stack frame 943 (? maybe asan usage enforces 
lower stack size?)


Repository:
  rC Clang

https://reviews.llvm.org/D50050



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

[PATCH] D54013: [analyzer] MallocChecker: Avoid redundant transitions.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2369
   ProgramStateRef state = C.getState();
-  RegionStateTy RS = state->get();
+  RegionStateTy OldRS = state->get();
   RegionStateTy::Factory  = state->get_context();

Szelethus wrote:
> We are acquiring an object of type `ImmutableMap`, modify it, and put it back 
> into `state`? Can't we just modify it through `ProgramState`'s interface 
> directly?
state is immutable, I don't think we can put anything into it.
We also can't modify ImmutableMap because, well, it's immutable.


Repository:
  rC Clang

https://reviews.llvm.org/D54013



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

[PATCH] D53277: [analyzer][NFC] Collect all -analyzer-config options in a .def file

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

@NoQ @Szelethus Actually pushed a fix.


Repository:
  rL LLVM

https://reviews.llvm.org/D53277



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

[PATCH] D53277: [analyzer][NFC] Collect all -analyzer-config options in a .def file

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

@Szelethus @NoQ OK it seems you need to annotate those headers in modulemap.

E.g. take a look at:

  george1@/Volumes/Transcend/code/monorepo/llvm-project/clang (master)≻ rg 
SVals.def
  include/clang/module.modulemap
  131:  textual header "StaticAnalyzer/Core/PathSensitive/SVals.def"
  
  include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
  58:#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"
  68:#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"
  82:#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"
  
  include/clang/StaticAnalyzer/Core/PathSensitive/SValVisitor.h
  39:#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"
  44:#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"
  51:#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"
  66:#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"
  
  include/clang/StaticAnalyzer/Core/PathSensitive/SVals.def
  1://===-- SVals.def - Metadata about SVal kinds ---*- C++ 
-*-===//


Repository:
  rL LLVM

https://reviews.llvm.org/D53277



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

[PATCH] D53277: [analyzer][NFC] Collect all -analyzer-config options in a .def file

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

@Szelethus you can see the command required to get modules build in the 
mentioned log. I think -DLLVM_ENABLE_MODULES=On might be enough.


Repository:
  rL LLVM

https://reviews.llvm.org/D53277



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

[PATCH] D53982: Output "rule" information in SARIF

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov accepted this revision.
george.karpenkov added inline comments.
This revision is now accepted and ready to land.



Comment at: 
test/Analysis/diagnostics/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif:10
   },
-  "length": 510,
+  "length": 413,
   "mimeType": "text/plain",

You have probably noticed that seeing those changes is much nicer in diff than 
in FileCheck :P


https://reviews.llvm.org/D53982



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

[PATCH] D53982: Output "rule" information in SARIF

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov requested changes to this revision.
george.karpenkov added a comment.
This revision now requires changes to proceed.

Minor nit: let's create a custom substitution for your diff command, like 
`diff_plist`.
Otherwise LGTM.




Comment at: test/Analysis/diagnostics/sarif-multi-diagnostic-test.c:1
+// RUN: %clang_analyze_cc1 
-analyzer-checker=core,alpha.security.taint,debug.TaintTest %s -verify 
-analyzer-output=sarif -o - | diff -U1 -w -I 
".*file:.*sarif-multi-diagnostic-test.c" -I '"version":' -I 
"2\.0\.0\-csd\.[0-9]*\.beta\." - 
%S/Inputs/expected-sarif/sarif-multi-diagnostic-test.c.sarif
+#include "../Inputs/system-header-simulator.h"

In order to avoid duplication in `diff` commands between the tests, it might be 
better to define a custom command in `lit.cfg`, as we did for `diff_plist`
(alternatively, create a custom flag to generate stable output)


https://reviews.llvm.org/D53982



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

[PATCH] D53902: RetainCountChecker: for now, do not trust the summaries of inlined code

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC345746: [analyzer] RetainCountChecker: for now, do not trust 
the summaries of inlined… (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53902?vs=171816=171964#toc

Repository:
  rC Clang

https://reviews.llvm.org/D53902

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
  test/Analysis/osobject-retain-release.cpp

Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -2,10 +2,9 @@
 
 struct OSMetaClass;
 
-#define TRUSTED __attribute__((annotate("rc_ownership_trusted_implementation")))
-#define OS_CONSUME TRUSTED __attribute__((annotate("rc_ownership_consumed")))
-#define OS_RETURNS_RETAINED TRUSTED __attribute__((annotate("rc_ownership_returns_retained")))
-#define OS_RETURNS_NOT_RETAINED TRUSTED __attribute__((annotate("rc_ownership_returns_not_retained")))
+#define OS_CONSUME __attribute__((annotate("rc_ownership_consumed")))
+#define OS_RETURNS_RETAINED __attribute__((annotate("rc_ownership_returns_retained")))
+#define OS_RETURNS_NOT_RETAINED __attribute__((annotate("rc_ownership_returns_not_retained")))
 
 #define OSTypeID(type)   (type::metaClass)
 
@@ -62,6 +61,37 @@
   // expected-note@-1{{Object leaked}}
 }
 
+struct ArrayOwner : public OSObject {
+  OSArray *arr;
+  ArrayOwner(OSArray *arr) : arr(arr) {}
+
+  static ArrayOwner* create(OSArray *arr) {
+return new ArrayOwner(arr);
+  }
+
+  OSArray *getArray() {
+return arr;
+  }
+
+  OSArray *createArray() {
+return OSArray::withCapacity(10);
+  }
+
+  OSArray *createArraySourceUnknown();
+
+  OSArray *getArraySourceUnknown();
+};
+
+void check_confusing_getters() {
+  OSArray *arr = OSArray::withCapacity(10);
+
+  ArrayOwner *AO = ArrayOwner::create(arr);
+  AO->getArray();
+
+  AO->release();
+  arr->release();
+}
+
 void check_rc_consumed() {
   OSArray *arr = OSArray::withCapacity(10);
   OSArray::consumeArray(arr);
@@ -140,31 +170,17 @@
   arr->release(); // 0
 }
 
-struct ArrayOwner {
-  OSArray *arr;
-
-  OSArray *getArray() {
-return arr;
-  }
-
-  OSArray *createArray() {
-return OSArray::withCapacity(10);
-  }
-
-  OSArray *createArraySourceUnknown();
-
-  OSArray *getArraySourceUnknown();
-};
-
 unsigned int no_warning_on_getter(ArrayOwner *owner) {
   OSArray *arr = owner->getArray();
   return arr->getCount();
 }
 
 unsigned int warn_on_overrelease(ArrayOwner *owner) {
-  OSArray *arr = owner->getArray(); // expected-note{{function call returns an OSObject of type struct OSArray * with a +0 retain count}}
-  arr->release(); // expected-warning{{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}}
-  // expected-note@-1{{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}}
+  // FIXME: summaries are not applied in case the source of the getter/setter
+  // is known.
+  // rdar://45681203
+  OSArray *arr = owner->getArray();
+  arr->release();
   return arr->getCount();
 }
 
Index: lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
===
--- lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
+++ lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
@@ -101,10 +101,11 @@
 return true;
 
   for (ParmVarDecl *Param : MD->parameters()) {
-QualType PT = Param->getType();
-if (CXXRecordDecl *RD = PT->getPointeeType()->getAsCXXRecordDecl())
-  if (isOSObjectSubclass(RD))
-return true;
+QualType PT = Param->getType()->getPointeeType();
+if (!PT.isNull())
+  if (CXXRecordDecl *RD = PT->getAsCXXRecordDecl())
+if (isOSObjectSubclass(RD))
+  return true;
   }
 
   return false;
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -420,13 +420,6 @@
   RetEffect RE = Summ.getRetEffect();
 
   if (SymbolRef Sym = CallOrMsg.getReturnValue().getAsSymbol()) {
-if (const auto *MCall = dyn_cast()) {
-  if (Optional updatedRefVal =
-  refValFromRetEffect(RE, MCall->getResultType())) {
-state = setRefBinding(state, Sym, *updatedRefVal);
-  }
-}
-
 if (RE.getKind() == RetEffect::NoRetHard)
   state = removeRefBinding(state, Sym);
   }
@@ -1103,9 +1096,8 @@
   WhitelistedSymbols.insert(SR->getSymbol());
   }
 
-  for (InvalidatedSymbols::const_iterator I=invalidated->begin(),
-   E = 

[PATCH] D53628: [analyzer] Remove custom rule for OSIterator in RetainCountChecker

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

Actually, I'm reverting this: the rule seems fairly ubiquitous.


Repository:
  rC Clang

https://reviews.llvm.org/D53628



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

[PATCH] D53814: Allow the analyzer to output to a SARIF file

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov accepted this revision.
george.karpenkov added a comment.
This revision is now accepted and ready to land.

I much prefer this version.
We've had the same problem with diffing plist output.
One thing we have learned is using a FileCheck was definitely a bad idea, as it 
leads to unreadable, unmaintainable, and very hard to update tests,
so either diff or your custom tool is way better.

As for the ultimate solution, I'm still not sure: I agree that maintaining 
those `-I` flags is annoying.
One option is having an extra flag to produce "stable" output, which does not 
include absolute URLs/versions/etc.


https://reviews.llvm.org/D53814



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

[PATCH] D53814: Allow the analyzer to output to a SARIF file

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov requested changes to this revision.
george.karpenkov added a comment.
This revision now requires changes to proceed.

I don't think a new PathGenerationScheme is needed, unless you plan changes to 
BugReporter.cpp.

The code is fine otherwise, but could we try to use `diff` for testing?




Comment at: Analysis/diagnostics/sarif-diagnostics-taint-test.c:18
+// Test the basics for sanity.
+// CHECK: sarifLog['version'].startswith("2.0.0")
+// CHECK: sarifLog['runs'][0]['tool']['fullName'] == "clang static analyzer"

aaron.ballman wrote:
> george.karpenkov wrote:
> > Would it make more sense to just use `diff` + json pretty-formatter to 
> > write a test?
> > With this test I can't even quite figure out how the output should look 
> > like.
> I'm not super comfortable with that approach, but perhaps I'm thinking of 
> something different than what you're proposing. The reason I went with this 
> approach is because diff would be fragile (depends heavily on field ordering, 
> which the JSON support library doesn't give control over) and the physical 
> layout of the file isn't what needs to be tested anyway. SARIF has a fair 
> amount of optional data that can be provided as well, so using a purely 
> textual diff worried me that exporting additional optional data in the future 
> would require extensive unrelated changes to all SARIF diffs in the test 
> directory.
> 
> The goal for this test was to demonstrate that we can validate that the 
> interesting bits of information are present in the output without worrying 
> about the details.
> 
> Also, the python approach allows us to express relationships between data 
> items more easily than a textual diff tool would. I've not used that here, 
> but I could imagine a test where we want to check that each code location has 
> a corresponding file entry in another list.
Using a sample file + diff would have an advantage of being easier to read 
(just glance at the "Inputs/blah.serif" to see a sample output), and consistent 
with how we already do checking for plists.

> depends heavily on field ordering

Is it an issue in practice though? I would assume that JSON support library 
would not switch field ordering too often (and even if it does, we can have a 
python wrapper testing that)

> SARIF has a fair amount of optional data

Would diff `--ignore-matching-lines` be enough for those?



Comment at: StaticAnalyzer/Core/SarifDiagnostics.cpp:69
+return std::string(, 1);
+  return "%" + llvm::toHex(StringRef(, 1));
+}

+1, I would use this in other consumers.



Comment at: clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h:128
+/// Used for SARIF output.
+Sarif,
   };

Do you actually need a new generation scheme here?
I'm pretty sure that using "Minimal" would give you the same effect.


https://reviews.llvm.org/D53814



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

[PATCH] D53814: Allow the analyzer to output to a SARIF file

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov requested changes to this revision.
george.karpenkov added a comment.
This revision now requires changes to proceed.

Minor style notes + context missing.

I think using `diff` would be better than a custom python tool.


https://reviews.llvm.org/D53814



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

[PATCH] D53814: Allow the analyzer to output to a SARIF file

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

Patch context is missing.




Comment at: Analysis/diagnostics/sarif-check.py:22
+passes = 0
+with open(testfile) as testfh:
+lineno = 0

Wow, this is super neat!
Since you are quite active in LLVM community, would you think it's better to 
have this tool in `llvm/utils` next to FileCheck? The concept is very general, 
and I think a lot of people really feel FileCheck limitations.



Comment at: Analysis/diagnostics/sarif-diagnostics-taint-test.c:18
+// Test the basics for sanity.
+// CHECK: sarifLog['version'].startswith("2.0.0")
+// CHECK: sarifLog['runs'][0]['tool']['fullName'] == "clang static analyzer"

Would it make more sense to just use `diff` + json pretty-formatter to write a 
test?
With this test I can't even quite figure out how the output should look like.



Comment at: StaticAnalyzer/Core/SarifDiagnostics.cpp:74
+  // Add the rest of the path components, encoding any reserved characters.
+  std::for_each(std::next(sys::path::begin(Filename)), 
sys::path::end(Filename),
+[](StringRef Component) {

Nitpicking style, but I don't see why for-each loop, preferably with a range 
wrapping the iterators would not be more readable.



Comment at: StaticAnalyzer/Core/SarifDiagnostics.cpp:182
+  case PathDiagnosticPiece::Kind::Note:
+// FIXME: What should be reported here?
+break;

"Note" are notes which do not have to be attached to a particular path element.



Comment at: StaticAnalyzer/Core/SarifDiagnostics.cpp:243
+
+  llvm::for_each(Diags, [&](const PathDiagnostic *D) {
+Results.push_back(createResult(*D, Files));

I like closures, but what's wrong with just using a `for` loop here?



Comment at: StaticAnalyzer/Core/SarifDiagnostics.cpp:254
+std::vector , FilesMade *) {
+  // FIXME: if the file already exists, do we overwrite it with a single run,
+  // or do we append a run into the file if it's a valid SARIF log?

Usually we overwrite the file and note that on stderr in such cases.


https://reviews.llvm.org/D53814



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

[PATCH] D53628: [analyzer] Remove custom rule for OSIterator in RetainCountChecker

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC345339: [analyzer] Remove custom rule for OSIterator in 
RetainCountChecker (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53628?vs=170796=171225#toc

Repository:
  rC Clang

https://reviews.llvm.org/D53628

Files:
  lib/StaticAnalyzer/Core/RetainSummaryManager.cpp


Index: lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
===
--- lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
+++ lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
@@ -69,10 +69,6 @@
   return S == "safeMetaCast";
 }
 
-static bool isOSIteratorSubclass(const Decl *D) {
-  return isSubclass(D, "OSIterator");
-}
-
 static bool hasRCAnnotation(const Decl *D, StringRef rcAnnotation) {
   for (const auto *Ann : D->specific_attrs()) {
 if (Ann->getAnnotation() == rcAnnotation)
@@ -240,10 +236,6 @@
 
 // All objects returned with functions starting with "get" are getters.
 if (II->getName().startswith("get")) {
-
-  // ...except for iterators.
-  if (isOSIteratorSubclass(PD))
-return getOSSummaryCreateRule(FD);
   return getOSSummaryGetRule(FD);
 } else {
   return getOSSummaryCreateRule(FD);


Index: lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
===
--- lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
+++ lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
@@ -69,10 +69,6 @@
   return S == "safeMetaCast";
 }
 
-static bool isOSIteratorSubclass(const Decl *D) {
-  return isSubclass(D, "OSIterator");
-}
-
 static bool hasRCAnnotation(const Decl *D, StringRef rcAnnotation) {
   for (const auto *Ann : D->specific_attrs()) {
 if (Ann->getAnnotation() == rcAnnotation)
@@ -240,10 +236,6 @@
 
 // All objects returned with functions starting with "get" are getters.
 if (II->getName().startswith("get")) {
-
-  // ...except for iterators.
-  if (isOSIteratorSubclass(PD))
-return getOSSummaryCreateRule(FD);
   return getOSSummaryGetRule(FD);
 } else {
   return getOSSummaryCreateRule(FD);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov requested changes to this revision.
george.karpenkov added a comment.
This revision now requires changes to proceed.

Actually, apologies again: I have rushed through this too much.
@NoQ has a good point: we need to preserve the distinction between the things 
analyzer "assumes" and the things analyzer "knows".


https://reviews.llvm.org/D53076



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

[PATCH] D53276: [analyzer][NFC] Fix some incorrect uses of -analyzer-config options

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov requested changes to this revision.
george.karpenkov added a comment.
This revision now requires changes to proceed.

Minor nitpicks, otherwise makes sense!




Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:746
+  /// exploring a top level function (for each exploded graph). 0 means no 
limit.
+  unsigned getRegionStoreSmallStructLimit();
+

The comment seems completely distinct from the function name.



Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:748
+
+  StringRef getModelPath();
 };

Comment?


https://reviews.llvm.org/D53276



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov requested changes to this revision.
george.karpenkov added a comment.
This revision now requires changes to proceed.

Thanks a lot, this is almost ready!
I think it should be good to go after this round of nitpicks.




Comment at: 
include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h:168
 
-  virtual void EndPath(ProgramStateRef state) {}
-

From a brief inspection this indeed seems dead code. However, this removal 
should be moved into a separate revision (sorry!)



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:680
-ConstraintMgr->EndPath(St);
-  }
 };

Same: should be moved into a separate revision, same as the other removal.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1853
+
+const auto *Cond = cast(PS->getStmt());
+auto Piece = VisitTrueTest(Cond, tag == tags.first, BRC, BR, N);

How do we know that it's always an `Expr`?



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1854
+const auto *Cond = cast(PS->getStmt());
+auto Piece = VisitTrueTest(Cond, tag == tags.first, BRC, BR, N);
+if (!Piece)

`/*tookTrue=*/tag == tag.first`



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1970
 
+bool ConditionBRVisitor::insertOrUpdateConstraintMessage(const Stmt *Cond,
+ StringRef Message) {

1. What about the refactoring I have previously suggested twice?
2. I know you have started with that -- and sorry for spurious changes -- but I 
also think your original name of `constraintsChanged` is better.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2261
   PrettyStackTraceLocationContext CrashInfo(Pred->getLocationContext());
-  StateMgr.EndPath(Pred->getState());
 

Should be moved together with other two removals.



Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:29
 
+/// Increments the number of times this state is referenced.
 void ProgramStateRetain(const ProgramState *state) {

While this minor formatting is correct, it's better to remove it to simplify 
future archaeology.


https://reviews.llvm.org/D53076



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

[PATCH] D18860: [analyzer] Fix the "Zombie symbols" issue.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov accepted this revision.
george.karpenkov added inline comments.
This revision is now accepted and ready to land.



Comment at: test/Analysis/self-assign.cpp:42
   str = rhs.str;
-  rhs.str = nullptr; // FIXME: An improved leak checker should warn here
+  rhs.str = nullptr; // expected-warning{{Potential memory leak}} 
expected-note{{Potential memory leak}}
   return *this;

Woo-hoo!



Comment at: test/Analysis/simple-stream-checks.c:96
+  fp = 0;
+} // expected-warning {{Opened file is never closed; potential resource leak}}

Woo-hoo, were we losing this case before?


https://reviews.llvm.org/D18860



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

[PATCH] D53206: Allow padding checker to traverse simple class hierarchies

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

I'll try to take a look this week.


Repository:
  rC Clang

https://reviews.llvm.org/D53206



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov requested changes to this revision.
george.karpenkov added inline comments.
This revision now requires changes to proceed.



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:80
   typedef llvm::ImmutableMap GenericDataMap;
+  typedef llvm::DenseMap  ConstraintMap;
 

Generally, `StringRef`'s shouldn't be stored in containers, as containers might 
outlive them.
It might be fine in this case, but I would prefer to be on the safe side and 
just use `std::string`



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:238
+  /// message on that constraint being changed.
+  bool isChangedOrInsertConstraint(ConstraintMap , const Stmt 
*Cond,
+   StringRef Message) const {

whisperity wrote:
> `insertOrUpdateContraintMessage`
> 
> and mention in the documentation that it returns whether or not the actual 
> insertion or update change took place
If constraints is now a property of the visitor, shouldn't this method with the 
typedef above be moved to the visitor as well?



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:244
+if (I == Constraints.end() || !Message.equals(I->second)) {
+  Constraints[Cond] = Message;
+  return true;

Isn't that equivalent to `Constraints.insert(make_pair(Cond, Message)).second` ?
I think I have written that before.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2232
+ const ExplodedNode *N, BugReport ) 
{
+  Constraints.clear();
+}

It does not seem necessary, because a new copy of the visitor is created for 
each new bug report.


https://reviews.llvm.org/D53076



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

[PATCH] D53069: [analyzer][www] Update avaible_checks.html

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov requested changes to this revision.
george.karpenkov added a comment.
This revision now requires changes to proceed.

Good to go provided you will add an example.
If we want to be serious about this page, it really has to be auto-generated 
(like clang-tidy one), but I understand that this is a larger undertaking.




Comment at: www/analyzer/available_checks.html:496
+
+
+

Szelethus wrote:
> george.karpenkov wrote:
> > If we don't have a description, let's just drop it.
> I added a description, I'd strongly prefer keeping this in.
Also would need a short example. Usually one can be found in tests.


https://reviews.llvm.org/D53069



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

[PATCH] D53615: [analyzer] [NFC] Change scanReachableSymbols to use ranges

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC345101: [analyzer] [NFC] Change scanReachableSymbols to use 
ranges (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53615?vs=170761=170779#toc

Repository:
  rC Clang

https://reviews.llvm.org/D53615

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Core/ProgramState.cpp

Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -662,22 +662,12 @@
   return S.scan(val);
 }
 
-bool ProgramState::scanReachableSymbols(const SVal *I, const SVal *E,
-   SymbolVisitor ) const {
+bool ProgramState::scanReachableSymbols(
+llvm::iterator_range Reachable,
+SymbolVisitor ) const {
   ScanReachableSymbols S(this, visitor);
-  for ( ; I != E; ++I) {
-if (!S.scan(*I))
-  return false;
-  }
-  return true;
-}
-
-bool ProgramState::scanReachableSymbols(const MemRegion * const *I,
-   const MemRegion * const *E,
-   SymbolVisitor ) const {
-  ScanReachableSymbols S(this, visitor);
-  for ( ; I != E; ++I) {
-if (!S.scan(*I))
+  for (const MemRegion *R : Reachable) {
+if (!S.scan(R))
   return false;
   }
   return true;
@@ -845,4 +835,3 @@
 
   return false;
 }
-
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -2539,8 +2539,7 @@
   }
 
   state =
-state->scanReachableSymbols(Regions.data(),
-Regions.data() + Regions.size()).getState();
+state->scanReachableSymbols(Regions).getState();
   C.addTransition(state);
 }
 
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -175,9 +175,7 @@
 Regions.push_back(VR);
   }
 
-  state =
-state->scanReachableSymbols(Regions.data(),
-Regions.data() + Regions.size()).getState();
+  state = state->scanReachableSymbols(Regions).getState();
   C.addTransition(state);
 }
 
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
@@ -348,6 +348,8 @@
   /// a value of such type.
   SVal getSValAsScalarOrLoc(const MemRegion *R) const;
 
+  using region_iterator = const MemRegion **;
+
   /// Visits the symbols reachable from the given SVal using the provided
   /// SymbolVisitor.
   ///
@@ -357,24 +359,14 @@
   /// \sa ScanReachableSymbols
   bool scanReachableSymbols(SVal val, SymbolVisitor& visitor) const;
 
-  /// Visits the symbols reachable from the SVals in the given range
-  /// using the provided SymbolVisitor.
-  bool scanReachableSymbols(const SVal *I, const SVal *E,
-SymbolVisitor ) const;
-
   /// Visits the symbols reachable from the regions in the given
   /// MemRegions range using the provided SymbolVisitor.
-  bool scanReachableSymbols(const MemRegion * const *I,
-const MemRegion * const *E,
+  bool scanReachableSymbols(llvm::iterator_range Reachable,
 SymbolVisitor ) const;
 
   template  CB scanReachableSymbols(SVal val) const;
-  template  CB scanReachableSymbols(const SVal *beg,
- const SVal *end) const;
-
   template  CB
-  scanReachableSymbols(const MemRegion * const *beg,
-   const MemRegion * const *end) const;
+  scanReachableSymbols(llvm::iterator_range Reachable) const;
 
   /// Create a new state in which the statement is marked as tainted.
   LLVM_NODISCARD ProgramStateRef
@@ -883,17 +875,10 @@
 }
 
 template 
-CB ProgramState::scanReachableSymbols(const SVal *beg, const SVal *end) const {
-  CB cb(this);
-  scanReachableSymbols(beg, end, cb);
-  return cb;
-}
-
-template 
-CB ProgramState::scanReachableSymbols(const MemRegion * const *beg,
- const MemRegion * const *end) const {
+CB ProgramState::scanReachableSymbols(
+llvm::iterator_range Reachable) const {
   CB cb(this);
-  scanReachableSymbols(beg, end, cb);
+  scanReachableSymbols(Reachable, cb);
   

[PATCH] D53550: [analyzer] Do not stop tracking CXX methods touching OSObject.

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC345100: [analyzer] Do not stop tracking CXX methods touching 
OSObject. (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53550?vs=170760=170778#toc

Repository:
  rC Clang

https://reviews.llvm.org/D53550

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
  test/Analysis/osobject-retain-release.cpp

Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -2,6 +2,11 @@
 
 struct OSMetaClass;
 
+#define TRUSTED __attribute__((annotate("rc_ownership_trusted_implementation")))
+#define OS_CONSUME TRUSTED __attribute__((annotate("rc_ownership_consumed")))
+#define OS_RETURNS_RETAINED TRUSTED __attribute__((annotate("rc_ownership_returns_retained")))
+#define OS_RETURNS_NOT_RETAINED TRUSTED __attribute__((annotate("rc_ownership_returns_not_retained")))
+
 #define OSTypeID(type)   (type::metaClass)
 
 #define OSDynamicCast(type, inst)   \
@@ -21,14 +26,53 @@
   unsigned int getCount();
 
   static OSArray *withCapacity(unsigned int capacity);
+  static void consumeArray(OS_CONSUME OSArray * array);
+
+  static OSArray* consumeArrayHasCode(OS_CONSUME OSArray * array) {
+return nullptr;
+  }
+
+  static OS_RETURNS_NOT_RETAINED OSArray *MaskedGetter();
+  static OS_RETURNS_RETAINED OSArray *getOoopsActuallyCreate();
+
 
   static const OSMetaClass * const metaClass;
 };
 
+struct OtherStruct {
+  static void doNothingToArray(OSArray *array);
+};
+
 struct OSMetaClassBase {
   static OSObject *safeMetaCast(const OSObject *inst, const OSMetaClass *meta);
 };
 
+void check_no_invalidation() {
+  OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to function 'withCapacity' returns an OSObject of type struct OSArray * with a +1 retain count}}
+  OtherStruct::doNothingToArray(arr);
+} // expected-warning{{Potential leak of an object stored into 'arr'}}
+  // expected-note@-1{{Object leaked}}
+
+void check_rc_consumed() {
+  OSArray *arr = OSArray::withCapacity(10);
+  OSArray::consumeArray(arr);
+}
+
+void check_rc_consume_temporary() {
+  OSArray::consumeArray(OSArray::withCapacity(10));
+}
+
+void check_rc_getter() {
+  OSArray *arr = OSArray::MaskedGetter();
+  (void)arr;
+}
+
+void check_rc_create() {
+  OSArray *arr = OSArray::getOoopsActuallyCreate();
+  arr->release();
+}
+
+
 void check_dynamic_cast() {
   OSArray *arr = OSDynamicCast(OSArray, OSObject::generateObject(1));
   arr->release();
@@ -80,11 +124,6 @@
   OSArray *getArraySourceUnknown();
 };
 
-//unsigned int leak_on_create_no_release(ArrayOwner *owner) {
-  //OSArray *myArray = 
-
-//}
-
 unsigned int no_warning_on_getter(ArrayOwner *owner) {
   OSArray *arr = owner->getArray();
   return arr->getCount();
Index: lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
===
--- lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
+++ lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
@@ -8,8 +8,8 @@
 //===--===//
 //
 //  This file defines summaries implementation for retain counting, which
-//  implements a reference count checker for Core Foundation and Cocoa
-//  on (Mac OS X).
+//  implements a reference count checker for Core Foundation, Cocoa
+//  and OSObject (on Mac OS X).
 //
 //===--===//
 
@@ -94,6 +94,22 @@
   return FName.contains_lower("MakeCollectable");
 }
 
+/// A function is OSObject related if it is declared on a subclass
+/// of OSObject, or any of the parameters is a subclass of an OSObject.
+static bool isOSObjectRelated(const CXXMethodDecl *MD) {
+  if (isOSObjectSubclass(MD->getParent()))
+return true;
+
+  for (ParmVarDecl *Param : MD->parameters()) {
+QualType PT = Param->getType();
+if (CXXRecordDecl *RD = PT->getPointeeType()->getAsCXXRecordDecl())
+  if (isOSObjectSubclass(RD))
+return true;
+  }
+
+  return false;
+}
+
 const RetainSummary *
 RetainSummaryManager::generateSummary(const FunctionDecl *FD,
   bool ) {
@@ -322,12 +338,10 @@
 }
   }
 
-  if (isa(FD)) {
-
-// Stop tracking arguments passed to C++ methods, as those might be
-// wrapping smart pointers.
-return getPersistentSummary(RetEffect::MakeNoRet(), DoNothing, StopTracking,
-DoNothing);
+  if (const auto *MD = dyn_cast(FD)) {
+if (!(TrackOSObjects && isOSObjectRelated(MD)))
+  return getPersistentSummary(RetEffect::MakeNoRet(), DoNothing, StopTracking,
+  DoNothing);
   }
 
   return 

[PATCH] D53549: [analyzer] Trust summaries for OSObject::retain and OSObject::release

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC345099: [analyzer] Trust summaries for OSObject::retain and 
OSObject::release (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53549?vs=170553=170777#toc

Repository:
  rC Clang

https://reviews.llvm.org/D53549

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
  test/Analysis/osobject-retain-release.cpp

Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -9,7 +9,7 @@
 
 struct OSObject {
   virtual void retain();
-  virtual void release();
+  virtual void release() {};
   virtual ~OSObject(){}
 
   static OSObject *generateObject(int);
Index: lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
===
--- lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
+++ lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
@@ -102,9 +102,6 @@
 return getPersistentStopSummary();
   }
 
-  // [PR 3337] Use 'getAs' to strip away any typedefs on the
-  // function's type.
-  const FunctionType *FT = FD->getType()->getAs();
   const IdentifierInfo *II = FD->getIdentifier();
   if (!II)
 return getDefaultSummary();
@@ -115,7 +112,8 @@
   // down below.
   FName = FName.substr(FName.find_first_not_of('_'));
 
-  // Inspect the result type.
+  // Inspect the result type. Strip away any typedefs.
+  const auto *FT = FD->getType()->getAs();
   QualType RetTy = FT->getReturnType();
   std::string RetTyName = RetTy.getAsString();
 
@@ -506,12 +504,6 @@
 bool RetainSummaryManager::canEval(const CallExpr *CE,
const FunctionDecl *FD,
bool ) {
-  // For now, we're only handling the functions that return aliases of their
-  // arguments: CFRetain (and its families).
-  // Eventually we should add other functions we can model entirely,
-  // such as CFRelease, which don't invalidate their arguments or globals.
-  if (CE->getNumArgs() != 1)
-return false;
 
   IdentifierInfo *II = FD->getIdentifier();
   if (!II)
@@ -533,13 +525,26 @@
   return isRetain(FD, FName) || isAutorelease(FD, FName) ||
  isMakeCollectable(FName);
 
+// Process OSDynamicCast: should just return the first argument.
+// For now, treating the cast as a no-op, and disregarding the case where
+// the output becomes null due to the type mismatch.
+if (TrackOSObjects && FName == "safeMetaCast") {
+  return true;
+}
+
 const FunctionDecl* FDD = FD->getDefinition();
 if (FDD && isTrustedReferenceCountImplementation(FDD)) {
   hasTrustedImplementationAnnotation = true;
   return true;
 }
   }
 
+  if (const auto *MD = dyn_cast(FD)) {
+const CXXRecordDecl *Parent = MD->getParent();
+if (TrackOSObjects && Parent && isOSObjectSubclass(Parent))
+  return FName == "release" || FName == "retain";
+  }
+
   return false;
 
 }
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -776,31 +776,27 @@
 
   const LocationContext *LCtx = C.getLocationContext();
 
-  // Process OSDynamicCast: should just return the first argument.
-  // For now, tresting the cast as a no-op, and disregarding the case where
-  // the output becomes null due to the type mismatch.
-  if (FD->getNameAsString() == "safeMetaCast") {
-state = state->BindExpr(CE, LCtx, 
-state->getSVal(CE->getArg(0), LCtx));
-C.addTransition(state);
-return true;
-  }
-
   // See if it's one of the specific functions we know how to eval.
   if (!SmrMgr.canEval(CE, FD, hasTrustedImplementationAnnotation))
 return false;
 
   // Bind the return value.
-  SVal RetVal = state->getSVal(CE->getArg(0), LCtx);
-  if (RetVal.isUnknown() ||
-  (hasTrustedImplementationAnnotation && !ResultTy.isNull())) {
+  // For now, all the functions which we can evaluate and which take
+  // at least one argument are identities.
+  if (CE->getNumArgs() >= 1) {
+SVal RetVal = state->getSVal(CE->getArg(0), LCtx);
+
 // If the receiver is unknown or the function has
 // 'rc_ownership_trusted_implementation' annotate attribute, conjure a
 // return value.
-SValBuilder  = C.getSValBuilder();
-RetVal = SVB.conjureSymbolVal(nullptr, CE, LCtx, ResultTy, C.blockCount());
+if (RetVal.isUnknown() ||
+(hasTrustedImplementationAnnotation && !ResultTy.isNull())) {
+  SValBuilder  = C.getSValBuilder();
+  RetVal 

[PATCH] D53543: [analyzer] MallocChecker: pr39348: Realize that sized delete isn't a custom delete.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

> because the guard that prevents it from working is useless and can be removed 
> as well

Should we remove it then?




Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:729
+  SourceLocation L = FD->getLocation();
+  return !L.isValid() || C.getSourceManager().isInSystemHeader(L);
 }

Should we return true on invalid source location?
Do we have a test case for that?



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2444
-isStandardNewDelete(FD, Ctx))
-  return;
   }

why it's fine to remove this branch?



Comment at: test/Analysis/NewDelete-custom.cpp:7
 
-#if !(LEAKS && !ALLOCATOR_INLINING)
 // expected-no-diagnostics
 

MTC wrote:
> Should we continue to keep this line?
yes, since there are not diagnostics emitted. it would complain otherwise.



Comment at: test/Analysis/NewDelete-sized-deallocation.cpp:1
+// RUN: %clang_analyze_cc1 -std=c++17 -analyzer-checker=core,cplusplus -verify 
-analyzer-output=text %s
+// RUN: %clang_analyze_cc1 -std=c++17 -analyzer-checker=core,cplusplus -verify 
-analyzer-output=text %s -fsized-deallocation

MTC wrote:
> I believe `sized deallocation` is the feature since C++14, see 
> https://en.cppreference.com/w/cpp/memory/new/operator_delete. 
specifying 17 is also fine


https://reviews.llvm.org/D53543



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov requested changes to this revision.
george.karpenkov added inline comments.
This revision now requires changes to proceed.



Comment at: 
include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h:202
+  void finalizeConstraints() {
+Constraints.clear();
+  }

These constraints are conceptually part of the visitor, not part of the 
constraint manager. Could they be simply stored in the visitor?



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:235
+  /// Check if the state has changed based on the constraint has changed.
+  bool isChanged(const Stmt *Cond, StringRef Message) const;
+

Probably should be replaced by the expression above and inlined.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:187
 
-void
-BugReporterVisitor::finalizeVisitor(BugReporterContext &,
-const ExplodedNode *, BugReport &) {}
+void BugReporterVisitor::finalizeVisitor(BugReporterContext &,
+ const ExplodedNode *, BugReport &) {}

spurious change



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1831
+  const Stmt *Cond = srcBlk->getTerminatorCondition();
+  auto piece = VisitTerminator(term, N, srcBlk, BE->getDst(), BR, BRC);
+  if (piece && State->isChanged(Cond, piece->getString()))

capital letter


https://reviews.llvm.org/D53076



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added inline comments.



Comment at: 
include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h:80
 class ConstraintManager {
+  using ConstraintMap = std::map;
+  ConstraintMap Constraints;

Traditionally LLVM projects use `llvm::DenseMap`



Comment at: 
include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h:183
+  bool isChanged(const Stmt *Cond, StringRef Message) {
+ConstraintMap::iterator I = Constraints.find(Cond);
+

`return Constraints.insert(make_pair(Cond, Message)).second` ?


https://reviews.llvm.org/D53076



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

[PATCH] D52844: [analyzer] [testing] Compute data on path length, compute percentiles

[George Karpenkov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC344990: [analyzer] [testing] Compute data on path length, 
compute percentiles (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D52844?vs=168160=170556#toc

Repository:
  rC Clang

https://reviews.llvm.org/D52844

Files:
  utils/analyzer/CmpRuns.py


Index: utils/analyzer/CmpRuns.py
===
--- utils/analyzer/CmpRuns.py
+++ utils/analyzer/CmpRuns.py
@@ -281,9 +281,21 @@
 
 return res
 
+def computePercentile(l, percentile):
+"""
+Return computed percentile.
+"""
+return sorted(l)[int(round(percentile * len(l) + 0.5)) - 1]
+
 def deriveStats(results):
 # Assume all keys are the same in each statistics bucket.
 combined_data = defaultdict(list)
+
+# Collect data on paths length.
+for report in results.reports:
+for diagnostic in report.diagnostics:
+combined_data['PathsLength'].append(diagnostic.getPathLength())
+
 for stat in results.stats:
 for key, value in stat.iteritems():
 combined_data[key].append(value)
@@ -293,6 +305,8 @@
 "max": max(values),
 "min": min(values),
 "mean": sum(values) / len(values),
+"90th %tile": computePercentile(values, 0.9),
+"95th %tile": computePercentile(values, 0.95),
 "median": sorted(values)[len(values) / 2],
 "total": sum(values)
 }


Index: utils/analyzer/CmpRuns.py
===
--- utils/analyzer/CmpRuns.py
+++ utils/analyzer/CmpRuns.py
@@ -281,9 +281,21 @@
 
 return res
 
+def computePercentile(l, percentile):
+"""
+Return computed percentile.
+"""
+return sorted(l)[int(round(percentile * len(l) + 0.5)) - 1]
+
 def deriveStats(results):
 # Assume all keys are the same in each statistics bucket.
 combined_data = defaultdict(list)
+
+# Collect data on paths length.
+for report in results.reports:
+for diagnostic in report.diagnostics:
+combined_data['PathsLength'].append(diagnostic.getPathLength())
+
 for stat in results.stats:
 for key, value in stat.iteritems():
 combined_data[key].append(value)
@@ -293,6 +305,8 @@
 "max": max(values),
 "min": min(values),
 "mean": sum(values) / len(values),
+"90th %tile": computePercentile(values, 0.9),
+"95th %tile": computePercentile(values, 0.95),
 "median": sorted(values)[len(values) / 2],
 "total": sum(values)
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53206: Allow padding checker to traverse simple class hierarchies

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

@bcraig comments look valid, marking as "Needs Changes"


Repository:
  rC Clang

https://reviews.llvm.org/D53206



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

[PATCH] D53483: [analyzer] Restrict AnalyzerOptions' interface so that non-checker objects have to be registered

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

OK, so the overall direction makes sense: unregistered options are restricted 
to checkers, and for others, an explicit getter must be maintained.
(though I would also prefer if even checkers could pre-register their options 
somehow)

@NoQ does this make sense to you?




Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:243
+  /// specified.
+  StringRef getStringOption(StringRef Name, StringRef DefaultVal);
 

xazax.hun wrote:
> If you want the devs to maintain an explicit getter for each analyzer option 
> rather than making this one public at some point, please document expectation 
> this somewhere.
+1


Repository:
  rC Clang

https://reviews.llvm.org/D53483



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

[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like elements

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.
Herald added subscribers: dkrupp, donat.nagy.

I'm marking it as "needs changes" for now -- the idea seems reasonable, but 
it's hard to tell without a thorough evaluation on at least a few codebases.
Additionally, we should figure out a checker group for this - if "alpha" means 
"work in progress", then it should be under something else, like "linting" or 
similar.


https://reviews.llvm.org/D50488



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

[PATCH] D53069: [analyzer][www] Update avaible_checks.html

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov requested changes to this revision.
george.karpenkov added inline comments.
This revision now requires changes to proceed.



Comment at: www/analyzer/available_checks.html:459
 
+
 

Spurious newline



Comment at: www/analyzer/available_checks.html:496
+
+
+

If we don't have a description, let's just drop it.



Comment at: www/analyzer/available_checks.html:677
+
+void use_semaphor_antipattern() {
+  dispatch_semaphore_t sema = dispatch_semaphore_create(0);

I have a longer description:

This checker finds a common performance anti-pattern in a code that uses Grand 
Central dispatch.
The anti-pattern involves emulating a synchronous call from an asynchronous API 
using semaphores,
as in the snippet below, where the `requestCurrentTaskName` function makes an 
XPC call and then uses the semaphore to
block until the XPC call returns:

```
+ (NSString *)requestCurrentTaskName {
__block NSString *taskName = nil;
dispatch_semaphore_t sema = dispatch_semaphore_create(0);
NSXPCConnection *connection = [[NSXPCConnection alloc] 
initWithServiceName:@"MyConnection"];
id remoteObjectProxy = connection.remoteObjectProxy;
[remoteObjectProxy requestCurrentTaskName:^(NSString *task) {
taskName = task;
dispatch_semaphore_signal(sema);
}];
dispatch_semaphore_wait(sema, DISPATCH_TIME_FOREVER);
return taskName;
}
```

Usage of such a pattern in production code running on the main thread is 
discouraged, as the main queue gets blocked waiting for the background queue,
which could be running at a lower priority, and unnecessary threads are spawned 
in the process.

In order to avoid the anti-pattern, the available alternatives are:

 - Use the synchronous version of the API, if available.
In the example above, the synchronous XPC proxy object can be used:

```
+ (NSString *)requestCurrentTaskName {
__block NSString *taskName = nil;
NSXPCConnection *connection = [[NSXPCConnection alloc] 
initWithServiceName:@"MyConnection"];
id remoteObjectProxy = [connection 
synchronousRemoteObjectProxyWithErrorHandler:^(NSError *error) {
  NSLog(@"Error = %@", error);

}];
[remoteObjectProxy requestCurrentTaskName:^(NSString *task) {
taskName = task;
}];
return taskName;
}
```

 - Alternatively, the API can be used in the asynchronous way.




Comment at: www/analyzer/available_checks.html:768
+Check for proper uses of Objective-C properties
+
+

If we don't have proper description, let's drop.



Comment at: www/analyzer/available_checks.html:877
+(ObjC)
+Warn about potentially crashing writes to autoreleasing objects from different
+autoreleasing pools in Objective-C.

I have a longer description:

Under ARC, function parameters which are pointers to pointers (e.g. NSError**) 
are `__autoreleasing`.
Writing to such parameters inside autoreleasing pools might crash whenever the 
parameter outlives the pool. Detecting such crashes may be difficult, as usage 
of autorelease pool is usually hidden inside the called functions 
implementation. Examples include:

```
BOOL writeToErrorWithIterator(NSError *__autoreleasing* error, NSArray *a) { [a 
enumerateObjectsUsingBlock:^{
*error = [NSError errorWithDomain:1];
}];
}
```

and

```
BOOL writeToErrorInBlockFromCFunc(NSError *__autoreleasing* error) {
dispatch_semaphore_t sem = dispatch_semaphore_create(0l);
dispatch_async(queue, ^{
if (error) {
*error = [NSError errorWithDomain:1];
}
dispatch_semaphore_signal(sem);
});
 
dispatch_semaphore_wait(sem, 100);
  return 0;
}
```




Comment at: www/analyzer/available_checks.html:1071
+(ObjC)
+Model the APIs that are guaranteed to return a non-nil value.
+

Probably should be dropped.



Comment at: www/analyzer/available_checks.html:1119
+
+
+

Top of the checker file has a somewhat reasonable description:

// A checker for detecting leaks resulting from allocating temporary
// autoreleased objects before starting the main run loop.
//
// Checks for two antipatterns:
// 1. ObjCMessageExpr followed by [[NSRunLoop mainRunLoop] run] in the same
// autorelease pool.
// 2. ObjCMessageExpr followed by [[NSRunLoop mainRunLoop] run] in no
// autorelease pool.
//
// Any temporary objects autoreleased in code called in those expressions
// will not be deallocated until the program exits, and are effectively leaks.



https://reviews.llvm.org/D53069



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