[PATCH] D90434: [CodeGen] Correct codegen for self-capturing __block var

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/AST/Decl.cpp:2491
+bool VarDecl::isCapturedByOwnInit() const {
+  return hasAttr<BlocksAttr>() && NonParmVarDeclBits.CapturedByOwnInit;
+}
----------------
ille wrote:
> rjmccall wrote:
> > You should check `isEscapingByref()` here rather than just `hasAttr`; if none of the capturing blocks are escaping, we don't need to worry about this.  Or are you handling this implicitly in your pass during escape-marking?  That seems subtle.
> On second thought, the `hasAttr<BlocksAttr>()` shouldn't be there; it's unnecessary.  (I based it on `isEscapingByref`, where it's also unnecessary - but it's needed in `isNonEscapingByref`, so I suppose `isEscapingByref` has it for symmetry.).  I should replace it with just `!isa<ParmVarDecl>(this)` to ensure that `NonParmVarDeclBits` is valid.  And the name `isCapturedByOwnInit` is also a misnomer; I should change it to something like `isByRefCapturedByOwnInit`.
> 
> I want the substantive conditions for `isCapturedByOwnInit` to be handled during escape marking, for two reasons.  One, that's where the warning is emitted, and I want to ensure it doesn't ever disagree with codegen about whether init-on-heap needs to be applied.  Two, it can't mean "is captured by own initializer regardless of __block", because that would imply unnecessarily running the capture check on all variables.
> 
Okay.  In that case, this can just go in the header, and it's worth documenting in comments (on either the accessor or the underlying bit) that it's only set on `__block` variables.


================
Comment at: clang/lib/Sema/Sema.cpp:1949
+  return nullptr;
+}
+
----------------
ille wrote:
> rjmccall wrote:
> > There is an EvaluatedExprVisitor class which might make this easier, and which will definitely eliminate some false positives.
> Hmm… In [an earlier revision by jfb to this same check](https://reviews.llvm.org/D47096?id=147614), you were concerned about using RecursiveASTVisitor because 'RecursiveASTVisitor instantiations are huge'.  EvaluatedExprVisitor inherits from a different class from RecursiveASTVisitor, but they look pretty similar, so doesn't the same concern about template bloat apply?  Perhaps I'm misunderstanding that earlier comment.
`StmtVisitor` instantiations don't come with the same code-size impact as `RecursiveASTVisitor`, mostly because they primarily depend on the same recursive `children()` walk rather than the explicit repeated logic and careful tracking of the latter, but also because they don't make an effort to recurse into various syntactic but unevaluated expressions, and because the main "heavyweight" thing in their instantiation (the switch in `Visit`) is pretty easily optimized by the compiler if you actually end up only caring about particular cases.  But also, `EvaluatedExprVisitor` is the right tool for this job because you only care about evaluated references to the variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90434