gnustep / libobjc2

Objective-C runtime library intended for use with Clang.
http://www.gnustep.org/
MIT License
434 stars 118 forks source link

Add test variant testing GCC runtime compatibility #259

Closed qmfrederik closed 6 months ago

qmfrederik commented 9 months ago

Using -fobjc-runtime=gcc with clang will cause clang to use the GNU_ObjC_SEH personality, and link with __gnu_objc_personality_seh0. This allows testing of SEH exception handling in MinGW.

davidchisnall commented 9 months ago

[ Updating the branch to pick up fixes to the CI scripts. ]

davidchisnall commented 9 months ago

I don't really like this because ripping out all of the GCC-runtime-compat code is top of my to-do list for after the next release. There's discussion in Clang of removing support for all fragile ObjC ABIs, which would make this go away.

qmfrederik commented 9 months ago

The only way I know of to compile GNUstep code reliably on Windows is to use the fragile ObjC ABI (with e.g. GCC's objc runtime). If you use the 2.0 ABI, you need to use the lld linker, but my understanding is that that linker doesn't properly export symbols for instance variables. There's work in GNUstep to work around that limitation, but it isn't complete for libs-gui and libs-back.

It'd be helpful to have at least one release which supports compiling under MinGW and with the fragile ABI, and to have Clang support the fragile ObjC ABIs until we find a way for GNUstep to export the ivar symbols (either by workarounds in GNUstep or through a fix in ldd).

davidchisnall commented 9 months ago

Hmm, interesting, I haven't actually tried GNUstep on Windows (and, since I left MS, no longer have a Windows machine to test with). I'd personally regard accessing an ivar from another DLL as a bad idea, but I can imagine that there are cases where it's unavoidable (I believe WinObjC just didn't do this at all and used declared properties for anything where a subclass may need to access it in another DLL).

You shouldn't need to use lld. Last time I tested it, LINK.EXE worked as long as you disabled incremental linking (otherwise it added padding in the middle of data structures, which was unhelpful).

I think the problem is here: https://github.com/llvm/llvm-project/blob/c7c912cff945033918367c4a37121dfc09b9759e/clang/lib/CodeGen/CGObjCGNU.cpp#L1682

We should be setting the ivar offset variable as dllexport if the ivar is public and the class is dllexport (when targeting Windows). Does doing that fix the issue? If I remember correctly, PE/COFF allows you to refer to exported globals in code from other DLLs, just not in initialisation of globals, and that's all that we need to do here.

qmfrederik commented 9 months ago

We should be setting the ivar offset variable as dllexport if the ivar is public and the class is dllexport (when targeting Windows). Does doing that fix the issue?

If you link GNUstep on MinGW with the ld linker, it exports those symbols (e.g. __objc_ivar_offset_NSTask._internal) and that works just fine. I think we'd be fine if ldd does the same thing. As a short-term fix, is there any way I can force lld to export these symbols if I know their names upfront?

Thanks for the tip about link.exe, I'll try that in the coming days.

davidchisnall commented 9 months ago

If you link GNUstep on MinGW with the ld linker, it exports those symbols (e.g. __objc_ivar_offset_NSTask._internal) and that works just fine.

I believe this is because MinGW defaults to exporting all symbols.

I think we'd be fine if ldd does the same thing. As a short-term fix, is there any way I can force lld to export these symbols if I know their names upfront?

I believe lld is a drop-in replacement for LINK.EXE on Windows, so you should be able to list them in a .DEF file. The right fix is to make clang properly mark them as exported though.

qmfrederik commented 9 months ago

Another data point which may be relevant: most Linux distributions ship a copy of gnustep which is linked with the GCC libobjc implementation:

So if you are developing a GNUstep application on those platforms and use the distribution-provided copy of GNUstep, you’ll end up using the GCC objc runtime.

I’d love for those distributions to start packaging libobjc2 and use it as the default objc runtime, but that will take time.

davidchisnall commented 9 months ago

Most distros either require packages to be compiled with the default compiler (which is usually gcc) or simply do not care about GNUstep. We switched the GNUstep packages over to compiling with clang and using libobjc2 for the FreeBSD packages around ten years ago. Objective-C support has been unmaintained in GCC for longer than that.

triplef commented 9 months ago

Just chiming in here about link.exe: unfortunately using link.exe for Objective-C code does not seem to be working any more (even with incremental linking disabled) and causes crashes in objc_msgSend, at least when I tried last summer. This mailing list thread has some more info. I’m pretty sure it worked at one point a couple years ago, not sure what might have changed.

qmfrederik commented 9 months ago

Thanks for that reference, @triplef . I believe on of the most impactful things we can do to move GNUstep forward on the Windows platform, is to make sure lld properly exports symbols for instance variable offsets. Working around that limitation is tedious, at the least, and maintainers (rightfully) push back to those kinds of workarounds: https://github.com/gnustep/libs-gui/pull/177#issuecomment-1493385659

davidchisnall commented 9 months ago

I believe on of the most impactful things we can do to move GNUstep forward on the Windows platform, is to make sure lld properly exports symbols for instance variable offsets

I believe the clang change outlined earlier should fix that (setting the correct dllexport attribute on the ivar variables). This isn't a problem with lld, it's a problem with clang not marking the symbols as exported. Clang is also the only thing that knows which of them should be exported (ones for private / package ivars should not be).

triplef commented 9 months ago

I believe the clang change outlined earlier should fix that (setting the correct dllexport attribute on the ivar variables). This isn't a problem with lld, it's a problem with clang not marking the symbols as exported. Clang is also the only thing that knows which of them should be exported (ones for private / package ivars should not be).

Makes sense. @qmfrederik if you need help with making the change outlined above in LLVM I can put you in touch with @weliveindetail (about), who fixed a bunch of other LLVM issues for ObjC on Windows for us.

davidchisnall commented 9 months ago

It should be a simple change. You will need to do ->setDLLStorageClass(llvm::GlobalValue::DLLExportStorageClass) on the IVar offset variable where it's defined if the class is declared dllexport (Interface ->hasAttr<DLLExportAttr>()) and if the Ivar is public (Ivar->getAccessControl() == ObjCIvarDecl::Public).

I think you will also need to do a similar check for hasAttr<DLLImportAttr> on the class when introducing a reference to an ivar offset global and then set the DLL import storage class here:

https://github.com/llvm/llvm-project/blob/c7c912cff945033918367c4a37121dfc09b9759e/clang/lib/CodeGen/CGObjCGNU.cpp#L1844

qmfrederik commented 9 months ago

@davidchisnall Something like this: https://github.com/qmfrederik/llvm-project/commit/00e75676161076ec8d6395f28dd3b33f36465915 ? I'll try to get this tested later today.

davidchisnall commented 9 months ago

Yup. I don’t think you need the visibility check on the import, we shouldn’t get to codegen with IVars you can’t see.

qmfrederik commented 9 months ago

I tried that patch, and compiled a simple library:

#import "../objc/runtime.h"
#import "../objc/objc-arc.h"

 __declspec(dllexport)
@interface Visibility {
@public BOOL*testVar;
}
@end

@implementation Visibility
@end
cmake_minimum_required(VERSION 3.9)
project(Visibility)
add_library(visibility SHARED Visibility.m)
target_include_directories(visibility PRIVATE ../build/)
target_link_libraries(visibility objc)

I compiled with both vanilla clang 17 and clang 18 with the patch described above.

In both cases, the .o file does contain a symbol for __objc_ivar_offset_Visibility[_value].testVar, but the .dll.a file does not.

Here's the output for the library built with vanilla clang 17:

vagrant@DESKTOP-RNTVKUC UCRT64 ~/git/libobjc2/Visibility
# nm -g build17/CMakeFiles/visibility.dir/Visibility.m.obj | grep ivar
0000000000000004 B __objc_ivar_offset_value_Visibility.testVar
0000000000000028 D __objc_ivar_offset_Visibility.testVar

vagrant@DESKTOP-RNTVKUC UCRT64 ~/git/libobjc2/Visibility
# nm -g build17/libvisibility.dll.a

libvisibility_dll_d000003.o:
0000000000000000 I libvisibility_dll_iname

libvisibility_dll_d000000.o:
0000000000000000 I _head_libvisibility_dll
                 U libvisibility_dll_iname

libvisibility_dll_d000002.o:
0000000000000000 I __imp__OBJC_METACLASS_Visibility
0000000000000000 I __nm__OBJC_METACLASS_Visibility
                 U _head_libvisibility_dll

libvisibility_dll_d000001.o:
0000000000000000 I __imp__OBJC_CLASS_Visibility
0000000000000000 I __nm__OBJC_CLASS_Visibility
                 U _head_libvisibility_dll

and here the equivalent output for clang 18 with the patch:

vagrant@DESKTOP-RNTVKUC UCRT64 ~/git/libobjc2/Visibility
# nm -g build18/CMakeFiles/visibility.dir/Visibility.m.obj | grep ivar
0000000000000004 B __objc_ivar_offset_value_Visibility.testVar
0000000000000028 D __objc_ivar_offset_Visibility.testVar

vagrant@DESKTOP-RNTVKUC UCRT64 ~/git/libobjc2/Visibility
# nm -g build18/libvisibility.dll.a

libvisibility_dll_d000003.o:
0000000000000000 I libvisibility_dll_iname

libvisibility_dll_d000000.o:
0000000000000000 I _head_libvisibility_dll
                 U libvisibility_dll_iname

libvisibility_dll_d000002.o:
0000000000000000 I __imp__OBJC_METACLASS_Visibility
0000000000000000 I __nm__OBJC_METACLASS_Visibility
                 U _head_libvisibility_dll

libvisibility_dll_d000001.o:
0000000000000000 I __imp__OBJC_CLASS_Visibility
0000000000000000 I __nm__OBJC_CLASS_Visibility
                 U _head_libvisibility_dll
davidchisnall commented 9 months ago

I am not sure what the output of nm means on Windows. What does dumpbin /exports show?

qmfrederik commented 9 months ago

Here's the dumpbin output:

clang 18:

``` C:\tools\msys64\home\vagrant\git\libobjc2\Visibility>dumpbin /exports build18\libvisibility.dll Microsoft (R) COFF/PE Dumper Version 14.38.33133.0 Copyright (C) Microsoft Corporation. All rights reserved. Dump of file build18\libvisibility.dll File Type: DLL Section contains the following exports for libvisibility.dll 00000000 characteristics 6598FC9E time date stamp Fri Jan 5 23:09:18 2024 0.00 version 1 ordinal base 2 number of functions 2 number of names ordinal hint RVA name 1 0 000030C8 _OBJC_CLASS_Visibility 2 1 00003040 _OBJC_METACLASS_Visibility Summary 1000 .CRT 1000 .bss 1000 .data 2000 .debug_abbrev 1000 .debug_aranges 1000 .debug_frame 9000 .debug_info 2000 .debug_line 1000 .debug_line_str 2000 .debug_loclists 1000 .debug_rnglists 1000 .debug_str 1000 .edata 1000 .idata 1000 .pdata 1000 .rdata 1000 .reloc 2000 .text 1000 .tls 1000 .xdata ```

clang 17:

``` C:\tools\msys64\home\vagrant\git\libobjc2\Visibility>dumpbin /exports build17\libvisibility.dll Microsoft (R) COFF/PE Dumper Version 14.38.33133.0 Copyright (C) Microsoft Corporation. All rights reserved. Dump of file build17\libvisibility.dll File Type: DLL Section contains the following exports for libvisibility.dll 00000000 characteristics 6598FCC4 time date stamp Fri Jan 5 23:09:56 2024 0.00 version 1 ordinal base 2 number of functions 2 number of names ordinal hint RVA name 1 0 000030C8 _OBJC_CLASS_Visibility 2 1 00003040 _OBJC_METACLASS_Visibility Summary 1000 .CRT 1000 .bss 1000 .data 2000 .debug_abbrev 1000 .debug_aranges 1000 .debug_frame 9000 .debug_info 2000 .debug_line 1000 .debug_line_str 2000 .debug_loclists 1000 .debug_rnglists 1000 .debug_str 1000 .edata 1000 .idata 1000 .pdata 1000 .rdata 1000 .reloc 2000 .text 1000 .tls 1000 .xdata ```
davidchisnall commented 9 months ago

Oh, I see the problem! On line 1689, you're modifying the AST, not the emitted global. This should be setting the DLL storage class on IvarOffsetPointer, not Ivar.

qmfrederik commented 9 months ago

Hmm, looks I already made that change: https://github.com/llvm/llvm-project/commit/ffaa2382b1fbf19b3df42e5c5ba8f2c54c2bca3e and the result appears to be the same. I also uncommented the if(...) clause and left the statement in there as is, without any change.

davidchisnall commented 9 months ago

It shouldn't make a difference, but can you move the code setting up the global into the if block above where it's created (line 1681)?

Can you also dump the IR and make sure that the dllexport attribute is actually added to the ivar global?

qmfrederik commented 9 months ago

@davidchisnall It looks like https://github.com/llvm/llvm-project/commit/958455749a899edd99ddb1ca1834d4956775d305 did the trick. Does this look good to you? If so, I can open a PR in the llvm-project repo.

qmfrederik commented 9 months ago

... it also helped to specify -fobjc-runtime=gnustep-2.0 🤦 . Here's the dumpbin output for confirmation:

clang 18:

``` Microsoft (R) COFF/PE Dumper Version 14.38.33133.0 Copyright (C) Microsoft Corporation. All rights reserved. Dump of file libvisibility.dll File Type: DLL Section contains the following exports for libvisibility.dll 00000000 characteristics 659C621E time date stamp Mon Jan 8 12:59:10 2024 0.00 version 1 ordinal base 3 number of functions 3 number of names ordinal hint RVA name 1 0 000030C0 $_OBJC_CLASS_Visibility 2 1 00004000 $_OBJC_REF_CLASS_Visibility 3 2 00008020 __objc_ivar_offset_Visibility.testVar.^i Summary 1000 .CRT 1000 .bss 1000 .data 2000 .debug_abbrev 1000 .debug_aranges 1000 .debug_frame 8000 .debug_info 2000 .debug_line 1000 .debug_line_str 2000 .debug_loclists 1000 .debug_rnglists 1000 .debug_str 1000 .edata 1000 .idata 1000 .objcrt 1000 .pdata 1000 .rdata 1000 .reloc 2000 .text 1000 .tls 1000 .xdata ```

clang 17:

``` Microsoft (R) COFF/PE Dumper Version 14.38.33133.0 Copyright (C) Microsoft Corporation. All rights reserved. Dump of file libvisibility.dll File Type: DLL Section contains the following exports for libvisibility.dll 00000000 characteristics 659C62F4 time date stamp Mon Jan 8 13:02:44 2024 0.00 version 1 ordinal base 2 number of functions 2 number of names ordinal hint RVA name 1 0 000030C0 $_OBJC_CLASS_Visibility 2 1 00004000 $_OBJC_REF_CLASS_Visibility Summary 1000 .CRT 1000 .bss 1000 .data 2000 .debug_abbrev 1000 .debug_aranges 1000 .debug_frame 9000 .debug_info 2000 .debug_line 1000 .debug_line_str 2000 .debug_loclists 1000 .debug_rnglists 1000 .debug_str 1000 .edata 1000 .idata 1000 .objcrt 1000 .pdata 1000 .rdata 1000 .reloc 2000 .text 1000 .tls 1000 .xdata ```
davidchisnall commented 9 months ago

Clang changes look good, please raise a PR and tag me to review.

qmfrederik commented 8 months ago

With that PR merged, I now got here: https://lists.nongnu.org/archive/html/gnustep-dev/2021-10/msg00030.html

ld.lld: error: <root>: undefined symbol: Q^?LL}
ld.lld: error: <root>: undefined symbol: }
ld.lld: error: <root>: undefined symbol: ^{autorelease_array_list}II[0☺]}
ld.lld: error: <root>: undefined symbol: ^{_NSZone}QQ^{_GSIMapBucket}^{_GSIMapNode}Q^^{_GSIMapNode}Q}
ld.lld: error: <root>: undefined symbol: ^?^?^?^?^?^?^?Q☺^{_NSZone}}
ld.lld: error: <root>: undefined symbol: ^{_NSNotificationQueueRegistration}^{_NSNotificationQueueRegistration}}
ld.lld: error: <root>: undefined symbol: ☺I^☺ii}
ld.lld: error: <root>: undefined symbol: [16{_SETJMP_FLOAT128=[2Q]}]^{_NSHandler}☺}
ld.lld: error: <root>: undefined symbol: ☺☺{?=b0I1b1I31}}
davidchisnall commented 8 months ago

I think the linked post has the solution. Does that work?

qmfrederik commented 8 months ago

Yes, that's on my todo list. I'd like to get https://github.com/llvm/llvm-project/pull/77255 and https://github.com/gnustep/libobjc2/pull/267 merged first before starting something new. Also, using the GNU ABI acts as a (short-term) workaround; here's the make check result for libs-base with the latest libobjc2 on mingw:

   6343 Passed tests
     52 Failed tests
     25 Dashed hopes
     10 Skipped sets
      9 Failed files
qmfrederik commented 8 months ago

@davidchisnall This behavior seems to be specific to lld. Consider this file:

#define GS_EXPORT_CLASS __declspec(dllexport) 

typedef struct autorelease_array_list
{
  struct autorelease_array_list *next;
  unsigned size;
  unsigned count;
  __unsafe_unretained id objects[0];
} array_list_struct;

GS_EXPORT_CLASS
@interface NSAutoreleasePool
{
  struct autorelease_array_list *_released;
}
@end

@implementation NSAutoreleasePool
@end

You can compile and link this to a shared with ld:

# clang -shared -o test.dll test.m -lobjc -fobjc-runtime=gnustep-2.0
test.m:12:12: warning: class 'NSAutoreleasePool' defined without specifying a base class [-Wobjc-root-class]
   12 | @interface NSAutoreleasePool
      |            ^
test.m:12:29: note: add a super class to fix this problem
   12 | @interface NSAutoreleasePool
      |                             ^
1 warning generated.

and the ivar symbol is exported correctly:

# nm -g test.dll | grep ivar
00000001eabc8020 B __objc_ivar_offset_NSAutoreleasePool._released.^{autorelease_array_list=^{autorelease_array_list}II[0]}

But you'll get an error with lld:

# clang -shared -o test.dll test.m -lobjc -fobjc-runtime=gnustep-2.0 -fuse-ld=lld
test.m:12:12: warning: class 'NSAutoreleasePool' defined without specifying a base class [-Wobjc-root-class]
   12 | @interface NSAutoreleasePool
      |            ^
test.m:12:29: note: add a super class to fix this problem
   12 | @interface NSAutoreleasePool
      |                             ^
1 warning generated.
ld.lld: error: <root>: undefined symbol: ^{autorelease_array_list}II[0☺]}
clang: error: linker command failed with exit code 1 (use -v to see invocation)
davidchisnall commented 8 months ago

Probably easier to work around than fix, if we just turn = into \2 in ivar encodings on Windows.

qmfrederik commented 8 months ago

Yes, I'll give that a try: https://github.com/llvm/llvm-project/commit/c045bbf83d254ba4ed2f42dbcb5e0a454d3ae816. clang is rebuilding (sigh), will let you know once that's done. (Looks like this is known to work: https://lists.nongnu.org/archive/html/gnustep-dev/2021-11/msg00000.html)

@triplef, how did you work around this for Windows MSVC?

triplef commented 8 months ago

If you mean work around the linker errors with libs-gui: I didn’t – I gave up on building libs-gui on Windows back then since the issues were too involved to justify spending more time on it while we don’t need it ourselves (we use Qt/QML for our UI on Windows and Android).

qmfrederik commented 8 months ago

@triplef Well, I'm seeing this linker issue when building and linking libs-base, so I was wondering whether you had to do something special to make this work with clang-cl? Did you apply patches to libs-base to work around this (e.g. don't expose any ivars which are of a struct type), or use a different linker?

qmfrederik commented 8 months ago

@davidchisnall Yes, the patch proposed in that mail thread works. I can now compile and link libs-base on MinGW using the gnustep 2.0 ABI, using this patch: https://github.com/llvm/llvm-project/commit/c65ae76326040369bcd3a85678ab76e402140036. 🎉

So it looks like clang should be in a good shape if we can get that change upstreamed. I've got a couple of questions:

Once we've got some clarity on how we want to implement this, I can go ahead and create a PR.

qmfrederik commented 8 months ago

Update: libs-gui compiles and links, though libgmodel still fails. That'll be for another day.

triplef commented 8 months ago

@triplef Well, I'm seeing this linker issue when building and linking libs-base, so I was wondering whether you had to do something special to make this work with clang-cl?

I don’t think so. We’re using the LLD linker and we don’t apply any patches in that regard to libs-base. I guess the codegen must be somehow different with MinGW?

davidchisnall commented 8 months ago

Thanks. We do want to do it in all of those places. It’s worth pulling it out into a separate function called makeValidSybolName or similar. The @ replacement should be necessary only on ELF platforms (it conflicts with symbol versioning). The = replacement should not be needed anywhere other than Windows.

I think it’s probably okay as an ABI break on Windows. The ivar variables are the only ones where it matters (the others will simply do less uniquing). If we were not able to link with the old mangling then the impact should be low.

qmfrederik commented 8 months ago

https://github.com/llvm/llvm-project/pull/77797 should implement this

qmfrederik commented 6 months ago

We now have good support for Objective C on MSYS2 using Clang 18.1.0, so I'm closing this for now.