The Catalina Merge #2782

millenomi · 2020-05-06T19:14:04Z

This patch brings swift-corelibs-foundation closer to behavior parity with released versions of macOS Catalina and aligned Apple releases. Most of these changes are under-the-hood bug fixes or performance improvements, but a few may affect the behavior of your applications. In particular:

The deprecation of NSURL.parameterString is now also effective in Swift for Linux, with similar effects. This brings URL parsing in line with Catalina. Note that Swift for Linux does not support link checks, since there is no binary compatibility guarantee on Linux; this change will be effective to all application code running on Swift for Linux 5.3 or later.
Certain CFXML… types that have been deprecated on macOS are now removed, along with their associated implementation: CFXMLInputStream, CFXMLNode, CFXMLParser. Modern code should be using the FoundationXML module (or, on Darwin, the XMLDocument and related classes in Foundation) to parse documents.

This patch brings swift-corelibs-foundation closer to behavior parity with released versions of macOS Catalina and aligned Apple releases. Most of these changes are under-the-hood bug fixes or performance improvements, but a few may affect the behavior of your applications. In particular: - The deprecation of NSURL.parameterString is now also effective in Swift for Linux, with similar effects. This brings URL parsing in line with Catalina. Note that Swift for Linux does not support link checks, since there is no binary compatibility guarantee on Linux; this change will be effective to all application code running on Swift for Linux 5.3 or later. - Certain CFXML… types that have been deprecated on macOS are now removed, along with their associated implementation: CFXMLInputStream, CFXMLNode, CFXMLParser. Modern code should be using the FoundationXML module (or, on Darwin, the XMLDocument and related classes in Foundation) to parse documents.

millenomi · 2020-05-06T19:14:13Z

@swift-ci please test

compnerd · 2020-05-06T19:58:56Z

https://dev.azure.com/compnerd/swift-build/_build/results?buildId=31292&view=results for a run on Windows

millenomi · 2020-05-06T21:16:24Z

@swift-ci please test

…te its own include.

millenomi · 2020-05-06T22:30:39Z

@swift-ci please test

xwu · 2020-05-06T23:44:39Z

CoreFoundation/Base.subproj/CFPlatform.c

-        return NULL;
-    }
-    return data->value;
+  return (CFTypeRef)FlsGetValue(key);


Suggested change

return (CFTypeRef)FlsGetValue(key);

return (CFTypeRef)FlsGetValue(key);

I've seen a few like this. Especially wrt the implementation of TSDs on Windows, @compnerd, let me know if it looks bad?

My mental map of The Whole Of Foundation gets a little murky around the Windows bits.

xwu · 2020-05-06T23:50:54Z

CoreFoundation/Base.subproj/ForFoundationOnly.h

        static bool warnonce = false;   \
        if (!warnonce) {                \
            warnonce = true;            \
            CFLog(kCFLogLevelWarning, CFSTR("*** %@: Range {%lu, %lu} out of bounds; string length %lu. This will become an exception for apps linked after 10.10 and iOS 8. Warning shown once per app execution."), __CFExceptionProem((id)self, _cmd), (unsigned long)range.location, (unsigned long)range.length, (unsigned long)len);        \
    }   \
-}
+} while(0)


Nit: the indentation is off here.

I'm gonna note these down and work on them later, for internal back-and-forth merging reasons.

millenomi · 2020-05-06T23:58:39Z

@swift-ci please test

millenomi · 2020-05-07T00:05:44Z

@swift-ci please test

millenomi · 2020-05-07T00:12:15Z

@swift-ci please test

xwu

Some more nits.

xwu · 2020-05-07T00:06:53Z

CoreFoundation/Parsing.subproj/CFBinaryPList.c

-	    CFDictionarySetValue(objects, (const void *)(uintptr_t)startOffset, *plist);
-	}
-	return (*plist) ? true : false;
+        if (outPlistTypeID) *outPlistTypeID = _kCFRuntimeIDCFNumber;


Nit: indentation appears off here and below.

xwu · 2020-05-07T00:07:20Z

CoreFoundation/Parsing.subproj/CFBinaryPList.c

-		CFDictionarySetValue(objects, (const void *)(uintptr_t)startOffset, *plist);
-	    }
-	    return (*plist) ? true : false;
+            if (outPlistTypeID) *outPlistTypeID = _kCFRuntimeIDCFNumber;


Nit: likewise indentation appears off here and below, and it appears pervasively throughout the rest of the file.

xwu · 2020-05-07T00:17:47Z

CoreFoundation/RunLoop.subproj/CFMessagePort.c

-	    }
-	} else {
-//#warning CF: should create a no-copy data here that has a custom VM-freeing allocator, and not vm_dealloc here
+            } else {


Nit: indentation consistency problem here also, it appears.

xwu · 2020-05-07T00:19:35Z

CoreFoundation/RunLoop.subproj/CFRunLoop.c

-
-    if (timers) CFRelease(timers);
-
+        if (timers) CFRelease(timers);


Suggested change

if (timers) CFRelease(timers);

if (timers) CFRelease(timers);

xwu · 2020-05-07T00:30:06Z

CoreFoundation/String.subproj/CFString.c

@@ -6420,7 +6495,7 @@ reswtch:switch (ch) {
 	    if (spec->size != CFFormatSize16) spec->size = CFFormatSize8;
 	    return true;
 	case 'n':		/* %n is not handled correctly; for Leopard or newer apps, we disable it further */
-	    spec->type = 1 ? CFFormatDummyPointerType : CFFormatPointerType;
+            spec->type = CFFormatDummyPointerType;


Suggested change

spec->type = CFFormatDummyPointerType;

spec->type = CFFormatDummyPointerType;

xwu · 2020-05-07T00:31:28Z

CoreFoundation/String.subproj/CFString.c

+//
+//                   👩             ❤    Var_Sel           💋             👨
+//   ┌──────────┐│┌───────┐┌─────┐┌──────┐┌──────┐┌─────┐┌───────┐┌─────┐┌───────┐│┌──────────┐
+//   │ stuff... │ │ 1F469 ││ ZWJ ││ 2764 ││ FE0F ││ ZWJ ││ 1F48B ││ ZWJ ││ 1F468 │ │ stuff... │


These diagrams are fantastic!

xwu · 2020-05-07T00:34:01Z

CoreFoundation/StringEncodings.subproj/CFStringEncodingConverter.c

@@ -607,6 +607,7 @@ static const _CFEncodingConverter *__CFGetConverter(uint32_t encoding) {
 #warning This case must match __defaultEncoding value defined in CFString.c
 	case kCFStringEncodingISOLatin1: commonConverterSlot = (const _CFEncodingConverter **)(&(commonConverters[1])); break;
 #endif
+        case kCFStringEncodingInvalidId: return NULL;


Nit: space-tab mismatch here.

xwu · 2020-05-07T00:35:41Z

CoreFoundation/URL.subproj/CFURL.c

@@ -3756,7 +3715,7 @@ static Boolean decomposeToRFC1808(CFURLRef url, CFURLComponentsRFC1808 *componen
    } else {
        components->port = kCFNotFound;
    }
-    components->parameterString = _retainedComponentString(url, HAS_PARAMETERS, false, false);
+        components->parameterString = NULL;


Suggested change

components->parameterString = NULL;

components->parameterString = NULL;

xwu · 2020-05-07T00:37:33Z

CoreFoundation/URL.subproj/CFURLAccess.c

@@ -629,7 +629,9 @@ static Boolean _CFDataURLCreateDataAndPropertiesFromResource(CFAllocatorRef allo
 	if (! parseDataRequestURL(url, &data, &mimeType, &textEncodingName)) {
 		if (errorCode)
 			*errorCode = kCFURLUnknownError;
-		*fetchedData = NULL;
+                if (fetchedData) {


Nit: space-tab mismatch here.

millenomi · 2020-05-07T01:20:02Z

@swift-ci please test

millenomi · 2020-05-07T01:30:23Z

@swift-ci please test

millenomi · 2020-05-07T08:23:19Z

The patch passes on Linux and Mac but breaks the Windows build; there will be some iteration.

lxbndr · 2020-05-07T11:53:54Z

This reverts #2764. I'm afraid NotificationQueue could be affected n Windows, as well as Operation/OperationQueue.

millenomi · 2020-05-07T14:57:55Z

Thank you for catching that. I’ll un-revert.

xwu · 2020-05-07T16:25:19Z

For ease of reviewing, would it be possible to commit the ICU upgrade separately? It would at least make the GitHub interface slightly more usable.

millenomi · 2020-05-07T17:12:54Z

I haven't tested the ICU upgrade with the previous version of the code, and I don't know that I want to put in the time to do that.

I understand this is a large patch, but extricating it would add more to what has been so far already ~1 work-hours-month-person of work that I need to coordinate with some expedition, and I'd rather not unless the need is really strong.

See <#2764>.

millenomi · 2020-05-07T17:19:03Z

(The ICU parts are showing collapsed for me? I'm not sure how their presence impacts usability. While you can review them, I'm not going to modify them: this is just a straight import of the headers generated by compiling Apple ICU from https://opensource.apple.com/source/ICU/ICU-64252.0.1/.)

millenomi · 2020-05-07T17:19:36Z

@swift-ci please test

xwu · 2020-05-07T17:29:04Z

@millenomi Yeah, I mention this thought precisely because there is no point in reviewing the ICU parts. Having not interfaced with the library extensively myself, I don't have a sense of its forward compatibility guarantees. If it's essentially a drop-in replacement, then perhaps there's some net benefit to doing that separately. If it's much more involved than that, then there's certainly no point in extricating it.

millenomi · 2020-05-07T20:20:44Z

@swift-ci please test

millenomi · 2020-05-07T20:54:48Z

@swift-ci please test

millenomi · 2020-05-07T21:31:25Z

I'm going to avoid starting tests when I'm doing Windows runs.

millenomi · 2020-05-07T21:31:53Z

https://dev.azure.com/compnerd/swift-build/_build/results?buildId=31349&view=results

…operands because the compiler runtime may not have an implementation.

millenomi · 2020-05-07T22:37:07Z

This one needs a test.

millenomi · 2020-05-07T22:38:50Z

@swift-ci please test

millenomi · 2020-05-07T22:44:44Z

https://dev.azure.com/compnerd/swift-build/_build/results?buildId=31352&view=logs&s=2bc04634-cf71-5486-fada-47995d15befc

millenomi · 2020-05-07T22:44:50Z

@swift-ci please test

…he double negation.

millenomi · 2020-05-07T22:46:29Z

@swift-ci please test

millenomi · 2020-05-07T22:47:01Z

https://dev.azure.com/compnerd/swift-build/_build/results?buildId=31353&view=results

millenomi · 2020-05-07T22:58:31Z

@swift-ci please test

millenomi · 2020-05-08T03:31:05Z

@swift-ci please test

millenomi · 2020-05-08T03:50:36Z

https://dev.azure.com/compnerd/swift-build/_build/results?buildId=31368&view=logs&s=2bc04634-cf71-5486-fada-47995d15befc

millenomi · 2020-05-08T15:02:12Z

This is going to be merged today barring last-minute objections.

xwu · 2020-05-08T15:27:40Z

Last minute objection: Unless I misread, I think your fix is still not quite right after the amendment. The wrapper function inverts the logic of os_mul_overflow, but on non-Win32 platforms is an alias of os_mul_overflow.

Never mind, saw the == 0 on scrolling right. Ignore me!

millenomi · 2020-05-08T15:42:53Z

Thanks for checking!

millenomi · 2020-05-08T20:17:59Z

Merging this.

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

finagolfin · 2020-05-12T06:37:36Z

Looks like this broke the linux CI, because of this line:

swift-corelibs-foundation/CoreFoundation/String.subproj/CFString.c:3552:100: error: use of undeclared identifier 'UCHAR_EMOJI_MODIFIER'

millenomi added 2 commits May 6, 2020 15:27

Win32 was missing the os_unfair_lock_t definition. Restore it.

76e7782

TARGET_OS_* is defined by TargetConditionals.h, so don't use it to ga…

d0f1385

…te its own include.

xwu reviewed May 6, 2020

View reviewed changes

Coordination mishap: these are defined just below for all platforms.

58a6638

xwu reviewed May 7, 2020

View reviewed changes

Fix incorrect calloc() invocation; remove stray undeclared call.

659b9b1

Un-revert the CF portions of #2764

284cd84

See <#2764>.

New CFStream code must use _CFThreadRef.

7013016

Remove non-existing header include.

acbe9c6

On Win32, experiment with limiting overflow multiplication to 32-bit …

9a0efdf

…operands because the compiler runtime may not have an implementation.

Amend the previous commit: the return value is now inverted, remove t…

2388adc

…he double negation.

millenomi merged commit 42bfad4 into apple:master May 8, 2020

This was referenced May 8, 2020

[5.3] The Catalina Merge #2786

Merged

[5.3][For real] The Catalina Merge #2787

Merged

3405691582 mentioned this pull request May 11, 2020

[CF] Rename remaining __kCFAllocatorTypeID_CONST. #2790

Merged

	return (CFTypeRef)FlsGetValue(key);
	return (CFTypeRef)FlsGetValue(key);


		if (timers) CFRelease(timers);

		if (timers) CFRelease(timers);

	spec->type = CFFormatDummyPointerType;
	spec->type = CFFormatDummyPointerType;

	components->parameterString = NULL;
	components->parameterString = NULL;

The Catalina Merge #2782

The Catalina Merge #2782

Conversation

millenomi commented May 6, 2020

millenomi commented May 6, 2020

compnerd commented May 6, 2020

millenomi commented May 6, 2020

millenomi commented May 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millenomi commented May 6, 2020

millenomi commented May 7, 2020

millenomi commented May 7, 2020

xwu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millenomi commented May 7, 2020

millenomi commented May 7, 2020

millenomi commented May 7, 2020

lxbndr commented May 7, 2020 • edited

millenomi commented May 7, 2020

xwu commented May 7, 2020

millenomi commented May 7, 2020

millenomi commented May 7, 2020

millenomi commented May 7, 2020

xwu commented May 7, 2020

millenomi commented May 7, 2020

millenomi commented May 7, 2020

millenomi commented May 7, 2020

millenomi commented May 7, 2020

millenomi commented May 7, 2020

millenomi commented May 7, 2020

millenomi commented May 7, 2020

millenomi commented May 7, 2020

millenomi commented May 7, 2020

millenomi commented May 7, 2020

millenomi commented May 7, 2020

millenomi commented May 8, 2020

millenomi commented May 8, 2020

millenomi commented May 8, 2020

xwu commented May 8, 2020 • edited

millenomi commented May 8, 2020

millenomi commented May 8, 2020

finagolfin commented May 12, 2020

lxbndr commented May 7, 2020 •

edited

xwu commented May 8, 2020 •

edited