KeyPath issue with s390x

Hi, there, i am having a trouble with using KeyPath on s390x platform. Here is the example:

func generic<A: Equatable, B: Equatable>(a: A, b: B) {
    typealias T = (A, B)
    let kp_t_1 = \T.1

    let tuple = (a, b)
    let k1 = tuple[keyPath: kp_t_1]
    print(k1)
}

generic(a: 2, b: 31)

It supposes to print 31, however it prints 2 instead, which is obviously wrong. I have managed to find a hack fix in the source code:

diff --git a/stdlib/public/core/KeyPath.swift b/stdlib/public/core/KeyPath.swift
index 599757bce3..7159d78e37 100644
--- a/stdlib/public/core/KeyPath.swift
+++ b/stdlib/public/core/KeyPath.swift
@@ -3134,14 +3134,24 @@ internal struct InstantiateKeyPathBuffer : KeyPathPatternVisitor {
         // Look up offset in the type metadata. The value in the pattern is
         // the offset within the metadata object.
         let metadataPtr = unsafeBitCast(base, to: UnsafeRawPointer.self)
         let offset: UInt32
         switch kind {
         case .class:
           offset = UInt32(metadataPtr.load(fromByteOffset: Int(offsetOfOffset),
                                            as: UInt.self))
         case .struct:
           offset = UInt32(metadataPtr.load(fromByteOffset: Int(offsetOfOffset),
-                                           as: UInt32.self))
+                                           as: UInt.self))
         }

The idea is to load the value at metadataPtr+offsetOfOffset as 8 bytes (UInt) wide rather than 4 bytes (UInt32) wide. After checking lib/IRGen/GenKeyPath.swift, it doesn’t seem there is a 64-bit offset stored anywhere. Does anyone have any idea where the root cause might be?

This is the an update for the issue raised above. And also an update on the solution proposed:

diff --git a/stdlib/public/core/KeyPath.swift b/stdlib/public/core/KeyPath.swift
index cc8c089a2e..3e4fa788d8 100644
--- a/stdlib/public/core/KeyPath.swift
+++ b/stdlib/public/core/KeyPath.swift
@@ -3140,8 +3140,12 @@ internal struct InstantiateKeyPathBuffer : KeyPathPatternVisitor {
           offset = UInt32(metadataPtr.load(fromByteOffset: Int(offsetOfOffset),
                                            as: UInt.self))
         case .struct:
-          offset = UInt32(metadataPtr.load(fromByteOffset: Int(offsetOfOffset),
-                                           as: UInt32.self))
+          var ml = metadataPtr.load(fromByteOffset: Int(offsetOfOffset), as: UInt32.self)
+          if(ml == 0)
+            ml = metadataPtr.load(fromByteOffset: Int(offsetOfOffset), as: UInt.self)
+          offset = UInt32(ml)
         }

         let header = RawKeyPathComponent.Header(storedWithOutOfLineOffset: kind,

This is very similar to a PR proposed on an older version of swift https://github.com/apple/swift/pull/12796. But the difference is for the newer master branch, metadataPtr.load(fromByteOffset: Int(offsetOfOffset), as: UInt.self) doesn't always hold. Hi, @Joe_Groff, I notice that you helped with that PR, do you mind having a quick look at this one and sharing some ideas?

Structs only contain 32-bit offsets, so that wouldn't be correct. We should change tuple metadata to use 32-bit offsets to match.

I can take a look at this. We'd want to fix this for Swift 5.1 since it would impact the ABI of how key path patterns for tuples get emitted.

Thanks for the attention. If there is anything I can do, I will be more than happy to help.

@Joe_Groff has this something to do with how I implemented KP support for tuples? If there's anything I can do, I'll try and help!

2 Likes

It's not your fault, to be clear. It was an oversight on my part. It should be straightforward to address.

1 Like

This should fix it: https://github.com/apple/swift/pull/25391

It doesn't seem to fix the issue on my system. I have been using the following two test cases:

import StdlibUnittest

  func generic<A: Equatable, B: Equatable>(a: A, b: B) {

    typealias T = (A, B)
    let kp_t_1 = \T.1
    let tuple = (a, b)
    expectEqual(tuple[keyPath: kp_t_1], tuple.1)
  }

  generic(a: "Tuples Hello", b: 31)
import StdlibUnittest

    struct Container<T, U> {
    let x: (T, U)
    var y: (a: T, b: U)
  }

  func generic<A: Equatable, B: Equatable>(a: A, b: B) {

    let kp_c_y_a = \Container<A, B>.y.a
    let container = Container(x: (a, b), y: (a, b))
    expectEqual(container[keyPath: kp_c_y_a], container.y.0)
  }
  generic(a: "Tuples Hello", b: 31)

And both are failing with segmentation fault after applying the patch.

I see; I forgot that the compiler also emits direct offsets into tuple metadata to project, not only key paths. For ABI compatibility on Apple platforms, the field would have to remain zero-padded for the full 64 bits. On s390x, you have a few more options. For tuple key paths, as a short term solution, you might for instance be able to add four to the offset that the compiler emits so that it loads the correct 32 bits during key path instantiation. A more principled solution would be to take my patch and also change loadTupleOffsetFromMetadata to do a 32-bit load as well; if you want to go further, it would be nice to also reconfigure TupleTypeMetadata on platforms which aren't yet ABI-constrained so that it stores element types and offsets in separate arrays, so that the 32-bit offsets can be densely packed and save some memory.

1 Like

Thanks for the suggestion, @Joe_Groff. The patch posted earlier (below) seems to be the first solution you mentioned:

diff --git a/stdlib/public/core/KeyPath.swift b/stdlib/public/core/KeyPath.swift
index cc8c089a2e..3e4fa788d8 100644
--- a/stdlib/public/core/KeyPath.swift
+++ b/stdlib/public/core/KeyPath.swift
@@ -3140,8 +3140,12 @@ internal struct InstantiateKeyPathBuffer : KeyPathPatternVisitor {
           offset = UInt32(metadataPtr.load(fromByteOffset: Int(offsetOfOffset),
                                            as: UInt.self))
         case .struct:
-          offset = UInt32(metadataPtr.load(fromByteOffset: Int(offsetOfOffset),
-                                           as: UInt32.self))
+          var ml = metadataPtr.load(fromByteOffset: Int(offsetOfOffset), as: UInt32.self)
+          if(ml == 0)
+            ml = metadataPtr.load(fromByteOffset: Int(offsetOfOffset), as: UInt.self)
+          offset = UInt32(ml)
         }

         let header = RawKeyPathComponent.Header(storedWithOutOfLineOffset: kind,

For the more principled solution, I have been playing around the following function in lib/IRGen/GenTuple.cpp.

  /// Project a tuple offset from a tuple metadata structure.
  static llvm::Value *loadTupleOffsetFromMetadata(IRGenFunction &IGF,
                                                  llvm::Value *metadata,
                                                  unsigned index) {
    auto asTuple = IGF.Builder.CreateBitCast(metadata,
                                             IGF.IGM.TupleTypeMetadataPtrTy);

    llvm::Value *indices[] = {
      IGF.IGM.getSize(Size(0)),                   // (*tupleType)
      llvm::ConstantInt::get(IGF.IGM.Int32Ty, 3), //   .Elements
      IGF.IGM.getSize(Size(index)),               //     [index]
      llvm::ConstantInt::get(IGF.IGM.Int32Ty, 1)  //       .Offset
    };
    auto slot = IGF.Builder.CreateInBoundsGEP(asTuple, indices);

    return IGF.Builder.CreateLoad(slot, IGF.IGM.getPointerAlignment(),
                                  metadata->getName()
                                    + "." + Twine(index) + ".offset");
  }

But none is successful. Can you please clarify on what to do? Thanks.

1 Like

To repeat, changing KeyPath.swift in that way is not correct; struct metadata does in fact store an array of UInt32 offsets, and that will do the wrong thing if a struct field is at an offset of zero. The first solution I was suggesting was to change GenKeyPath.cpp on the compiler side to emit the tuple metadata offset +4 so that it loads the least significant 32 bits.

For the second solution, you will need the change from my linked PR to make the field in TargetTupleMetadata::Element be a uint32_t. On the compiler side, you would also need to change IRGenModule::TupleTypeMetadataPtrTy to have an i32 field for .Offset so that the result of CreateInBoundsGEP is an i32* instead of an i64*.

1 Like

Thanks! The following change makes the test pass.

diff --git a/include/swift/ABI/Metadata.h b/include/swift/ABI/Metadata.h
index 7016f8a849..94702ff0a3 100644
--- a/include/swift/ABI/Metadata.h
+++ b/include/swift/ABI/Metadata.h
@@ -1495,7 +1495,7 @@ struct TargetTupleTypeMetadata : public TargetMetadata<Runtime> {
     ConstTargetMetadataPointer<Runtime, swift::TargetMetadata> Type;

     /// The offset of the tuple element within the tuple.
-    StoredSize Offset;
+    uint32_t Offset;

     OpaqueValue *findIn(OpaqueValue *tuple) const {
       return (OpaqueValue*) (((char*) tuple) + Offset);
@@ -1506,6 +1506,9 @@ struct TargetTupleTypeMetadata : public TargetMetadata<Runtime> {
     }
   };

+  static_assert(sizeof(Element) == sizeof(StoredSize) * 2,
+                "element size should be two words");
+
   Element *getElements() {
     return reinterpret_cast<Element*>(this + 1);
   }
diff --git a/lib/IRGen/IRGenModule.cpp b/lib/IRGen/IRGenModule.cpp
index 1cfb741f44..a89c974662 100644
--- a/lib/IRGen/IRGenModule.cpp
+++ b/lib/IRGen/IRGenModule.cpp
@@ -266,7 +266,7 @@ IRGenModule::IRGenModule(IRGenerator &irgen,
   // A tuple type metadata record has a couple extra fields.
   auto tupleElementTy = createStructType(*this, "swift.tuple_element_type", {
     TypeMetadataPtrTy,      // Metadata *Type;
-    SizeTy                  // size_t Offset;
+    Int32Ty                  // size_t Offset;
   });
   TupleTypeMetadataPtrTy = createStructPointerType(*this, "swift.tuple_type", {
     TypeMetadataStructTy,   // (base)
3 Likes