The Mojave Core Foundation merge. #1708

millenomi · 2018-09-26T20:41:03Z

Welcome to macOS Mojave and iOS 12. This patch merges improvements from the Core Foundation version found in these operating systems into Swift Foundation.

For more information on changes to clients of Core Foundation on these operating systems, see the Foundation release notes. In particular, macros for closed enumerations and CFMessagePort improvements apply to this codebase as well.

The changes in this merge mostly revolve around performance improvements. For example, type IDs are now constants and we can lay out constant objects' memory directly into CF instead of requiring dispatch_onces and allocations in __CFInitialize. We also audited which symbols are exported, picked more specific kinds of locks to improve performance in some situations, and improved the correctness of some code (e.g. CFURLComponents parsing; ensuring subclasses of NSData that override copy interact correctly with CFDataCreateCopy). Finally, we bring headers to closer parity with the macOS and iOS ones.

millenomi · 2018-09-26T20:41:16Z

@swift-ci please test

…e merge issue.

millenomi · 2018-09-26T21:02:15Z

If you're working on a port of Swift, or on ABI stability, I'd highly recommend you read the patch. In particular, @compnerd, @johnno1962, @Kaiede, @tinysun212 (for the Cygwin parts), @mikeash, would you like to take a look at this?

spevans · 2018-09-26T21:54:00Z

CoreFoundation/Locale.subproj/CFLocale.c


 static bool __CFLocaleGetMeasurementSystemForPreferences(CFTypeRef metricPref, CFTypeRef measurementPref, UMeasurementSystem *outMeasurementSystem) {
    if (metricPref || measurementPref) {
        if (metricPref == kCFBooleanTrue && measurementPref && CFEqual(measurementPref, _measurementUnitsInches)) {
-#if U_ICU_VERSION_MAJOR_NUM >= 55


Will removing this check break Ubuntu14 builds which I think only has version 52?

Good catch.

millenomi · 2018-09-26T22:06:01Z

@swift-ci please test

parkera · 2018-09-26T23:12:59Z

CoreFoundation/Base.subproj/CFUtilities.c

@@ -564,13 +572,13 @@ CF_EXPORT void _CFGetUGIDs(uid_t *euid, gid_t *egid) {
    __CFGetUGIDs(euid, egid);
 }

-CF_EXPORT uid_t _CFGetEUID(void) {
+CF_EXPORT uid_t _CFGetEUID() {


Should we have lost the void here?

Nope. To fix.

parkera · 2018-09-26T23:13:41Z

CoreFoundation/Base.subproj/DarwinSymbolAliases

@@ -1,7 +1,7 @@


 # This is specific to Swift open source; Foundation's NSCFConstantString is used as our constant string class reference
-__TMC15SwiftFoundation19_NSCFConstantString ___CFConstantStringClassReference
+_$s15SwiftFoundation19_NSCFConstantStringCN ___CFConstantStringClassReference


Just a note here to double check this vs recent discussions about the mangling changing.

Yup; it's using $s, which is the new prefix. (The other class names in the source have been updated accordingly as well.)

parkera · 2018-09-26T23:16:44Z

CoreFoundation/Collections.subproj/CFData.c


    __CFDataSetNumBytesUsed(memory, 0);
    __CFDataSetLength(memory, 0);
-    __CFDataSetDontDeallocate(memory, isDontDeallocate);
+    __CFDataSetDontDeallocate(memory, true);


I think this is reverting an earlier change

Yup. To fix.

parkera · 2018-09-26T23:17:19Z

CoreFoundation/Collections.subproj/CFData.c

@@ -460,7 +460,7 @@ CFDataRef CFDataCreateCopy(CFAllocatorRef allocator, CFDataRef data) {
    Boolean allowRetain = true;
    if (allowRetain) {
        CF_OBJC_FUNCDISPATCHV(CFDataGetTypeID(), CFDataRef, (NSData *)data, copy);
-        CF_SWIFT_FUNCDISPATCHV(CFDataGetTypeID(), CFDataRef, (CFSwiftRef)data, NSObject.copyWithZone, NULL);
+        CF_SWIFT_FUNCDISPATCHV(CFDataGetTypeID(), CFDataRef, (CFSwiftRef)data, NSData.copy);


I think this may also be reverting an earlier change. Not sure.

There was at least one test failing because a NSData subclass overrode copy() instead of copy(with:).

(This brings it into consistency with ObjC, just above.)

parkera · 2018-09-26T23:17:43Z

CoreFoundation/Collections.subproj/CFStorage.c

@@ -242,7 +245,7 @@ static inline void __CFStorageSetChild(CFStorageNode *parentNode, CFIndex childI
    *((void **)&parentNode->info.notLeaf.child[childIndex]) = newChild;
 }

-static inline void __CFStorageGetChildren(const CFStorageNode *parent, CFStorageNode ** _CF_RESTRICT resultArray, bool shouldRetain, bool shouldFreeze) {
+static inline void __CFStorageGetChildren(const CFStorageNode *parent, CFStorageNode ** restrict resultArray, bool shouldRetain, bool shouldFreeze) {


This was recently added for C++ support so we need to use the macro.

I will make sure all mentions remain in the codebase.

parkera · 2018-09-26T23:20:05Z

CoreFoundation/NumberDate.subproj/CFTimeZone.c

@@ -696,50 +701,6 @@ static void __InitTZStrings(void) {
    }
    __CFUnlock(&__CFTZDirLock);
 }
-
-#elif DEPLOYMENT_TARGET_MACOSX || DEPLOYMENT_TARGET_EMBEDDED


We sure it's correct to remove this?

Tests pass, and it matches the original.

parkera · 2018-09-26T23:20:42Z

CoreFoundation/NumberDate.subproj/CFTimeZone.c

@@ -813,15 +769,6 @@ static CFTimeZoneRef __CFTimeZoneCreateSystem(void) {
        CFRelease(name);
        if (result) return result;
    }
-#if DEPLOYMENT_TARGET_ANDROID


I suspect most of the changes in CFTimeZone.c should probably prefer the SCL-F version.

Yeah. This bit absolutely needs to be preserved.

jakepetroules · 2018-09-26T23:29:32Z

CoreFoundation/Base.subproj/CFPlatform.c

-void *_CFReallocf(void *ptr, size_t size) {
-#if DEPLOYMENT_TARGET_WINDOWS | DEPLOYMENT_TARGET_LINUX
+CF_CROSS_PLATFORM_EXPORT void *_CFReallocf(void *ptr, size_t size) {
+#if TARGET_OS_WIN32 | DEPLOYMENT_TARGET_LINUX


Err... should be ||

Good catch.

uraimo · 2018-09-27T15:08:36Z

CoreFoundation/Base.subproj/CFSortFunctions.c

@@ -52,8 +52,6 @@ enum {

 #define __checkint_int64_mul(x, y, err)         (x * y)
 #define __checkint_uint64_add(x, y, err)        (x + y)
-#define __checkint_int32_mul(x,y,err)           (x * y)


Hi, while this is not required on 64bits platforms it should still be on arm32 and x86 (committed by me with #1396).

I'll fix it!

Kaiede · 2018-09-27T15:36:23Z

CoreFoundation/Base.subproj/CFInternal.h

+// On systems where we have ObjC support (DEPLOYMENT_RUNTIME_OBJC), STATIC_CLASS_REF(…) can statically produce a reference to the ObjC class symbol that ties into this particular type.
+// When compiling for Swift Foundation, STATIC_CLASS_REF returns a Swift class. There's a mapping of ObjC name classes to Swift symbols in the header that defines it that should be kept up to date if more constant objects are defined.
+// On all other platforms, it returns NULL, which is okay; we only need the type ID if CF is to be used by itself.
+#if __LP64__


This set of macros is a little weird for a couple reasons:

It is using a ULL in the 32-bit portion.

It assumes that Darwin and non-Darwin both use effectively “unsigned” as the cfinfoa type. Probably not fatal, but confusing to read.

Also curious why this was added here rather than where the other initialization macros are, and why do we need another set?

Out of curiosity, does anyone know if _CFInfo is only used on Linux (non-ObjC) targets? Seems like it is?

_cfinfoa is used on all targets. Its bits correspond to the ObjC object flag word semantics (please, runtime people, correct me if I'm wrong?), and it is used to store the refcount in ObjC and standalone (non-Swift) builds.

A few more points:

This set of macros is used for constant objects — it prevents us from having to do initialization of their fields at __CFInitialize time, which is processing time that does add up on Darwin or in the case of processes that potentially restart often (say, server-side).

_CFInfo is a bitfield on all platforms and it doesn't make sense for it to be signed.

Okay, so these macros add more parameters that INIT_CFRUNTIME_BASE in CFRuntime.h doesn't. That's fine. It's just a little hard to compare the two sets of macros that do incredibly similar things because they are in different files. But since this is for internal, I guess that's the point.

My comment about _cfinfoa is more that on 32-bit, you've got a ULL (64-bit) instead of a UL (32-bit) const.

As for _CFInfo, I bring it up because it's padded to 64-bits, at least on Linux, which makes macros like this... interesting.

mikeash · 2018-09-27T15:21:50Z

CoreFoundation/Base.subproj/CFInternal.h

+    #define CFAssert1(cond, prio, desc, a1) do { if (!(cond)) { CFLog(prio, CFSTR(desc), a1); /* HALT; */ } } while (0)
+    #define CFAssert2(cond, prio, desc, a1, a2) do { if (!(cond)) { CFLog(prio, CFSTR(desc), a1, a2); /* HALT; */ } } while (0)
+    #define CFAssert3(cond, prio, desc, a1, a2, a3) do { if (!(cond)) { CFLog(prio, CFSTR(desc), a1, a2, a3); /* HALT; */ } } while (0)
+    #define CFAssert4(cond, prio, desc, a1, a2, a3, a4) do { if (!(cond)) { CFLog(prio, CFSTR(desc), a1, a2, a3, a4); /* HALT; */ } } while (0)


What's the reason for making these not halt anymore? Seems like they ought to since these are for debug builds.

I'll research, and I'll uncomment if the reason doesn't apply to SCF.

mikeash · 2018-09-27T15:32:34Z

CoreFoundation/Base.subproj/CFSortFunctions.c

@@ -52,8 +52,6 @@ enum {

 #define __checkint_int64_mul(x, y, err)         (x * y)
 #define __checkint_uint64_add(x, y, err)        (x + y)


I'm sure these are only used in circumstances where it's fine, but the lack of parentheses around x and y makes me really paranoid.

I hear you; this needs a follow-up, I assume.

…anges.

phausler · 2018-09-26T22:29:49Z

CoreFoundation/Base.subproj/CFPlatform.c

 #if DEPLOYMENT_TARGET_MACOSX || DEPLOYMENT_TARGET_EMBEDDED || DEPLOYMENT_TARGET_EMBEDDED_MINI
    return pthread_getname_np(pthread_self(), buf, length);
-#elif DEPLOYMENT_TARGET_ANDROID


is this a regression for Android?

phausler · 2018-09-26T22:31:01Z

CoreFoundation/Base.subproj/CFRuntime.c

@@ -997,15 +1042,6 @@ static struct {
    {"CFNumberDisableCache", NULL},
    {"__CFPREFERENCES_AVOID_DAEMON", NULL},
    {"APPLE_FRAMEWORKS_ROOT", NULL},
-#if DEPLOYMENT_RUNTIME_SWIFT


this looks like a regression for XDG

Yep, that's incorrect.

phausler · 2018-09-26T22:32:06Z

CoreFoundation/Base.subproj/CFRuntime_Internal.h

+//  CFRuntime_Internal.h
+//  CoreFoundation
+//
+//  Created by Tony Parker on 4/21/18.


this should have an updated copyright header

…iled off ObjC OSes.

@uraimo

…ing of @uraimo's #1396).

millenomi · 2018-09-28T21:27:40Z

@swift-ci please test.

millenomi · 2018-09-28T21:27:46Z

@swift-ci please test macOS

millenomi · 2018-09-28T21:29:45Z

@shahmishal (is swift-ci working? it has responded only once to commands on this patch).

shahmishal · 2018-09-28T22:03:51Z

@swift-ci please test

millenomi · 2018-10-01T03:34:52Z

This will be merged Monday morning, if nothing else pops up in the meanwhile.

millenomi · 2018-10-01T16:52:00Z

I'm going to ask swift-ci to test and merge around 10.15 PST today.

millenomi · 2018-10-01T18:05:00Z

@swift-ci please test and merge

millenomi · 2018-10-01T19:50:21Z

@swift-ci please test and merge

millenomi · 2018-10-01T22:10:31Z

Seems like an upstream issue.

millenomi · 2018-10-01T22:10:38Z

@swift-ci please test and merge

millenomi · 2018-10-02T15:05:26Z

@swift-ci test and merge

millenomi · 2018-10-02T17:15:39Z

https://bugs.swift.org/browse/SR-8899 has been filed to track the blocker.

millenomi · 2018-10-03T23:32:46Z

Uses bool instead of _Bool to match the API surface in Core Foundation
You may notice we are moving some DEPLOYMENT_TARGET_* flags to TARGET_OS_*. In general, we will strongly prefer TARGET_OS_* going forward wherever possible. (DEPLOYMENT_RUNTIME_SWIFT and DEPLOYMENT_RUNTIME_OBJC remain.)
Changing some extern function(); to extern function(void); was premature for subtle source compatibility reasons.

millenomi · 2018-10-03T23:38:56Z

(on the off chance these changes actually happen to not trigger the blocker anymore.)

millenomi · 2018-10-03T23:55:08Z

@swift-ci please test

millenomi · 2018-10-04T01:06:41Z

Turns out: no.

…ilding SCF on iOS, it’s equivalent to TARGET_OS_MAC for Swift.

millenomi · 2018-10-04T21:55:49Z

@swift-ci please test and merge

drodriguez · 2018-10-04T21:00:15Z

CoreFoundation/Locale.subproj/CFDateFormatter.c

@@ -1517,7 +1524,7 @@ static UDate __CFDateFormatterCorrectTimeToARangeAroundCurrentDate(UCalendar *ca
        }
    } else {
        if (period < INT_MAX && futureMax > period) {
-            futureRange.location = 1;
+            futureRange.location = 1,


Is this correct?

The comma operator will discard the first operand (1) and evaluate the second operator. Both futureRange.location and futureRange.length will end up having the same value futureMax - period.

Oh. This must've reverted a fix inadvertently.

Just for the record, in case it's somehow relevant in the future, the comma operator has a lower precedence than the assignment operator so this code does work as expected with the comma, although it's definitely weird. It's equivalent to (futureRange.location = 1), (futureRange.length = futureMax - period);. This very occasionally has a practical use in for loops, where you can use it to do multiple assignments in the increment section of the for(), but mostly this tidbit is only useful for making strange code.

drodriguez · 2018-10-04T22:14:25Z

Foundation/NSData.swift

@@ -1138,3 +1138,7 @@ extension NSData : _StructTypeBridgeable {
        return Data._unconditionallyBridgeFromObjectiveC(self)
    }
 }
+
+internal func _CFSwiftDataCreateCopy(_ data: AnyObject) -> Unmanaged<AnyObject> {
+    return Unmanaged<AnyObject>.passRetained((data as! NSDictionary).copy() as! NSObject)


I don’t think as! NSDictionary is correct. Shouldn’t it be as! NSData?

A copy-and-paste error. I postponed adding tests due to the size, but this may merit one. Thanks!

millenomi · 2018-10-05T00:47:28Z

@swift-ci please test

compnerd · 2018-10-07T04:33:36Z

CoreFoundation/Base.subproj/CFPriv.h

+
+#if TARGET_OS_WIN32
+  // No C99 support
+  #define _CF_RESTRICT


We should conditional this over #if defined(_MSC_VER) && !defined(__clang__)

I’ll write a follow-up bug to do it tomorrow.

millenomi · 2018-10-07T04:41:01Z

@swift-ci please test

millenomi · 2018-10-09T20:48:47Z

@swift-ci please test

millenomi · 2018-10-10T01:03:23Z

@swift-ci please test and merge

but this was removed in swiftlang#1708. After interpreting the commit, this looks like it was renamed to _kCFRuntimeIDCFAllocator. Therefore, make this rename again.

PR swiftlang#2786 references symbol __kCFAllocatorTypeID_CONST in CFRuntime.c but this was removed in swiftlang#1708. After interpreting the commit, it looks like this symbol was renamed to _kCFRuntimeIDCFAllocator. Therefore, make this rename again.

PR swiftlang#2782 references symbol __kCFAllocatorTypeID_CONST in CFRuntime.c but this was removed in swiftlang#1708. After interpreting the commit, it looks like this symbol was renamed to _kCFRuntimeIDCFAllocator. Therefore, make this rename again.

Mojave merge.

480ceeb

_SwiftValue was renamed to __SwiftValue after the merge began. Fix th…

9d7c9f5

…e merge issue.

millenomi requested review from phausler and parkera September 26, 2018 20:52

johnno1962 mentioned this pull request Sep 26, 2018

Revert "https://bugs.swift.org/browse/SR-6812" #1693

Closed

jpsim mentioned this pull request Sep 26, 2018

[119] Issue #119 - October 4, 2018 SwiftWeekly/swiftweekly.github.io#413

Closed

spevans reviewed Sep 26, 2018

View reviewed changes

Un-revert ICU 55-or-previous compatibility fixes.

3d72955

parkera reviewed Sep 26, 2018

View reviewed changes

jakepetroules reviewed Sep 27, 2018

View reviewed changes

uraimo reviewed Sep 27, 2018

View reviewed changes

Kaiede reviewed Sep 27, 2018

View reviewed changes

mikeash approved these changes Sep 27, 2018

View reviewed changes

Restore CFTimeZone.c as it was, modulo the static class definition ch…

fa3991d

…anges.

phausler reviewed Sep 28, 2018

View reviewed changes

millenomi added 4 commits September 28, 2018 14:09

Ensure __CFEnv still reads XDG-related variables at startup when comp…

af0e80b

…iled off ObjC OSes.

Address the rest of @parkera’s feedback.

c4fc02a

Logical, not bitwise here.

ba09c4f

Reinstate __checkint_{u,}int32_mul in CFSortFunctions (prevent revert…

f76b5c7

…ing of @uraimo's #1396).

Add Swift license header to CFRuntime_Internal.h.

fb234f6

Some changes from Foundation team review. Details in a PR comment.

867a4b2

TARGET_OS_OSX was not defined. Since we (currently) do not support bu…

808d760

…ilding SCF on iOS, it’s equivalent to TARGET_OS_MAC for Swift.

drodriguez reviewed Oct 4, 2018

View reviewed changes

Correct feedback from @drodriguez.

a629eb3

compnerd reviewed Oct 7, 2018

View reviewed changes

swift-ci merged commit 05b5a05 into swiftlang:master Oct 10, 2018

3405691582 mentioned this pull request May 11, 2020

[CF] Rename remaining __kCFAllocatorTypeID_CONST. #2790

Merged

		@@ -52,8 +52,6 @@ enum {

		#define __checkint_int64_mul(x, y, err) (x * y)
		#define __checkint_uint64_add(x, y, err) (x + y)

The Mojave Core Foundation merge. #1708

The Mojave Core Foundation merge. #1708

Uh oh!

Conversation

millenomi commented Sep 26, 2018

Uh oh!

millenomi commented Sep 26, 2018

Uh oh!

millenomi commented Sep 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

millenomi commented Sep 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

millenomi commented Oct 3, 2018 •

edited

Loading