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

Tue Sep 28 14:46:15 PDT 2021

vsapsai added a comment.

In D110280#3016911 <https://reviews.llvm.org/D110280#3016911>, @rjmccall wrote:

> 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.

I've experimented with RecordDecl and always_inline functions. Just "inline" is inlined in LLVM but not in Clang, and in IR we have a function declaration and its call, so I don't know where to look for potential IRGen problems for MemberExpr. Anyway, with always_inline function we can have MemberExpr with MemberDecl from a "wrong" module. `FieldDecl` parent is from the "right" module because we've merged decl contexts, so `CodeGenTypes::getCGRecordLayout` receives correct RecordDecl. Later, when we want to get offset for `FieldDecl` from "wrong" module, we are saved by `getCanonicalDecl` (in getLLVMFieldNo <https://github.com/llvm/llvm-project/blob/1ecb1bc3e214c8da53a7fda14f428786441109d7/clang/lib/CodeGen/CGRecordLayout.h#L198> and getBitFieldInfo <https://github.com/llvm/llvm-project/blob/1ecb1bc3e214c8da53a7fda14f428786441109d7/clang/lib/CodeGen/CGRecordLayout.h#L217>, for instance) that returns FieldDecl from the "right" module.

I wasn't able to reproduce the exact scenario as in this PR because need to access an ivar from a method and I don't think you can inline that. As for test-functions.m, the test wasn't failing earlier because of a different name lookup code path and I've added it for completeness. I'll add always_inline function accessing ivars to the test, it doesn't hurt.

In D110280#3016911 <https://reviews.llvm.org/D110280#3016911>, @rjmccall wrote:

> 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.

Unfortunately, we were doing lookup in the canonical definition but `ASTReader::FindExternalVisibleDeclsByName` found decls from both modules because we've merged decl contexts and lookup tables from both modules were available in `ASTReader.Lookups`. I was considering other options for calling `ObjCIvarDecl::getCanonicalDecl` and based on the stack trace

  ASTReader::FindExternalVisibleDeclsByName
  DeclContext::lookup
  ObjCContainerDecl::getIvarDecl
  ObjCInterfaceDecl::lookupInstanceVariable
  Sema::LookupIvarInObjCMethod

decided `ObjCContainerDecl::getIvarDecl` would be the best option as this is the earliest opportunity where we have to use a single ObjCIvarDecl instead of multiple options.

As for teaching record-layout code to properly handle non-canonical ivars, I don't know how involved it is going to be. In https://reviews.llvm.org/D106994 Richard also suggested adding support for "compatible types". Cannot really tell the long-term impact of different approaches but based on my limited experience, I still [naively] believe that carefully™ using correct decls is a viable solution.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110280