Change 'isValidPointerForNativeRetain' check for PowerPC(ppc64le) #21038
Conversation
stdlib/public/runtime/HeapObject.cpp
Outdated
| @@ -61,7 +61,7 @@ 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) || defined(__s390x__) | |||
| #if defined(__x86_64__) || defined(__arm64__) || defined(__aarch64__) || defined(_M_ARM64) || defined(__s390x__) || defined(__powerpc64__) || defined(__ppc__) || defined(__PPC__) || defined(__LITTLE_ENDIAN__) | |||
There was a problem hiding this comment.
I don't think __ppc__ or __PPC__ are appropriate, since those could conceivably be 32-bit PPC as well. __LITTLE_ENDIAN__ is definitely not correct here. Is __powerpc64__ sufficient?
There was a problem hiding this comment.
Why is __LITTLE_ENDIAN__ being added unconditionally? Shouldn't it really be || (defined(__powerpc64__) && defined(__LITTLE_ENDIAN__))? I believe that you are targeting ppc64le rather than the previous PPC64 BE ABI.
There was a problem hiding this comment.
Updated the condition as suggested || (defined(powerpc64) && defined(LITTLE_ENDIAN)) above and tested the same. Please review commit 7dbda78
There was a problem hiding this comment.
Can this be approved and merged now? Thanks in advance!
There was a problem hiding this comment.
Sorry for letting this sit. The new version looks good. I'll kick off testing and merge it upon success.
There was a problem hiding this comment.
Thanks @mikeash!
|
@swift-ci please test |
Change 'isValidPointerForNativeRetain' check for PowerPC(ppc64le)
Resolves a crash trying to retain the BridgeObject on PowerPC(ppc64le).