[PATCH] D110280: [modules] Fix IRGen assertion on accessing ObjC ivar inside a method.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Hmm.  It sounds like we misbehave if we're working with a non-canonical ivar.  While it does seem preferable in general for lookups done after merging modules to return the canonical ivar, I'm not sure we can rely on the AST only having references to that ivar, since e.g. there might be references to non-canonical ivars in static/inline functions in the modules that declare them.  So while this seems like a good change  in the abstract — although maybe we should just be doing the lookup in the canonical definition in the first place? — it also might not be enough, and the record-layout code might just need to properly handle non-canonical ivars.



================
Comment at: clang/lib/AST/DeclObjC.cpp:81-84
   lookup_result R = lookup(Id);
   for (lookup_iterator Ivar = R.begin(), IvarEnd = R.end();
        Ivar != IvarEnd; ++Ivar) {
+    if (auto *ivar = dyn_cast<ObjCIvarDecl>((*Ivar)->getCanonicalDecl()))
----------------
vsapsai wrote:
> Don't really know what are the common patterns for name lookup and if this approach is acceptable. For C we call `LookupResult::resolveKind` to deal with multiple decls (and to avoid ambiguous lookup errors) but that's not available for `lookup_result` (aka `DeclContextLookupResult`).
I think this is fine given the specific constraints on this lookup and on well-formed `@interface` declarations.


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3354
+  if (auto *OID = dyn_cast<ObjCInterfaceDecl>(DC))
+    return OID->getDefinition();
+
----------------
This is fine if this function is only ever called when there's actually a known member of the ID, which I  think is true.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110280