[PATCH] D131009: [analyzer] Fixing a bug raising false positives of stack block object leaking under ARC

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:313-315
+      // Under ARC, blocks are retained and released automatically:
+      if (isArcManagedBlock(Referred, Ctx))
+        return false;
----------------
ziqingluo-90 wrote:
> NoQ wrote:
> > Aha ok, it sounds like we can no longer be sure that the block is on the stack at this point, did I get it right?
> > 
> > In this case I think it's more productive to have the block's memory space be `UnknownSpaceRegion` from the start, so that it fell through the memory space check, both here and at other call sites of `isArcManagedBlock()` (so it can be removed), and in any other code that relies on memory spaces (so this mistake is never made again).
> //" ..., did I get it right?"//  Yes.
> 
> This suggestion makes sense to me.  To my understanding, I need to modify the symbolic execution engine to address it.  So shall I do it in a new patch?
There's no need to make a new review, you can update the current review. The test stays the same after all, who knows maybe we'll end up reverting to this solution. It's nice to have all approaches considered and explored behind the same link that ultimately ends up in the commit message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131009