Use `CFNumberCreate()` as factory initializer on `NSNumber` #1093

norio-nomura · 2017-07-02T14:31:35Z

This replaces #1025 by further utilizing the Factory Initializer Method (SR-5255).

Use `CFNumberCreate()` as factory initializer on `NSNumber.init(value:)`

Change NSNumber.init(value:) to returning instance created by CFNumberCreate()
Remove NSNumber._objCType

Change the properties of `NSNumber` to call `CFNumberGetValue ()` in the same way as `Foundation`.

Fixes SR-4907

Add TestNSNumber.test_objCType() reproducing crash
Expected results are sampled from macOS
Fix crash on accessing objCType property of NSNumber

Change `NSNumber.objCType` to returning same values with darwin platform version

Change NSNumber.objCType to returning same values with darwin
Change NSNumber.init(value: Bool) to returning __NSCFBoolean by using _Factory protocol

Since NSNumber.objCType returns c with both of Bool and Int8 on darwin platform, NSNumber.init(value: Bool) is changed to returns __NSCFBoolean by using _Factory protocol, and it changed to use _cfTypeID to distinguish between Bool and Int8.

Change JSONSerialization to use _cfTypeID for checking whether NSNumber is Boolean or not
Change _JSONDecoder to use _cfTypeID for checking whether NSNumber is Boolean or not
Change NSNumber.compare(_:) to use objCType
Add missing entry to TestNumber.allTests
Fix NSKeyedUnarchiver.decodeBool(forKey:) and add test to TestNSPredicate.test_NSCoding()

Expected results are sampled from macOS

…sing `_Factory` protocol

…SNumber` is Boolean

…edicate.test_NSCoding()`

phausler · 2017-07-02T16:10:13Z

Foundation/NSNumber.swift

@@ -198,14 +198,20 @@ extension NSNumber : ExpressibleByFloatLiteral, ExpressibleByIntegerLiteral, Exp

 }

+@_silgen_name("_CFNumberGetType2")


We really should not use silgen_name. Instead can you just put this in ForSwiftFoundationOnly.h?

What's the difference between ForSwiftFoundationOnly.h and ForFoundationOnly.h?
It seems _CFNumberInit*() functions are placed in ForFoundationOnly.h.
Should _CFNumberGetType2 also be placed in ForFoundationOnly.h?

ForFoundationOnly.h is used in both the CoreFoundation shipped on Darwin as well as swift-corlibs-foundation, whereas ForSwiftFoundationOnly.h is used for swift-corelibs-foundation exclusively.

Putting it in ForSwitFoundationOnly.h means that I can merge the closed source version and the open source version easier ;)

phausler · 2017-07-02T16:10:54Z

Foundation/NSNumber.swift

+    var high: Int64
+    var low: UInt64
+}
+
 open class NSNumber : NSValue {
    typealias CFType = CFNumber
    // This layout MUST be the same as CFNumber so that they are bridgeable
    private var _base = _CFInfo(typeID: CFNumberGetTypeID())


If we are using factories for all numbers then we can delete this!

These properties are necessary to maintain memory layout compatibility of NSDecimalNumber.

phausler · 2017-07-02T16:12:56Z

Foundation/NSCFBoolean.swift

@@ -78,7 +78,10 @@ internal class __NSCFBoolean : NSNumber {
    override var objCType: UnsafePointer<Int8> {
        // This must never be fixed to be "B", although that would
        // cause correct old-style archiving when this is unarchived.
-        return UnsafePointer<Int8>(bitPattern: UInt(_NSSimpleObjCType.Char.rawValue.value))!
+        func _objCType(_ staticString: StaticString) -> UnsafePointer<Int8> {


I like this trick better than the way I was thinking, however I wonder if we have a guarantee this is safe?

Wouldn't the static string need to be a static property to guarantee its lifetime?

I am not fully certain which way this falls, constant strings should emit static references in the binary so it is probably safe-ish?

We have a few alternatives; we could thunk out to C and use the real @encode since we always have clang available. We could thunk out to C and reference C strings as opaque memory blobs. This approach. Or the other thought I had was something like this https://gist.github.com/phausler/2630092f1b391d6a5db0a562288514bf

I was thinking along the same lines, but simpler:

fileprivate let boolEncoding: StaticString = "B" fileprivate let boolEncodingPtr: UnsafePointer<Int8> = { precondition(boolEncoding.isASCII) return UnsafeRawPointer(boolEncoding.utf8Start).assumingMemoryBound(to: Int8.self) }() // etc.

yea, I think there is also value in abstracting it out to an isolated file to do that work in and provide a shared internal (minimal) version of @encode

I discarded following codes that rewrote _NSSimpleObjCType in NSObjCRuntime.swift, since memory layout is not compatible with current implementation.
https://gist.github.com/norio-nomura/a1383f258c38bb1e081597a538f46e8f

Should we create another static values for @encode?
I feel we should do re-writing _NSSimpleObjCType instead of adding them, and doing that with this PR will make it too large.

As I tested, utf8Start points directly cstring in __TEXT,__cstring,cstring_literals whether StaticString is instantiated as static variable or function parameter.

phausler · 2017-07-02T16:14:55Z

Foundation/NSNumber.swift

+            return UnsafeRawPointer(staticString.utf8Start).assumingMemoryBound(to: Int8.self)
+        }
+        let numberType = _CFNumberGetType2(_cfObject)
+        switch numberType {


Iirc gettype2 will only emit SInt* and Float* types

I didn't know. It looks __CFNumberTypeTable.canonicalType contains only those types. 👍

phausler · 2017-07-02T16:16:26Z

The sil_gen part is the only absolute thing that is required

norio-nomura · 2017-07-02T23:45:01Z

Can we remove _CFNumberInit*() which is no longer used?

phausler · 2017-07-03T01:33:13Z

lets hold off on removing _CFNumberInit just yet, I need to verify we are not using that in the objc version (which I was considering)

norio-nomura · 2017-07-03T06:32:12Z

Ah, sorry, it seems I have to fix NSDecimalNumber as well.
It will take some time yet.

This reverts commit bb182da.

norio-nomura · 2017-07-03T07:12:13Z

Ah, sorry, it seems I have to fix NSDecimalNumber as well.

Removing stored instance properties caused issues on NSDecimalNumber. I reverted that.

phausler · 2017-07-03T16:59:59Z

I can follow up with the removals of _CFNumberInit* and the ivars.

phausler · 2017-07-03T17:00:38Z

@swift-ci please test and merge

norio-nomura · 2017-07-03T22:57:15Z

Thanks! 🙏

norio-nomura · 2017-07-04T02:32:24Z

I forgot that I have to ask a question.

The results of test are different between arch(arm64) and arch(arm).
On arch(arm64) (iPhone SE w/ iOS 10.3.3):

sInt8Type: c
sInt16Type: s
sInt32Type: i
sInt64Type: q
float32Type: f
float64Type: d
charType: c
shortType: s
intType: i
longType: q
longLongType: q
floatType: f
doubleType: d
cfIndexType: q
nsIntegerType: q
cgFloatType: d
sInt128Type: Q

On arch(arm) (iPad 3rd w/ iOS 9.3.5):

sInt8Type: i
sInt16Type: i
sInt32Type: i
sInt64Type: q
float32Type: f
float64Type: d
charType: i
shortType: i
intType: i
longType: i
longLongType: q
floatType: f
doubleType: d
cfIndexType: i
nsIntegerType: i
cgFloatType: f
sInt128Type: Q

Is not this a problem?

phausler · 2017-07-04T05:56:59Z

For now that is probably fine, my changes will hopefully bring our strange conditionals per 64 vs 32 bit in more pairity

norio-nomura added 8 commits July 2, 2017 15:58

Add TestNSNumber.test_objCType() reproducing SR-4907

0a7ddcb

Expected results are sampled from macOS

Add missing entry to TestNumber.allTests

67c7981

Change NSNumber.init(value: Bool) to returning __NSCFBoolean by u…

00737ae

…sing `_Factory` protocol

Change JSONSerialization to use _cfTypeID for checking whether `N…

3cbad1d

…SNumber` is Boolean

Update test_objCType

070cf25

Explicitly declare _Factory protocol as internal

358b4f3

Change NSNumber to initialize with CFNumberCreate()

b19da15

Fix NSKeyedUnarchiver.decodeBool(forKey:) and add test to `TestNSPr…

0592ccf

…edicate.test_NSCoding()`

norio-nomura mentioned this pull request Jul 2, 2017

Fix NSNumber.objCType crashes, make it returns same values with darwin platform version #1025

Closed

phausler reviewed Jul 2, 2017

View reviewed changes

norio-nomura added 2 commits July 3, 2017 08:29

Stop using @_silgen_name

b9fa1ac

Remove stored instance properties from NSNumber

bb182da

_CFNumberGetType2 only emit SInt* and Float* types

5a26410

norio-nomura changed the title ~~Use CFNumberCreate() as factory initializer on NSNumber~~ [WIP] Use CFNumberCreate() as factory initializer on NSNumber Jul 3, 2017

Revert "Remove stored instance properties from NSNumber"

829de81

This reverts commit bb182da.

norio-nomura changed the title ~~[WIP] Use CFNumberCreate() as factory initializer on NSNumber~~ Use CFNumberCreate() as factory initializer on NSNumber Jul 3, 2017

swift-ci merged commit 6d107c1 into swiftlang:master Jul 3, 2017

norio-nomura deleted the use-factory-initializer-on-nsnumber branch July 3, 2017 22:57

norio-nomura mentioned this pull request Jul 4, 2017

[SR-4907] Accessing NSNumber.objCType causes SIGSEGV: invalid address on Linux #3856

Closed

		@@ -198,14 +198,20 @@ extension NSNumber : ExpressibleByFloatLiteral, ExpressibleByIntegerLiteral, Exp

		}

		@_silgen_name("_CFNumberGetType2")

Use CFNumberCreate() as factory initializer on NSNumber #1093

Use CFNumberCreate() as factory initializer on NSNumber #1093

Uh oh!

Conversation

norio-nomura commented Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Use CFNumberCreate() as factory initializer on NSNumber.init(value:)

Change the properties of NSNumber to call CFNumberGetValue () in the same way as Foundation.

Fixes SR-4907

Change NSNumber.objCType to returning same values with darwin platform version

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!

phausler commented Jul 2, 2017

Uh oh!

norio-nomura commented Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phausler commented Jul 3, 2017

Uh oh!

norio-nomura commented Jul 3, 2017

Uh oh!

norio-nomura commented Jul 3, 2017

Uh oh!

phausler commented Jul 3, 2017

Uh oh!

phausler commented Jul 3, 2017

Uh oh!

norio-nomura commented Jul 3, 2017

Uh oh!

norio-nomura commented Jul 4, 2017

Uh oh!

phausler commented Jul 4, 2017

Uh oh!

Uh oh!

Use `CFNumberCreate()` as factory initializer on `NSNumber` #1093

Use `CFNumberCreate()` as factory initializer on `NSNumber` #1093

norio-nomura commented Jul 2, 2017 •

edited

Loading

Use `CFNumberCreate()` as factory initializer on `NSNumber.init(value:)`

Change the properties of `NSNumber` to call `CFNumberGetValue ()` in the same way as `Foundation`.

Change `NSNumber.objCType` to returning same values with darwin platform version

norio-nomura commented Jul 2, 2017 •

edited

Loading