[analyzer] Support implicit parameters in path note

[jkorous]

Summary

showBRParamDiagnostics assumed stores happen only via function parameters while that can also happen via implicit parameters like 'self' or 'this'.
The regression test caused a failed assert in the original cast to ParmVarDecl.

rdar://99687986

[jkorous]

@NoQ I tried to tease out a note that would allow me to test 'this' as well but didn't find any such situation.
Shall I just remove that case?

[NoQ]

Thanks nice!!

That's right, this cannot be hit there because it's not represented by VarRegion (but by a CXXThisRegion). It's also a slightly different story because this, unlike self, cannot be reassigned mid-way.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1384

This looks like an llvm_unreachable() to me.

clang/test/Analysis/path-notes-impl-param.m

25

It's usually a good idea to write down these warnings even if they aren't being tested. We want the programmers to revisit the test every time they break at least one of the notes, and then visually confirm that the updated report makes sense *as a whole*.

[jkorous]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1384

Do I understand correctly that would crash in a release build?
https://llvm.org/doxygen/Support_2ErrorHandling_8h_source.html#l00143

Wouldn't not-printing any parameter-kind-specific message be better?

If we want to get awareness of these cases we could just use assert.

clang/test/Analysis/path-notes-impl-param.m

25

Got it! I aimed for the test to be specific and didn't realize the consistency aspect.

[jkorous]

Check for specific diagnostics in the test.

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1384

Aha yeah, I was thinking like, the usual thing where we handle all the cases and then have a compile-time warning if something else can happen, so unreachable will be verified at compile time and won't actually be hit on switch branches.

But I agree that listing all the cases isn't very productive in this case. So yeah, let's just assert that it's a self.

But now that you listed all the cases, we should probably talk about ObjCCmd (it's spelled as _cmd in any ObjC method and resolves to the selector of that method). The selector pointer in any stack frame is usually brand new, it doesn't make sense to track it into the outside world. So I suspect that we should assert that it can't be encountered here. Unless we called a function by variable selector?... I don't think we model that yet, and even if so, I can't think of a checker that would warn in such case (if that selector is nil or undefined, there's no stack frame to pop back from; if the selector is constrained to nil inside the current stack frame, there's no reason to track it further). So let's assert that this never happens.

[jkorous]

Remove mention of other implicit parameter kinds.

[jkorous]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1384

Shall we just make it an if checking for self pointer kind?

[NoQ]

Ok LGTM!

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1384

Either way is fine I guess. One way or another, if the message isn't good enough we'll probably find out.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

if (isa<loc::ConcreteInt>(SI.Value)) {
     }
   }
 }
 
 /// Display diagnostics for passing bad region as a parameter.
 static void showBRParamDiagnostics(llvm::raw_svector_ostream &OS,
                                    StoreInfo SI) {
   const auto *VR = cast<VarRegion>(SI.Dest);
-  const auto *Param = cast<ParmVarDecl>(VR->getDecl());
+  const auto *D = VR->getDecl();
 
   OS << "Passing ";
 
   if (isa<loc::ConcreteInt>(SI.Value)) {
-    OS << (isObjCPointer(Param) ? "nil object reference"
-                                : "null pointer value");
+    OS << (isObjCPointer(D) ? "nil object reference" : "null pointer value");
 
   } else if (SI.Value.isUndef()) {
     OS << "uninitialized value";
 
   } else if (auto CI = SI.Value.getAs<nonloc::ConcreteInt>()) {
     OS << "the value " << CI->getValue();
 
   } else if (SI.Origin && SI.Origin->canPrintPretty()) {
     SI.Origin->printPretty(OS);
 
   } else {
     OS << "value";
   }
 
+  if (const auto *Param = dyn_cast<ParmVarDecl>(VR->getDecl())) {
     // Printed parameter indexes are 1-based, not 0-based.
     unsigned Idx = Param->getFunctionScopeIndex() + 1;
     OS << " via " << Idx << llvm::getOrdinalSuffix(Idx) << " parameter";
     if (VR->canPrintPretty()) {
       OS << " ";
       VR->printPretty(OS);
     }
+  } else if (const auto *ImplParam = dyn_cast<ImplicitParamDecl>(D)) {
+    if (ImplParam->getParameterKind() ==
+        ImplicitParamDecl::ImplicitParamKind::ObjCSelf) {
+      OS << " via implicit parameter 'self'";
+    }
+  }
 }
 
 /// Show default diagnostics for storing bad region.
 static void showBRDefaultDiagnostics(llvm::raw_svector_ostream &OS,
                                      StoreInfo SI) {
   const bool HasSuffix = SI.Dest->canPrintPretty();
 
   if (isa<loc::ConcreteInt>(SI.Value)) {
     OS << (isObjCPointer(SI.Dest) ? "nil object reference stored"
                                   : (HasSuffix ? "Null pointer value stored"
                                                : "Storing null pointer value"));
 
   } else if (SI.Value.isUndef()) {
     OS << (HasSuffix ? "Uninitialized value stored"
                      : "Storing uninitialized value");
 

clang/test/Analysis/path-notes-impl-param.m

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,osx.cocoa.RetainCount -analyzer-output=text -verify %s
+
+@protocol NSObject
+@end
+
+@interface NSObject <NSObject> {}
+- (id)init;
++ (id)alloc;
+- (id)autorelease;
+@end
+
+@interface Foo : NSObject
+@property(nonatomic) int bar;
+@end
+
+@implementation Foo
+-(int)bar {
+  return 0;
+}
+@end
+
+int baz() {
+  Foo *f = [Foo alloc];
+  // expected-note@-1 {{'f' initialized here}}
+  // expected-note@-2 {{Method returns an instance of Foo with a +1 retain count}}
+
+  return f.bar;
+  // expected-warning@-1 {{Potential leak of an object stored into 'self' [osx.cocoa.RetainCount]}}
+  // expected-note@-2 {{Passing value via implicit parameter 'self'}}
+  // expected-note@-3 {{Object leaked: object allocated and stored into 'self' is not referenced later in this execution path and has a retain count of +1}}
+}