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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:113
+
+bool isObjCClassType(QualType Type) {
+  if (const auto *PointerType = dyn_cast<ObjCObjectPointerType>(Type)) {
----------------
`static`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:134
+  // 1. Class is written directly in the message:
+  // \code
+  //   [ActualClass classMethod];
----------------
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?


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


================
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
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:919
+
+      ReceiverRuntimeType.Type->getSuperClassType();
+      QualType ReceiverClassType(ReceiverRuntimeType.Type, 0);
----------------
I don't think this line of code does anything.

(time for some `LLVM_NODISCARD`???)


================
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
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:1293
+          if (Receiver == getSelfSVal().getAsRegion()) {
             return RuntimeDefinition(findDefiningRedecl(E->getMethodDecl()));
+          }
----------------
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?


================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicType.cpp:85-87
+  if (const DynamicTypeInfo *DTI = State->get<DynamicClassObjectMap>(Sym))
+    return *DTI;
+  return {};
----------------
My favorite idiom for this stuff so far:
```lang=c++
  const DynamicTypeInfo *DTI = State->get<DynamicClassObjectMap>(Sym);
  return DTI ? *DTI : {};
```
(not sure `?:` will typecheck correctly in case of `{}` though)


================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicType.cpp:147
+static bool isLive(SymbolReaper &SR, const MemRegion *MR) {
+  return SR.isLiveRegion(MR);
+}
----------------
Feel free to rename `isLiveRegion` instead if it helps :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78286