Towards s390x support #19822

rposts · 2018-10-10T19:27:16Z

This PR partially enables s390x architecture support for Swift.

Specifically:

Modifies existing s390x SPARE_BIT_MASK
Adds s390x SWIFT_ABI_S390X_OBJC_RESERVED_BITS_MASK and supporting logic
Adds CLongDouble support on s390x
Updates isValidPointerForNativeRetain() function for s390x

millenomi

I’m a little confused; Apple doesn’t ship an ObjC runtime on s390x. Can you comment on what’s up a little?

rposts · 2018-10-11T14:29:19Z

@millenomi - these changes are to address several validation test-case failures and continuation of earlier commits -e.g.: 85fde8b

OBJC related changes are mostly addressing test case failures (>500 in number otherwise).

jckarter · 2018-10-11T17:18:47Z

What are those test failures? Ideally the ObjC defines would not impact platforms without an ObjC runtime. BridgeObject's masking behavior may still be entangled with these masks, though. cc @milseman

milseman · 2018-10-11T17:34:14Z

I'm not familiar with the current status quo, but @mikeash has been refactoring some of this logic. BridgeObjects still need to work on Linux, so I suspect that the ObjC defines shouldn't be necessary, just the Swift ones.

rposts · 2018-10-11T18:37:12Z

@jckarter - here is a snippet of failures after restoring SWIFT_ABI_DEFAULT_OBJC_RESERVED_BITS_MASK :

 #define _swift_abi_ObjCReservedBitsMask                                        \
-  (__swift_uintptr_t) SWIFT_ABI_S390X_OBJC_RESERVED_BITS_MASK
+  (__swift_uintptr_t) SWIFT_ABI_DEFAULT_OBJC_RESERVED_BITS_MASK
...
    Swift(linux-s390x) :: stdlib/mmap.swift
    Swift(linux-s390x) :: stdlib/ReverseCompatibility.swift
    Swift(linux-s390x) :: stdlib/Strideable.swift
    Swift(linux-s390x) :: stdlib/Reflection.swift
    Swift(linux-s390x) :: stdlib/StringAPI.swift
    Swift(linux-s390x) :: stdlib/UnsafeRawBufferPointer.swift
...
    Swift(linux-s390x) :: Evolution/test_superclass_methods.swift
    Swift(linux-s390x) :: StdlibUnittest/CrashingTests.swift
    Swift(linux-s390x) :: Evolution/test_class_add_virtual_method.swift
    Swift(linux-s390x) :: StdlibUnittest/Stdin.swift
    Swift(linux-s390x) :: StdlibUnittest/CommandLine.swift
....
    Swift(linux-s390x) :: IRGen/objc_simd.sil
    Swift(linux-s390x) :: IRGen/errors.sil
    Swift(linux-s390x) :: Index/Store/output-failure.swift

  Expected Passes    : 9135
  Expected Failures  : 77
  Unsupported Tests  : 1386
  Unexpected Failures: 546

So it appears that test cases are heavily dependent on how these masks are defined

mikeash · 2018-10-11T18:54:18Z

#19540 shuffled a bunch of this stuff around about a week ago. The ObjC masks shouldn't be needed if ObjC interop isn't available, but in practice it looks like they are. In particular, we unconditionally incorporate the ObjC reserved bits into the unTaggedNonNativeBridgeObjectBits constant:

https://github.com/apple/swift/blob/master/stdlib/public/runtime/SwiftObject.mm#L495

Ideally, we shouldn't have the ObjC masks around at all if interop isn't available. I think the best solution would be to put the heap_object_abi ObjC constants at the bottom of HeapObject.h inside #if SWIFT_OBJC_INTEROP, and then cleaning up any place where those constants are used outside such a section. I'm not sure if that's beyond the scope of this work, though.

rposts · 2018-10-11T19:21:19Z

I think this area has seen quite a bit of refactoring in recent years. 3d04fb5#diff-ac47cb08ca19bd2108eaf8cb32ddcbc4 appears to have moved bit masks out of Metadata.h.

Scope of this PR is more on s390x big-endian enablement. I wonder if broader cleanup can be attempted elsewhere to keep the scope separate with correct SME involved.

rposts · 2018-10-16T14:19:01Z

@millenomi - please let me know if changes are acceptable within s390x arch context. Thanks.

millenomi · 2018-10-16T14:37:48Z

While I do keep an eye on patches that may affect Foundation or general ObjC interop, I think runtime people like @mikeash are more suited to review.

rposts · 2018-10-22T13:59:17Z

@mikeash - let me know your thoughts on limiting this PR to s390x arch and whether proposed changes are acceptable. Thanks again for your insight.

mikeash · 2018-10-23T00:30:47Z

Yes, they look good. The cleanup I mentioned would be nice but there is no reason to make this change wait for it.

Just curious, when the comment says, "On s390x it is theoretically possible to have high bit set but in practice it is unlikely." How unlikely is it?

rposts · 2018-10-23T15:10:57Z

@mikeash - until recently this was not even possible on s390x with kernels supporting 4-level page table. On recent kernels with 5-level page tables, it is possible but we haven't seen any code using it. It remains unlikely but there can be always be cases where explicit address requests can be made in this range.

mikeash · 2018-10-23T16:30:00Z

Interesting. Thanks for the explanation.

gottesmm · 2018-10-23T16:50:12Z

@rposts question. Once we get this working would it be possible to get a community bot for s390x so we stop breaking you ; )? I.e. here: https://ci-external.swift.org

rposts · 2018-10-23T19:30:19Z

@gottesmm there were some attempts early on but they were stalled due to T&Cs. I am not sure what the latest status is but this is definitely something we would like to pursue.

compnerd · 2018-10-24T00:36:08Z

@swift-ci please test

swift-ci · 2018-10-24T00:36:38Z

Build failed
Swift Test OS X Platform
Git Sha - 99337fc

swift-ci · 2018-10-24T00:38:09Z

Build failed
Swift Test Linux Platform
Git Sha - 99337fc

rposts · 2018-10-24T14:51:03Z

@compnerd OS X platform failure above appears to be timeout related. Please let me know if something is regressed as part of this PR. I could not find anything off the top. thx.

rposts · 2018-10-26T17:23:05Z

@compnerd - can we please kick of another swift-ci. Failure source remains unclear to me - Thx

mikeash · 2018-10-26T20:03:07Z

@swift-ci please test

mikeash · 2018-10-26T20:03:16Z

Let's see if it works better this time.

swift-ci · 2018-10-26T21:12:13Z

Build failed
Swift Test OS X Platform
Git Sha - 33f03a6

milseman · 2018-10-27T02:30:50Z

@swift-ci please test os x platform

swift-ci · 2018-10-27T05:14:35Z

Build failed
Swift Test OS X Platform
Git Sha - 33f03a6

milseman · 2018-10-28T22:12:49Z

@swift-ci please test os x platform

rposts · 2018-10-29T14:55:02Z

Thanks @milseman - I assume all set to go.

DougGregor · 2018-10-29T17:11:58Z

stdlib/public/runtime/HeapObject.cpp

  // kernel, so we can assume that pointer values in this range are invalid.
+  // On s390x it is theoretically possible to have high bit set but in practice


This last commit is a bit nerve-wracking. Is there somewhere we could add an assertion for s390x so if this ever goes awry, it's easy to debug?

@jckarter and @mikeash convinced me that this should be okay, so I'll worry less.

Thanks all for helping us narrow the s390x gap!

Should be fine as long as swift_*alloc* never use the high half of address space. The issue here is whether a pointer to a refcountable Swift object is ever in the high half—if that's never the case, then it's OK.

Is there a way to make this comment more descriptive then? Maybe mention that while the address space theoretically includes it, swift_alloc can never allocated objects at those addresses.

@milseman I cannot be sure if swift_alloc can never allocate objects at those addresses. Is there somewhere I can look to confirm?

By default the allocator functions just call down to the platform malloc, so you could look at whatever malloc does.

Ah ok - yes, malloc can allocate upper range if other address space is exhausted.

How about changing the comment to read this:

// On s390x it is theoretically possible to have high bit set but in practice the swift/system allocators do not allocate in this range.

So it'll happen but only if you allocate ~9.2 exabytes of memory?

yes - that is correct.

Towards s390x support

99337fc

millenomi reviewed Oct 10, 2018

View reviewed changes

Fix case index on Big Endian

33f03a6

milseman merged commit ab2dd10 into swiftlang:master Oct 29, 2018

DougGregor reviewed Oct 29, 2018

View reviewed changes

rposts deleted the swift4.2-s390x-ABIandMask-fix branch November 6, 2018 14:23

rposts mentioned this pull request Nov 9, 2018

Towards s390x arch support - ConfigureSystemZ changes #20481

Merged

		// kernel, so we can assume that pointer values in this range are invalid.
		// On s390x it is theoretically possible to have high bit set but in practice

Towards s390x support #19822

Towards s390x support #19822

Uh oh!

Conversation

rposts commented Oct 10, 2018

Uh oh!

millenomi left a comment

Choose a reason for hiding this comment

Uh oh!

rposts commented Oct 11, 2018

Uh oh!

jckarter commented Oct 11, 2018

Uh oh!

milseman commented Oct 11, 2018

Uh oh!

rposts commented Oct 11, 2018

Uh oh!

mikeash commented Oct 11, 2018

Uh oh!

rposts commented Oct 11, 2018

Uh oh!

rposts commented Oct 16, 2018

Uh oh!

millenomi commented Oct 16, 2018

Uh oh!

rposts commented Oct 22, 2018

Uh oh!

mikeash commented Oct 23, 2018

Uh oh!

rposts commented Oct 23, 2018

Uh oh!

mikeash commented Oct 23, 2018

Uh oh!

gottesmm commented Oct 23, 2018

Uh oh!

rposts commented Oct 23, 2018

Uh oh!

compnerd commented Oct 24, 2018

Uh oh!

swift-ci commented Oct 24, 2018

Uh oh!

swift-ci commented Oct 24, 2018

Uh oh!

rposts commented Oct 24, 2018

Uh oh!

rposts commented Oct 26, 2018

Uh oh!

mikeash commented Oct 26, 2018

Uh oh!

mikeash commented Oct 26, 2018

Uh oh!

swift-ci commented Oct 26, 2018

Uh oh!

milseman commented Oct 27, 2018

Uh oh!

swift-ci commented Oct 27, 2018

Uh oh!

milseman commented Oct 28, 2018

Uh oh!

rposts commented Oct 29, 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!

jckarter Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

jckarter Oct 29, 2018 •

edited

Loading