[PATCH] D78286: [analyzer] Track runtime types represented by Obj-C Class objects

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko marked 7 inline comments as done.
vsavchenko added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:113
+
+bool isObjCClassType(QualType Type) {
+  if (const auto *PointerType = dyn_cast<ObjCObjectPointerType>(Type)) {
----------------
NoQ wrote:
> `static`?
It is all inside of `anonymous` namespace, so no need in putting static here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:134
+  // 1. Class is written directly in the message:
+  // \code
+  //   [ActualClass classMethod];
----------------
NoQ wrote:
> These comments will never be parsed by doxygen, i don't think those `\code` thingies are needed here. Or is it some editor/IDE-specific thingies that i'm not educated about?
No, you are correct. It's more like a matter of habit and consistent look of the comment.  I would anyway prefer to somehow tell the reader that this is a snippet. It can be with a more or less familiar way of doing it.  (I've seen a good amount of doxygen-style comments for `static` functions in the project, I guess similar logic applies to those as well)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:193
+        // Types in Class objects can be ONLY Objective-C types
+        return {cast<ObjCObjectType>(DTI.getType())};
+      }
----------------
NoQ wrote:
> Why are we discarding `CanBeSubClassed` here?
Good point, we should propagate it further


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:198
+      // 'self' variable of the current class method.
+      if (ReceiverSVal == Message.getSelfSVal()) {
+        // In this case, we should return the type of the enclosing class
----------------
NoQ wrote:
> I believe this is pretty much always the case. At least whenever `getInstanceReceiver()` is available. Another exception seem to be when `ReceiverSVal` is an `UnknownVal` (in this case `self` is going to be `SymbolRegionValue` because it's never set in the Store), but that's it. I inferred this by looking at `ObjCMethodCall::getInitialStackFrameContents()`.
> 
> I think we should have used `getSelfSVal()` to begin with.
> I believe this is pretty much always the case.

I didn't quite get what you mean by that




================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:919
+
+      ReceiverRuntimeType.Type->getSuperClassType();
+      QualType ReceiverClassType(ReceiverRuntimeType.Type, 0);
----------------
NoQ wrote:
> I don't think this line of code does anything.
> 
> (time for some `LLVM_NODISCARD`???)
Whoops, that's true!


================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:1219-1226
+  // NOTE: This cache is essentially a "global" variable, but it
+  // only gets lazily created when we get here.  The value of the
+  // cache probably comes from it being global across ExprEngines,
+  // where the same queries may get issued.  If we are worried about
+  // concurrency, or possibly loading/unloading ASTs, etc., we may
+  // need to revisit this someday.  In terms of memory, this table
+  // stays around until clang quits, which also may be bad if we
----------------
NoQ wrote:
> Before i forget: Stuff that's shared across analyses usually lives in `AnalysisConsumer`. Cf. `FunctionSummaries`. This is the intended way of avoiding globals in such cases.
We can re-do it in a separate commit I guess. 


================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:1293
+          if (Receiver == getSelfSVal().getAsRegion()) {
             return RuntimeDefinition(findDefiningRedecl(E->getMethodDecl()));
+          }
----------------
NoQ wrote:
> Wait, how do we get a decl here that's anyhow relevant if the compiler doesn't even know that it's a class method?
Compiler knows it, but still marks it as an instance method (because technically `self` is an object).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78286