Set dllimport on Objective C ivar offsets

[qmfrederik]

Ensures that offsets for instance variables are marked with dllimport if the interface to which they belong has this attribute.

[llvmbot]

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Frederik Carlier (qmfrederik)

Changes

Ensures that offsets for instance variables are marked with dllimport if the interface to which they belong has this attribute.

---

Full diff: https://github.com/llvm/llvm-project/pull/107604.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CGObjCGNU.cpp (+8-3)
• (modified) clang/test/CodeGenObjC/dllstorage.m (+1-1)

diff --git a/clang/lib/CodeGen/CGObjCGNU.cpp b/clang/lib/CodeGen/CGObjCGNU.cpp
index adc7cdbfded880..c78a3ab9830a1c 100644
--- a/clang/lib/CodeGen/CGObjCGNU.cpp
+++ b/clang/lib/CodeGen/CGObjCGNU.cpp
@@ -1698,12 +1698,17 @@ class CGObjCGNUstep2 : public CGObjCGNUstep {
   }
   llvm::Value *EmitIvarOffset(CodeGenFunction &CGF,
                               const ObjCInterfaceDecl *Interface,
-                              const ObjCIvarDecl *Ivar) override {
-    const std::string Name = GetIVarOffsetVariableName(Ivar->getContainingInterface(), Ivar);
+                              const ObjCIvarDecl *Ivar) override {                            
+    const ObjCInterfaceDecl *ContainingInterface = Ivar->getContainingInterface();
+    const std::string Name = GetIVarOffsetVariableName(ContainingInterface, Ivar);
     llvm::GlobalVariable *IvarOffsetPointer = TheModule.getNamedGlobal(Name);
-    if (!IvarOffsetPointer)
+    if (!IvarOffsetPointer) {
       IvarOffsetPointer = new llvm::GlobalVariable(TheModule, IntTy, false,
               llvm::GlobalValue::ExternalLinkage, nullptr, Name);
+      if (Ivar->getAccessControl() != ObjCIvarDecl::Private
+         && Ivar->getAccessControl() != ObjCIvarDecl::Package)
+        CGM.setGVProperties(IvarOffsetPointer, ContainingInterface);
+    }
     CharUnits Align = CGM.getIntAlign();
     llvm::Value *Offset =
         CGF.Builder.CreateAlignedLoad(IntTy, IvarOffsetPointer, Align);
diff --git a/clang/test/CodeGenObjC/dllstorage.m b/clang/test/CodeGenObjC/dllstorage.m
index c94f4c9b5804d0..0801fb049f6b45 100644
--- a/clang/test/CodeGenObjC/dllstorage.m
+++ b/clang/test/CodeGenObjC/dllstorage.m
@@ -151,7 +151,7 @@ id f(Q *q) {
 
 // CHECK-IR-DAG: @"OBJC_IVAR_$_M._ivar" = external dllimport global i32
 
-// CHECK-NF-DAG: @"__objc_ivar_offset_M._ivar.@" = external global i32
+// CHECK-NF-DAG: @"__objc_ivar_offset_M._ivar.@" = external dllimport global i32
 
 int g(void) {
   @autoreleasepool {

[qmfrederik]

@davidchisnall Would you mind reviewing this PR? This came up when trying to build gnustep-gui using the Windows-native version of LLVM/clang (i.e., not in MSYS).

The problem is that all the ivar references are missing the dllimport annotation. This is not a problem on MSYS2 because the linker detects this and will use pseudo relocations to correct for this. This does not happen on a 'native' Windows toolchain, though.

I can confirm that gnustep-gui builds after implementing this patch.

gnustep/tools-windows-msvc#36 (comment) has more information.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[compnerd]

What is no explicit access control specified treated as? I wonder if this should be explicitly checking for public or protected access controls instead.

[qmfrederik]

It appears to be a pattern in both this file and in CGObjCMac.cpp to consider an ivar hidden if the visibility is ObjCIvarDecl::Private and ObjCIvarDecl::Package and visible otherwise.

In Objective C, a scoping directive applies to all the instance variables listed after it, up to the next directive or the end of the list and unmarked instance variables are @protected. So I don't think we'd ever a hit case where getAccessControl() would return ObjCIvarDecl::None.

So I left it like this for the sake of consistency, but I'm not particularly wed to that opinion -- let me know if you prefer me to make the change!

[qmfrederik]

I think the added tests demonstrate that unmarked ivars are considered protected, so let me know if that resolves this comment?

[davidchisnall]

Looks good to me. It might be worth updating the test to make sure that one ivar is implicitly @protected if it isn’t already.

[qmfrederik]

I've added tests to ensure that unmarked ivars are considered protected -- which seems to be the case: cf93b21

[qmfrederik]

Thanks @davidchisnall and @compnerd . Let me know if there's anything else you need?

[qmfrederik]

@davidchisnall and @compnerd -- anything else you need before this can get merged?

[davidchisnall]

LGTM, do you need someone else to commit it for you?

[qmfrederik]

@davidchisnall Yes, I don't have merge permissions on this repo; so I'd need someone to merge it for me :-)