libs-base icon indicating copy to clipboard operation
libs-base copied to clipboard

Add +[NSObject _TrivialAllocInit] to enable fast-path alloc / init methods with libobjc2 2.2

Open triplef opened this issue 1 year ago • 4 comments

See https://github.com/gnustep/libobjc2/pull/268, requires the upcoming Clang 18 (but doesn't hurt when using older releases).

It’s not clear to me why _ARCCompliantRetainRelease was guarded by GS_ARC_COMPATIBLE instead of the more fine-grained OBJC_CAP_ARC. The latter seems to also be used in other places (like in NSAllocateObject) as a kind of general check for libobjc2 availability, so I used it here as well and removed the former as it’s no longer used. Let me know if that doesn’t make sense.

We should probably also update Make to default to libobjc2 2.2 instead of 2.0.

triplef avatar Feb 06 '24 09:02 triplef

Good point @rfm. I was going under the assumption that libobjc2 always supports ARC, but didn’t realize that in some cases GNUstep might not be compatible with it. I don’t fully understand the original commit https://github.com/gnustep/libs-base/commit/fe155240 adding this though, it’s pretty old so not sure if it’s still valid. @davidchisnall can you tell if this still makes sense?

Otherwise maybe it’s safest to keep the logic in place in order not to break some probably not very common platform.

As for defaulting to the 2.2 runtime in gnustep-make, that sounds good, but what is the best way for autoconf to test for the availability of the 2.2 runtime?

We could test for Clang 18 and whether objc_send_initialize is defined in runtime.h.

triplef avatar Feb 16 '24 15:02 triplef

I think unconditionally using 2.2 for the new ABI is fine, from a compiler perspective. If clang is too old then it will simply not use the new features, but they are not ABI breaks if you don't use them and so that's fine. You need only check that the runtime is the right one. A test that you can link a call to objc_send_initialize is a good one. I probably should have put something in a header. We have a few small fixes that probably merit a 2.2.1 release, if you want to add something that's easy for GNUstep Make to detect then I can roll it into the same release and you can just support the new things with 2.2.1 and ignore 2.2.0.

davidchisnall avatar Feb 16 '24 15:02 davidchisnall

Thanks. Should adding +_TrivialAllocInit here also be guarded by a check for the 2.2.1 runtime or can we just add it unconditionally?

And does the existing guarding for -_ARCCompliantRetainRelease still make sense (see https://github.com/gnustep/libs-base/commit/fe155240) or should it be ok to always have that if we detect libobjc2?

triplef avatar Feb 16 '24 15:02 triplef

As long as NSObject's +alloc just calls class_createInstance and its -init does nothing, it's safe to unconditionally implement +_TrivialAllocInit. _ARCCompliantRetainRelease requires that either NSObject does the same refcount manipulation as the runtime (including the handling of weak references), or simply delegates to the runtime.

davidchisnall avatar Feb 16 '24 15:02 davidchisnall

I spent a couple of hours looking at all the use of OBJC_CAP_ARC in the library, and as far as I can see (ei unless I missed something), when that is defined, we are always using the runtime method to allocate objects (and not using the referencecount/zone information in the memory before the isa pointer). That being so, the suggested patch looks good because we would only be implementing _TrivialAllocInit when we are not using the reference count mechanism supplied by the base library.

rfm avatar Apr 14 '24 09:04 rfm

Thanks @rfm!

@hmelder would you maybe be able to update the default runtime version in tools-make if you get a chance (see David’s comments above)?

triplef avatar Apr 15 '24 08:04 triplef

A test that you can link a call to objc_send_initialize is a good one. I probably should have put something in a header.

We are currently checking for __objc_load to be present.

hmelder avatar Apr 15 '24 11:04 hmelder

Thanks @rfm!

@hmelder would you maybe be able to update the default runtime version in tools-make if you get a chance (see David’s comments above)?

See https://github.com/gnustep/tools-make/pull/46

hmelder avatar Apr 15 '24 15:04 hmelder