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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
