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

[ille via Phabricator via cfe-commits]

ille added inline comments.


================
Comment at: clang/lib/AST/Decl.cpp:2491
+bool VarDecl::isCapturedByOwnInit() const {
+  return hasAttr<BlocksAttr>() && NonParmVarDeclBits.CapturedByOwnInit;
+}
----------------
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.



================
Comment at: clang/lib/AST/Decl.cpp:2498
+  return isCapturedByOwnInit() &&
+         isa<RecordType>(getType().getAtomicUnqualifiedType());
+}
----------------
rjmccall wrote:
> You should use `->is<RecordType>()` instead of `isa`, since the latter doesn't handle e.g. typedefs.
> 
> The reference to TEK_Aggregate is inappropriate here; it's okay to just talk about "aggregates" under the idea that anything else could validly just be zero-initialized prior to the capture.
You mean `isRecordType()`?  Good catch, I'll add a test for that.


================
Comment at: clang/lib/Sema/Sema.cpp:1949
+  return nullptr;
+}
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90434