Skip to content

Towards s390x support #19822

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 29, 2018
Merged

Towards s390x support #19822

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99337fc
Towards s390x support
Oct 10, 2018
33f03a6
Fix case index on Big Endian
Oct 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions stdlib/public/SwiftShims/HeapObject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,9 @@ static_assert(alignof(HeapObject) == alignof(void*),
#define _swift_abi_SwiftSpareBitsMask \
(__swift_uintptr_t) SWIFT_ABI_S390X_SWIFT_SPARE_BITS_MASK
#define _swift_abi_ObjCReservedBitsMask \
(__swift_uintptr_t) SWIFT_ABI_DEFAULT_OBJC_RESERVED_BITS_MASK
(__swift_uintptr_t) SWIFT_ABI_S390X_OBJC_RESERVED_BITS_MASK
#define _swift_abi_ObjCReservedLowBits \
(unsigned) SWIFT_ABI_DEFAULT_OBJC_NUM_RESERVED_LOW_BITS
(unsigned) SWIFT_ABI_S390X_OBJC_NUM_RESERVED_LOW_BITS
#define _swift_BridgeObject_TaggedPointerBits \
(__swift_uintptr_t) SWIFT_ABI_DEFAULT_BRIDGEOBJECT_TAG_64

Expand Down
20 changes: 19 additions & 1 deletion stdlib/public/SwiftShims/System.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,24 @@
/*********************************** s390x ************************************/

// Top byte of pointers is unused, and heap objects are eight-byte aligned.
#define SWIFT_ABI_S390X_SWIFT_SPARE_BITS_MASK 0x0000000000000007ULL
// On s390x it is theoretically possible to have high bit set but in practice
// it is unlikely.
#define SWIFT_ABI_S390X_SWIFT_SPARE_BITS_MASK 0xFF00000000000007ULL

// Objective-C reserves just the high bit for tagged pointers.
#define SWIFT_ABI_S390X_OBJC_RESERVED_BITS_MASK 0x8000000000000000ULL
#define SWIFT_ABI_S390X_OBJC_NUM_RESERVED_LOW_BITS 0

// BridgeObject uses this bit to indicate whether it holds an ObjC object or
// not.
#define SWIFT_ABI_S390X_IS_OBJC_BIT 0x4000000000000000ULL

// ObjC weak reference discriminator is the high bit
// reserved for ObjC tagged pointers plus the LSB.
#define SWIFT_ABI_S390X_OBJC_WEAK_REFERENCE_MARKER_MASK \
(SWIFT_ABI_S390X_OBJC_RESERVED_BITS_MASK | \
1<<SWIFT_ABI_S390X_OBJC_NUM_RESERVED_LOW_BITS)
#define SWIFT_ABI_S390X_OBJC_WEAK_REFERENCE_MARKER_VALUE \
(1<<SWIFT_ABI_S390X_OBJC_NUM_RESERVED_LOW_BITS)

#endif /* SWIFT_ABI_SYSTEM_H */
5 changes: 5 additions & 0 deletions stdlib/public/core/CTypes.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@ public typealias CLongDouble = Double
public typealias CLongDouble = Float80
#endif
// TODO: Fill in definitions for other OSes.
#if arch(s390x)
// On s390x '-mlong-double-64' option with size of 64-bits makes the
// Long Double type equivalent to Double type.
public typealias CLongDouble = Double
#endif
#endif

// FIXME: Is it actually UTF-32 on Darwin?
Expand Down
17 changes: 11 additions & 6 deletions stdlib/public/runtime/EnumImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,16 +110,19 @@ inline unsigned getEnumTagSinglePayloadImpl(
unsigned caseIndexFromExtraTagBits =
payloadSize >= 4 ? 0 : (extraTagBits - 1U) << (payloadSize * 8U);

#if defined(__BIG_ENDIAN__)
// On BE high order bytes contain the index
unsigned long caseIndexFromValue = 0;
unsigned numPayloadTagBytes = std::min(size_t(8), payloadSize);
if (numPayloadTagBytes)
memcpy(reinterpret_cast<uint8_t *>(&caseIndexFromValue) + 8 -
numPayloadTagBytes,
valueAddr, numPayloadTagBytes);
#else
// In practice we should need no more than four bytes from the payload
// area.
unsigned caseIndexFromValue = 0;
unsigned numPayloadTagBytes = std::min(size_t(4), payloadSize);
#if defined(__BIG_ENDIAN__)
if (numPayloadTagBytes)
small_memcpy(reinterpret_cast<uint8_t *>(&caseIndexFromValue) + 4 -
numPayloadTagBytes,
valueAddr, numPayloadTagBytes, true);
#else
if (numPayloadTagBytes)
small_memcpy(&caseIndexFromValue, valueAddr,
numPayloadTagBytes, true);
Expand Down Expand Up @@ -192,6 +195,8 @@ inline void storeEnumTagSinglePayloadImpl(
reinterpret_cast<uint8_t *>(&payloadIndex) + 4 -
numPayloadTagBytes,
numPayloadTagBytes, true);
if (payloadSize > 4)
memset(valueAddr + 4, 0, payloadSize - 4);
if (numExtraTagBytes)
small_memcpy(extraTagBitAddr,
reinterpret_cast<uint8_t *>(&extraTagIndex) + 4 -
Expand Down
6 changes: 4 additions & 2 deletions stdlib/public/runtime/HeapObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,11 @@ using namespace swift;
/// Returns true if the pointer passed to a native retain or release is valid.
/// If false, the operation should immediately return.
static inline bool isValidPointerForNativeRetain(const void *p) {
#if defined(__x86_64__) || defined(__arm64__) || defined(__aarch64__) || defined(_M_ARM64)
// On these platforms, the upper half of address space is reserved for the
#if defined(__x86_64__) || defined(__arm64__) || defined(__aarch64__) || defined(_M_ARM64) || defined(__s390x__)
// On these platforms, except s390x, the upper half of address space is reserved for the
// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks all for helping us narrow the s390x gap!

Copy link
Contributor

@jckarter jckarter Oct 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes - that is correct.

// it is unlikely.
return (intptr_t)p > 0;
#else
return p != nullptr;
Expand Down