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

[vsapsai]

Summary

When have ObjCInterfaceDecl with the same name in 2 different modules,
hitting the assertion

Assertion failed: (Index < RL->getFieldCount() && "Ivar is not inside record layout!"),
function lookupFieldBitOffset, file llvm-project/clang/lib/AST/RecordLayoutBuilder.cpp, line 3434.

on accessing an ivar inside a method. The assertion happens because
ivar belongs to one module while its containing interface belongs to
another module and then we fail to find the ivar inside the containing
interface. We already keep a single ObjCInterfaceDecl definition in
redecleration chain and in this case containing interface was correct.
The issue is with ObjCIvarDecl. IVar decl for IRGen is taken from
ObjCIvarRefExpr that is created in Sema::BuildIvarRefExpr using ivar
decl returned from Sema::LookupIvarInObjCMethod. And ivar lookup
returns a wrong decl because basically we take the first ObjCIvarDecl
found in ASTReader::FindExternalVisibleDeclsByName (called by
DeclContext::lookup). And in ASTReader.Lookups lookup table for a
wrong module comes first because ASTReader::finishPendingActions
processes PendingUpdateRecords in reverse order and the first
encountered ObjCIvarDecl will end up the last in ASTReader.Lookups.

Fix by merging ObjCIvarDecl from different modules correctly and by
using a canonical one in IRGen.

rdar://82854574

[vsapsai]

clang/lib/AST/DeclObjC.cpp

81–84 ↗

(On Diff #374319)

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

[rjmccall]

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.

clang/lib/AST/DeclObjC.cpp

81–84 ↗

(On Diff #374319)

I think this is fine given the specific constraints on this lookup and on well-formed @interface declarations.

clang/lib/Serialization/ASTReaderDecl.cpp

3354

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.

[vsapsai]

In 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 and getBitFieldInfo, 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, @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.

[vsapsai]

Rebase and add a test case with always_inline.

Interaction between always_inline function and testAccessBitField has
revealed that we cannot avoid having expressions referencing decls from "wrong"
module. So returning a canonical ivar in name lookup is not sufficient. Mimic
CGRecordLayout approach and use canonical ivar in
ASTContext::lookupFieldBitOffset, where we look up concrete layout data.

[vsapsai]

Adhere to clang-format requirements.

[vsapsai]

Are there any comments regarding this change?

[rjmccall]

Patch looks good to me now, thanks. I don't think always_inline should strictly be necessary vs. just static inline, because in either case the function definition containing the non-canonical ivar reference is emitted in the same IRGen, which is what's triggering the assertion. But it's all the same to the test case, and I have no objections to using always_inline.

[vsapsai]

Thanks for the review! And thanks for the insightful questions about inlined functions. Good to know static inline also should trigger the early inline and not only always_inline.

[SharonXu]

Hi @vsapsai, I checked out the apple stable/20211026 llvm-project branch at https://github.com/apple/llvm-project/tree/stable/20211026, and ran check-clang locally, but the tested added in the patch clang/test/Modules/merge-objc-interface.m failed with the error

clang: /home/sharonxu/local/apple_llvm_main/llvm-project/clang/lib/AST/RecordLayoutBuilder.cpp:3457: uint64_t clang::ASTContext::lookupFieldBitOffset(const clang::ObjCInterfaceDecl*, const clang::ObjCImplementationDecl*, const clang::ObjCIvarDecl*) const: Assertion `Index < RL->getFieldCount() && "Ivar is not inside record layout!"' failed.

Do you know what's going on? Any fix missing on the stable/20211026 branch?

Thanks!
Sharon

[vsapsai]

In D110280#3289431, @SharonXu wrote:

Hi @vsapsai, I checked out the apple stable/20211026 llvm-project branch at https://github.com/apple/llvm-project/tree/stable/20211026, and ran check-clang locally, but the tested added in the patch clang/test/Modules/merge-objc-interface.m failed with the error

clang: /home/sharonxu/local/apple_llvm_main/llvm-project/clang/lib/AST/RecordLayoutBuilder.cpp:3457: uint64_t clang::ASTContext::lookupFieldBitOffset(const clang::ObjCInterfaceDecl*, const clang::ObjCImplementationDecl*, const clang::ObjCIvarDecl*) const: Assertion `Index < RL->getFieldCount() && "Ivar is not inside record layout!"' failed.

Do you know what's going on? Any fix missing on the stable/20211026 branch?

Thanks!
Sharon

My understanding is that failure is caused by the missing https://reviews.llvm.org/D106994 If I'm not mistaken, it's not on next and stable/20211026 branches. Swift isn't happy with RecordDecl change, so I'm bringing it back by making https://reviews.llvm.org/D118855 change first (and relevant Swift changes). After that the plan is to re-land https://reviews.llvm.org/D106994 on stable/20211026. Meanwhile you can check if bringing it back fixes the tests locally.

clang/include/clang/AST/DeclObjC.h

public:
   /// interface the ivar is conceptually a part of in the case of synthesized
   /// ivars.
   const ObjCInterfaceDecl *getContainingInterface() const;
 
   ObjCIvarDecl *getNextIvar() { return NextIvar; }
   const ObjCIvarDecl *getNextIvar() const { return NextIvar; }
   void setNextIvar(ObjCIvarDecl *ivar) { NextIvar = ivar; }
 
+  ObjCIvarDecl *getCanonicalDecl() override {
+    return cast<ObjCIvarDecl>(FieldDecl::getCanonicalDecl());
+  }
+  const ObjCIvarDecl *getCanonicalDecl() const {
+    return const_cast<ObjCIvarDecl *>(this)->getCanonicalDecl();
+  }
+
   void setAccessControl(AccessControl ac) { DeclAccess = ac; }
 
   AccessControl getAccessControl() const { return AccessControl(DeclAccess); }
 
   AccessControl getCanonicalAccessControl() const {
     return DeclAccess == None ? Protected : AccessControl(DeclAccess);
   }
 

clang/lib/AST/RecordLayoutBuilder.cpp

uint64_t ASTContext::getFieldOffset(const ValueDecl *VD) const {
   }
 
   return OffsetInBits;
 }
 
 uint64_t ASTContext::lookupFieldBitOffset(const ObjCInterfaceDecl *OID,
                                           const ObjCImplementationDecl *ID,
                                           const ObjCIvarDecl *Ivar) const {
+  Ivar = Ivar->getCanonicalDecl();
   const ObjCInterfaceDecl *Container = Ivar->getContainingInterface();
 
   // FIXME: We should eliminate the need to have ObjCImplementationDecl passed
   // in here; it should never be necessary because that should be the lexical
   // decl context for the ivar.
 
   // If we know have an implementation (and the ivar is in it) then
   // look up in the implementation layout.

clang/lib/Serialization/ASTReaderDecl.cpp

DeclContext *ASTDeclReader::getPrimaryContextForMerging(ASTReader &Reader,
 
   if (auto *RD = dyn_cast<RecordDecl>(DC))
     return RD->getDefinition();
 
   if (auto *ED = dyn_cast<EnumDecl>(DC))
     return ED->getASTContext().getLangOpts().CPlusPlus? ED->getDefinition()
                                                       : nullptr;
 
+  if (auto *OID = dyn_cast<ObjCInterfaceDecl>(DC))
+    return OID->getDefinition();
+
   // We can see the TU here only if we have no Sema object. In that case,
   // there's no TU scope to look in, so using the DC alone is sufficient.
   if (auto *TU = dyn_cast<TranslationUnitDecl>(DC))
     return TU;
 
   return nullptr;
 }
 

clang/test/Modules/merge-objc-interface.m

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -F%t/Frameworks %t/test.m \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -F%t/Frameworks %t/test-functions.m \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Test a case when Objective-C interface ivars are present in two different modules.
+
+//--- Frameworks/Foundation.framework/Headers/Foundation.h
+@interface NSObject
+@end
+
+//--- Frameworks/Foundation.framework/Modules/module.modulemap
+framework module Foundation {
+  header "Foundation.h"
+  export *
+}
+
+//--- Frameworks/ObjCInterface.framework/Headers/ObjCInterface.h
+#import <Foundation/Foundation.h>
+@interface ObjCInterface : NSObject {
+@public
+  id _item;
+}
+@end
+
+@interface WithBitFields : NSObject {
+@public
+  int x: 3;
+  int y: 4;
+}
+@end
+
+//--- Frameworks/ObjCInterface.framework/Modules/module.modulemap
+framework module ObjCInterface {
+  header "ObjCInterface.h"
+  export *
+}
+
+//--- Frameworks/ObjCInterfaceCopy.framework/Headers/ObjCInterfaceCopy.h
+#import <Foundation/Foundation.h>
+@interface ObjCInterface : NSObject {
+@public
+  id _item;
+}
+@end
+
+@interface WithBitFields : NSObject {
+@public
+  int x: 3;
+  int y: 4;
+}
+@end
+
+// Inlined function present only in Copy.framework to make sure it uses decls from Copy module.
+__attribute__((always_inline)) void inlinedIVarAccessor(ObjCInterface *obj, WithBitFields *bitFields) {
+  obj->_item = 0;
+  bitFields->x = 0;
+}
+
+//--- Frameworks/ObjCInterfaceCopy.framework/Modules/module.modulemap
+framework module ObjCInterfaceCopy {
+  header "ObjCInterfaceCopy.h"
+  export *
+}
+
+//--- test.m
+#import <ObjCInterface/ObjCInterface.h>
+#import <ObjCInterfaceCopy/ObjCInterfaceCopy.h>
+
+@implementation ObjCInterface
+- (void)test:(id)item {
+  _item = item;
+}
+@end
+
+@implementation WithBitFields
+- (void)reset {
+  x = 0;
+  y = 0;
+}
+@end
+
+//--- test-functions.m
+#import <ObjCInterface/ObjCInterface.h>
+
+void testAccessIVar(ObjCInterface *obj, id item) {
+  obj->_item = item;
+}
+void testAccessBitField(WithBitFields *obj) {
+  obj->x = 0;
+}
+
+#import <ObjCInterfaceCopy/ObjCInterfaceCopy.h>
+
+void testAccessIVarLater(ObjCInterface *obj, id item) {
+  obj->_item = item;
+}
+void testAccessBitFieldLater(WithBitFields *obj) {
+  obj->y = 0;
+}
+void testInlinedFunction(ObjCInterface *obj, WithBitFields *bitFields) {
+  inlinedIVarAccessor(obj, bitFields);
+}