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