[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.


================
Comment at: lib/Analysis/ThreadSafetyCommon.cpp:280
     unsigned I = PV->getFunctionScopeIndex();
-
-    if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
-      // Substitute call arguments for references to function parameters
-      assert(I < Ctx->NumArgs);
-      return translate(Ctx->FunArgs[I], Ctx->Prev);
+    const auto *D = PV->getDeclContext();
+    if (Ctx && Ctx->FunArgs) {
----------------
aaron.ballman wrote:
> Please don't use `auto` when the type is not explicitly spelled out in the initialization.
It's a `DeclContext` as spelled in `getDeclContext`, but sure I can drop the `auto` nonetheless.


================
Comment at: lib/Analysis/ThreadSafetyCommon.cpp:282
+    if (Ctx && Ctx->FunArgs) {
+      const auto *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+      if (isa<FunctionDecl>(D)
----------------
aaron.ballman wrote:
> Same.
It's a `Decl` (that's canonical), but same.


================
Comment at: lib/Analysis/ThreadSafetyCommon.cpp:288
+        assert(I < Ctx->NumArgs);
+        return translate(Ctx->FunArgs[I], Ctx->Prev);
+      }
----------------
aaronpuchert wrote:
> Does your test run into this with an `ObjCMethodDecl`? I see how we run into the assignment to `VD` down below, but I don't understand how we could get here without annotating a method.
The problem is in the cast above:
```
(lldb) 
frame #5: clang::threadSafety::SExprBuilder::translateDeclRefExpr(this, DRE, Ctx) at ThreadSafetyCommon.cpp:280:9
   277 	  // Function parameters require substitution and/or renaming.
   278 	  if (const auto *PV = dyn_cast_or_null<ParmVarDecl>(VD)) {
   279 	    const auto *FD =
-> 280 	        cast<FunctionDecl>(PV->getDeclContext())->getCanonicalDecl();
   281 	    unsigned I = PV->getFunctionScopeIndex();
   282 	
   283 	    if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
```


Repository:
  rC Clang

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

https://reviews.llvm.org/D59523