gnustep / libobjc2

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

Various CMake cleanups and modernisations. #221

Closed davidchisnall closed 1 year ago

triplef commented 2 years ago

Thanks! I’ll give this a try with our Android and Windows toolchains (probably the second week of January after the holidays).

Looks like the Windows builds are still failing though (and we might also want to upgrade the CI to VS 2019 or 2022 according to the warning).

triplef commented 1 year ago

Talking to @weliveindetail today about extending libobjc2 tests (e.g. to cover additional EH edge cases that still have issues on Windows), we thought it would be helpful if the test suite also ran on Mac (excluding tests that cover libobjc2-specific behavior), in order to be able to compare and match behavior between the runtimes as much as possible. Do you think this would make sense @davidchisnall?

I’m adding this here, since this patch using native CMake ObjC support would probably be somewhat of a prerequisite for running the tests on Mac.

davidchisnall commented 1 year ago

That was always one of my goals. I used to run the tests manually on macOS.

triplef commented 1 year ago

Can you share how you ran them manually on macOS?

triplef commented 1 year ago

We’ve been looking into setting up CMake for our own projects and on Windows are running into the same CMake error mentioned above using CMake 3.24.2:

`CMAKE_OBJC_STANDARD_COMPUTED_DEFAULT` should be set for Clang

After looking at the source of the error message in CMake, I was able to work around this as follows:

foreach(lang IN ITEMS OBJC OBJCXX)
    set(CMAKE_${lang}_COMPILER_FORCED ON)
endforeach()

After this I got another error:

MSVC_RUNTIME_LIBRARY value 'MultiThreadedDebugDLL' not known for this OBJC compiler.

This can be worked around by resetting the corresponding runtime library option to an empty string (note that this must be done before the project() call, or before enable_language(OBJC)):

foreach(lang IN ITEMS OBJC OBJCXX)
    set(CMAKE_${lang}_COMPILER_FORCED ON)

    foreach(runtimeLibrary IN ITEMS MultiThreaded MultiThreadedDLL MultiThreadedDebug MultiThreadedDebugDLL)
        set(CMAKE_${lang}_COMPILE_OPTIONS_MSVC_RUNTIME_LIBRARY_${runtimeLibrary} "")
    endforeach()
endforeach()

While these workarounds seems to work fine, I was wondering if you might have any idea about what is missing for CMake on Windows to handle this correctly?

triplef commented 1 year ago

I think there’s another more general problem with having CMake detect the ObjC compiler: on non-Apple systems this might not work out of the box. For example on Android, I found the detection at least needs a couple compiler flags (e.g. -fobjc-runtime=gnustep-2.0) and linking libobjc2, which doesn’t exist yet at that point. I haven’t tried on Linux, but I would imagine it’s the same issue there?

I tried passing CMAKE_OBJC(XX)_COMPILER_FORCED=1, but at least on Android that leads to a bunch of linker errors when linking the tests (I’m guessing because it also disables some compiler/linker flags auto detection?).

What works for me is setting CMAKE_OBJC(XX)_COMPILER (or the OBJC/OBJCXX environment variables), as that skips the "check for working OBJC compiler" in CMake but seems to still set required flags. This is also why I hadn’t noticed this before, since our Android toolchain scripts set these environment variables.

However unfortunately the same trick doesn’t work on Windows, where I still get the error about CMAKE_OBJC_STANDARD_COMPUTED_DEFAULT unless I force the compiler and runtime library as shown above.

davidchisnall commented 1 year ago

It might be worth opening bugs with CMake. I suspect that they'd be willing to support more Objective-C platforms but without anyone on the CMake project using them they might not know that things are broken.

triplef commented 1 year ago

Makes sense, I’ll do that for the Windows error.

However I’m not sure about the second issue I wrote about, is it even possible to fix this in CMake? If there’s no runtime built yet, CMake couldn’t possible verify the ObjC compiler is working I think?

davidchisnall commented 1 year ago

However I’m not sure about the second issue I wrote about, is it even possible to fix this in CMake? If there’s no runtime built yet, CMake couldn’t possible verify the ObjC compiler is working I think?

I'm not sure. This was the problem that I had with depending on GNUstep Make originally: it expected that Objective-C projects would have a working Objective-C stack. For libcxxrt, I use a CMake flag that tells it to link as C, which avoids it adding the C++ libraries, but I don't know if that approach works for Objective-C.

davidchisnall commented 1 year ago

While these workarounds seems to work fine

I haven't been able to reproduce this, the Windows build is still failing for me. Can you push something to this branch with your fixes?

triplef commented 1 year ago

I’ve fixed the compiler detection on Windows. The COMPILER_FORCED option must be set before CMake configures the compiler, which happens as part of the project() call if the language is listed. So to work around this we need to remove OBJC(XX) from the project() call and then enable the language after forcing the compiler like so (setting these options before project() also didn’t work for me):

project(libobjc C ASM CXX)

# fix up CMake Objective-C compiler detection on Windows before enabling languages below
if (WIN32)
    foreach(lang IN ITEMS C CXX)
        set(CMAKE_OBJ${lang}_COMPILER_FORCED ON)
        foreach(runtimeLibrary IN ITEMS MultiThreaded MultiThreadedDLL MultiThreadedDebug MultiThreadedDebugDLL)
            set(CMAKE_OBJ${lang}_COMPILE_OPTIONS_MSVC_RUNTIME_LIBRARY_${runtimeLibrary} ${CMAKE_${lang}_COMPILE_OPTIONS_MSVC_RUNTIME_LIBRARY_${runtimeLibrary}})
        endforeach()
    endforeach()
endif()

enable_language(OBJC OBJCXX)

However there’s still a build error, I’m guessing because we need to add -fobjc-exceptions to CMAKE_OBJC(XX)_FLAGS?

associate.m(171,2): error: cannot use '@try' with Objective-C exceptions disabled
davidchisnall commented 1 year ago

Can you push the fix to this branch?

triplef commented 1 year ago

Yes already done. (I just thought I’d also post the solution above for others trying to use Objective-C with CMake on Windows.)

davidchisnall commented 1 year ago

However there’s still a build error, I’m guessing because we need to add -fobjc-exceptions to CMAKE_OBJC(XX)_FLAGS?

I think so, I'm a bit surprised it isn't in the default CMake flags. We already explicitly add -fexceptions.

triplef commented 1 year ago

CMake doesn’t seem to have any reference to -fobjc-exceptions. Is that part of the compiler defaults on other platforms maybe?

In any case, I added it in CMakeLists, but now a bunch of tests are failing and there are these compiler warnings:

clang-cl: warning: unknown argument ignored in clang-cl: '-x' [-Wunknown-argument]
clang-cl: warning: unknown argument ignored in clang-cl: '-std=gnu17' [-Wunknown-argument]
clang-cl: warning: objective-c: 'linker' input unused [-Wunused-command-line-argument]

If I build with clang.exe instead of clang-cl.exe (locally) I don’t get these warnings and all tests pass. Any idea what else we might need to do to make clang-cl happy?

weliveindetail commented 1 year ago

Just a note that you want to add -fobjc-arc-exceptions as well (if it's not added implicitly). See details here: https://reviews.llvm.org/D137939?id=475122#inline-1333144

davidchisnall commented 1 year ago

Not all of the Objective-C things are supported by default in clang-cl, because it aims for compatibility with cl.exe (which doesn't support Objective-C). You can pass clang arguments to the clang front end through the clang-cl driver with -Xclang -{argument}.

davidchisnall commented 1 year ago

The reason that I was using clang-cl, not clang, originally was that CMake conflated using-the-gcc-like-front-end with using-a-unix-compat-layer in a bunch of places. I think this was fixed around 3.19, so it's possible that we can just use clang.exe (which natively supports all of the Objective-C flags) instead now?

triplef commented 1 year ago

Yes that would be the easiest solution to just switch the CI to use clang instead of clang-cl – I’ve just pushed a change for that.

I also tried to compare clang-cl invocations between the master branch and this, and one difference is that on master the source file is passed as -c -- <source.m>, whereas on this branch the -- is missing. I’m not sure what this does but maybe this explains the 'linker' input unused warning?

davidchisnall commented 1 year ago

If we're not using clang-cl, we may not need the horrible CCC_OVERRIDE_OPTIONS hack. This uses a debug feature to rewrite the arguments because CMake was adding the cl.exe flags for C or C++ to the command line in a way that could not be changed.

The -- normally just indicates the end of flags, so won't treat files with flag-like names after it as flags. It shouldn't make a difference here.

davidchisnall commented 1 year ago

If you add -v to the ninja invocation in CI, it will show the complete command for each build step, which might help debugging.

davidchisnall commented 1 year ago

Should we still remove CCC_OVERRIDE_OPTIONS in main.yml and CMakeLists? Additionally we could output a warning if clang-cl is used.

Yup, I think both of those are good ideas.

triplef commented 1 year ago

I’ve done these updates.

Only downside I see with not supporting clang-cl is that I think it’s no longer possible to build with Visual Studio, as that doesn’t seem to support using clang according to this. Do you see an issue with that? If not I’ll update README.windows accordingly.

triplef commented 1 year ago

Just saw there was a test failure in the most recent run, but I don’t think it’s related to my changes:

88/90 Test #29: ManyManySelectors ........................Exit code 0xc0000409
***Exception:   1.59 sec
'345067selector543eb' != '345066selector543ea'
Assertion failed: strcmp(selBuffer, sel_getName(nextSel)) == 0, file D:/a/libobjc2/libobjc2/Test/ManyManySelectors.m, line 38
davidchisnall commented 1 year ago

That test seems to fail reliably on some of the QEMU things. I spent some time a couple of weeks ago trying to debug it. I strongly suspect a bug in my hash table code, so I plan on moving the selector table over to the tsl one soon to see if that fixes it.

davidchisnall commented 1 year ago

Only downside I see with not supporting clang-cl is that I think it’s no longer possible to build with Visual Studio, as that doesn’t seem to support using clang according to this. Do you see an issue with that? If not I’ll update README.windows accordingly

That's slightly unfortunate. I did debug the tests in Visual Studio a few times, though I eventually moved to WinDbg (time travel debugging is amazing for the test cases: I can run them until they crash and then set a watchpoint on the location of memory corruption and rewind them until I get to the point where the corruption happened). It sounds as if VS can drive CMake + Ninja, so that's probably fine.

triplef commented 1 year ago

Just FYI I believe even with these workarounds we put in place for Windows it still requires a newer CMake version than the current 3.16 minimum. I tried building with 3.20 and got the following error on Windows:

CMake Error at CMakeLists.txt:210 (add_library):
OBJC_STANDARD is set to invalid value ''

With CMake 3.24 it works, but I’m not sure which exact release fixes the issue. I just wanted to leave this here in case anyone else runs into this.