Change the RemoteMirror API to have extensible data layout callback (#15291)

* Change the RemoteMirror API to have extensible data layout callback

* Use DLQ_Get prefix on DataLayoutQueryType enum values

* Simplify MemoryReaderImpl and synthesize minimalDataLayoutQueryFunction

include/swift/Remote/CMemoryReader.h

@@ -21,6 +21,17 @@
 #include "swift/SwiftRemoteMirror/MemoryReaderInterface.h"
 #include "swift/Remote/MemoryReader.h"
 
+struct MemoryReaderImpl {
+  // Opaque pointer passed to all the callback functions.
+  void *reader_context;
+
+  QueryDataLayoutFunction queryDataLayout;
+  FreeBytesFunction free;
+  ReadBytesFunction readBytes;
+  GetStringLengthFunction getStringLength;
+  GetSymbolAddressFunction getSymbolAddress;
+};
+
 namespace swift {
 namespace remote {
 
@@ -31,19 +42,15 @@ class CMemoryReader final : public MemoryReader {
 
 public:
   CMemoryReader(MemoryReaderImpl Impl) : Impl(Impl) {
-    assert(this->Impl.getPointerSize && "No getPointerSize implementation");
+    assert(this->Impl.queryDataLayout && "No queryDataLayout implementation");
     assert(this->Impl.getStringLength && "No stringLength implementation");
     assert(this->Impl.readBytes && "No readBytes implementation");
-    assert(this->Impl.getPointerSize(this->Impl.reader_context) != 0 &&
-           "Invalid target pointer size");
   }
 
-  uint8_t getPointerSize() override {
+  bool queryDataLayout(DataLayoutQueryType type, void *inBuffer,
-    return Impl.getPointerSize(Impl.reader_context);
+                       void *outBuffer) override {
-  }
+    return Impl.queryDataLayout(Impl.reader_context, type, inBuffer,
-
+                                outBuffer) != 0;
-  uint8_t getSizeSize() override {
-    return Impl.getSizeSize(Impl.reader_context);
   }
 
   RemoteAddress getSymbolAddress(const std::string &name) override {

include/swift/Remote/InProcessMemoryReader.h

@@ -27,12 +27,22 @@ namespace remote {
 /// An implementation of MemoryReader which simply reads from the current
 /// address space.
 class InProcessMemoryReader final : public MemoryReader {
-  uint8_t getPointerSize() override {
+  bool queryDataLayout(DataLayoutQueryType type, void *inBuffer,
-    return sizeof(uintptr_t);
+                       void *outBuffer) override {
-  }
+    switch (type) {
+      case DLQ_GetPointerSize: {
+        auto result = static_cast<uint8_t *>(outBuffer);
+        *result = sizeof(void *);
+        return true;
+      }
+      case DLQ_GetSizeSize: {
+        auto result = static_cast<uint8_t *>(outBuffer);
+        *result = sizeof(size_t);
+        return true;
+      }
+    }
 
-  uint8_t getSizeSize() override {
+    return false;
-    return sizeof(size_t);
   }
 
   RemoteAddress getSymbolAddress(const std::string &name) override;

include/swift/Remote/MemoryReader.h

@@ -19,6 +19,7 @@
 #define SWIFT_REMOTE_MEMORYREADER_H
 
 #include "swift/Remote/RemoteAddress.h"
+#include "swift/SwiftRemoteMirror/MemoryReaderInterface.h"
 
 #include <cstring>
 #include <functional>
@@ -38,11 +39,8 @@ public:
   /// A convenient name for the return type from readBytes.
   using ReadBytesResult = std::unique_ptr<const void, std::function<void(const void *)>>;
 
-  /// Return the size of an ordinary pointer in the remote process, in bytes.
+  virtual bool queryDataLayout(DataLayoutQueryType type, void *inBuffer,
-  virtual uint8_t getPointerSize() = 0;
+                               void *outBuffer) = 0;
-
-  /// Return the size of size_t in the remote process, in bytes.
-  virtual uint8_t getSizeSize() = 0;
 
   /// Look up the given public symbol name in the remote process.
   virtual RemoteAddress getSymbolAddress(const std::string &name) = 0;

include/swift/SwiftRemoteMirror/MemoryReaderInterface.h

@@ -32,66 +32,70 @@ extern "C" {
 // in the system library, so we use 'swift_addr_t'.
 typedef uint64_t swift_addr_t;
 
+/// The following callbacks provide "reader_context", which is an opaque context
+/// provided in swift_reflection_createReflectionContext.
+
+/// Free memory returned from readBytes. May be NULL if memory never needs to be freed.
 typedef void (*FreeBytesFunction)(void *reader_context, const void *bytes, void *context);
 
+/// Get the size in bytes of the target's pointer type.
 typedef uint8_t (*PointerSizeFunction)(void *reader_context);
+
+/// Get the size in bytes of the target's size type.
 typedef uint8_t (*SizeSizeFunction)(void *reader_context);
+
+/// Read a sequence of bytes at an address in the target.
+///
+/// \param address the address in the target address space
+/// \param size the number of bytes to read
+/// \param outFreeContext on return, an arbitrary context pointer that the caller will
+///                       pass to the free function
+/// \returns A pointer to the requested memory, or NULL if the memory could not be read.
+///          The caller must invoke the free function on the returned pointer once it's
+///          done using the memory.
 typedef const void *(*ReadBytesFunction)(void *reader_context, swift_addr_t address,
                                          uint64_t size,
                                          void **outFreeContext);
+
+/// Get the string length at the given address.
+///
+/// This scan always occurs in a read-only data section. If the scan
+/// would go beyond the section boundary, a length of 0 should be
+/// returned.
+///
+/// \param address the address in the target address space
+/// \returns The length of the string or 0 if the scan was unsuccessful.
 typedef uint64_t (*GetStringLengthFunction)(void *reader_context,
                                             swift_addr_t address);
+
+/// Get the address of a symbol in the target address space.
+///
+/// \returns true if the lookup was successful.
 typedef swift_addr_t (*GetSymbolAddressFunction)(void *reader_context,
                                                  const char *name,
                                                  uint64_t name_length);
 
-typedef struct MemoryReaderImpl {
+typedef enum {
-  /// An opaque context that the implementor can specify to
+  /// The query should ignore inBuffer, and treat outBuffer as uint8_t* which
-  /// be passed to each of the APIs below.
+  /// should be populated with the size of an ordinary pointer in the remote
-  void *reader_context;
+  /// process, in bytes.
+  DLQ_GetPointerSize,
 
-  /// Get the size in bytes of the target's pointer type.
+  /// The query should ignore inBuffer, and treat outBuffer as uint8_t* which
-  PointerSizeFunction getPointerSize;
+  /// should be populated with the size of size_t in the remote process, in
+  /// bytes.
+  DLQ_GetSizeSize,
+} DataLayoutQueryType;
 
-  /// Get the size in bytes of the target's size type.
+/// Data layout query function, which returns answers based on query types (from
-  SizeSizeFunction getSizeSize;
+/// the DataLayoutQueryType enum). The meaning of the input and output buffer
-
+/// is defined by the type of the query. Unknown requests should be handled by
-  /// Free memory returned from readBytes. May be NULL if memory never needs to be freed.
+/// returning 0.
-  FreeBytesFunction free;
+///
-
+/// \returns 0 on error, non-zero on success.
-  // FIXME: -Wdocumentation complains about \param and \returns on function pointers.
+typedef int (*QueryDataLayoutFunction)(void *reader_context,
-#pragma clang diagnostic push
+                                       DataLayoutQueryType type, void *inBuffer,
-#pragma clang diagnostic ignored "-Wdocumentation"
+                                       void *outBuffer);
-
-  /// Read a sequence of bytes at an address in the target.
-  ///
-  /// \param address the address in the target address space
-  /// \param size the number of bytes to read
-  /// \param outFreeContext on return, an arbitrary context pointer that the caller will
-  ///                       pass to the free function
-  /// \returns A pointer to the requested memory, or NULL if the memory could not be read.
-  ///          The caller must invoke the free function on the returned pointer once it's
-  ///          done using the memory.
-  ReadBytesFunction readBytes;
-
-  /// Get the string length at the given address.
-  ///
-  /// This scan always occurs in a read-only data section. If the scan
-  /// would go beyond the section boundary, a length of 0 should be
-  /// returned.
-  ///
-  /// \param address the address in the target address space
-  /// \returns The length of the string or 0 if the scan was unsuccessful.
-  GetStringLengthFunction getStringLength;
-
-  /// Get the address of a symbol in the target address space.
-  ///
-  /// \returns true if the lookup was successful.
-  GetSymbolAddressFunction getSymbolAddress;
-
-#pragma clang diagnostic pop
-
-} MemoryReaderImpl;
 
 #ifdef __cplusplus
 } // extern "C"

include/swift/SwiftRemoteMirror/SwiftRemoteMirror.h

@@ -51,6 +51,16 @@ swift_reflection_createReflectionContext(void *ReaderContext,
                                          GetStringLengthFunction GetStringLength,
                                          GetSymbolAddressFunction GetSymbolAddress);
 
+/// \returns An opaque reflection context.
+SWIFT_REMOTE_MIRROR_LINKAGE
+SwiftReflectionContextRef
+swift_reflection_createReflectionContextWithDataLayout(void *ReaderContext,
+                                     QueryDataLayoutFunction DataLayout,
+                                     FreeBytesFunction Free,
+                                     ReadBytesFunction ReadBytes,
+                                     GetStringLengthFunction GetStringLength,
+                                     GetSymbolAddressFunction GetSymbolAddress);
+
 /// Destroys an opaque reflection context.
 SWIFT_REMOTE_MIRROR_LINKAGE
 void

stdlib/public/SwiftRemoteMirror/SwiftRemoteMirror.cpp

@@ -50,6 +50,18 @@ swift_reflection_getSupportedMetadataVersion() {
   return SWIFT_REFLECTION_METADATA_VERSION;
 }
 
+template <uint8_t WordSize>
+static int minimalDataLayoutQueryFunction(void *ReaderContext,
+                                          DataLayoutQueryType type,
+                                          void *inBuffer, void *outBuffer) {
+  if (type == DLQ_GetPointerSize || type == DLQ_GetSizeSize) {
+    auto result = static_cast<uint8_t *>(outBuffer);
+    *result = WordSize;
+    return 1;
+  }
+  return 0;
+}
+
 SwiftReflectionContextRef
 swift_reflection_createReflectionContext(void *ReaderContext,
                                          uint8_t PointerSize,
@@ -60,14 +72,31 @@ swift_reflection_createReflectionContext(void *ReaderContext,
   assert((PointerSize == 4 || PointerSize == 8) && "We only support 32-bit and 64-bit.");
   assert(PointerSize == sizeof(uintptr_t) &&
          "We currently only support the pointer size this file was compiled with.");
-  
+
-  auto GetSize = PointerSize == 4
+  auto *DataLayout = PointerSize == 4 ? minimalDataLayoutQueryFunction<4>
-    ? [](void *){ return (uint8_t)4; }
+                                      : minimalDataLayoutQueryFunction<8>;
-    : [](void *){ return (uint8_t)8; };
   MemoryReaderImpl ReaderImpl {
     ReaderContext,
-    GetSize,
+    DataLayout,
-    GetSize,
+    Free,
+    ReadBytes,
+    GetStringLength,
+    GetSymbolAddress
+  };
+
+  return new SwiftReflectionContext(ReaderImpl);
+}
+
+SwiftReflectionContextRef
+swift_reflection_createReflectionContextWithDataLayout(void *ReaderContext,
+                                    QueryDataLayoutFunction DataLayout,
+                                    FreeBytesFunction Free,
+                                    ReadBytesFunction ReadBytes,
+                                    GetStringLengthFunction GetStringLength,
+                                    GetSymbolAddressFunction GetSymbolAddress) {
+  MemoryReaderImpl ReaderImpl {
+    ReaderContext,
+    DataLayout,
     Free,
     ReadBytes,
     GetStringLength,

tools/swift-reflection-dump/swift-reflection-dump.cpp

@@ -161,12 +161,24 @@ public:
   {
   }
 
-  uint8_t getPointerSize() override {
+  bool queryDataLayout(DataLayoutQueryType type, void *inBuffer,
-    return sizeof(uintptr_t);
+                       void *outBuffer) override {
-  }
+    switch (type) {
-  uint8_t getSizeSize() override {
+      case DLQ_GetPointerSize: {
-    return sizeof(size_t);
+        auto result = static_cast<uint8_t *>(outBuffer);
+        *result = sizeof(void *);
+        return true;
+      }
+      case DLQ_GetSizeSize: {
+        auto result = static_cast<uint8_t *>(outBuffer);
+        *result = sizeof(size_t);
+        return true;
+      }
+    }
+
+    return false;
   }
+
   RemoteAddress getSymbolAddress(const std::string &name) override {
     for (auto &object : ObjectFiles) {
       for (auto &symbol : object->symbols()) {

tools/swift-reflection-test/swift-reflection-test.c

@@ -178,6 +178,25 @@ void PipeMemoryReader_collectBytesFromPipe(const PipeMemoryReader *Reader,
   }
 }
 
+static int PipeMemoryReader_queryDataLayout(void *Context,
+                                             DataLayoutQueryType type,
+                                             void *inBuffer, void *outBuffer) {
+  switch (type) {
+    case DLQ_GetPointerSize: {
+      uint8_t *result = (uint8_t *)outBuffer;
+      *result = sizeof(void *);
+      return 1;
+    }
+    case DLQ_GetSizeSize: {
+      uint8_t *result = (uint8_t *)outBuffer;
+      *result = sizeof(size_t);
+      return 1;
+    }
+  }
+
+  return 0;
+}
+
 static void PipeMemoryReader_freeBytes(void *reader_context, const void *bytes,
                                        void *context) {
   free((void *)bytes);
@@ -438,13 +457,12 @@ int doDumpHeapInstance(const char *BinaryFilename) {
     default: { // Parent
       close(PipeMemoryReader_getChildReadFD(&Pipe));
       close(PipeMemoryReader_getChildWriteFD(&Pipe));
-      SwiftReflectionContextRef RC = swift_reflection_createReflectionContext(
+      SwiftReflectionContextRef RC =
-        (void*)&Pipe,
+          swift_reflection_createReflectionContextWithDataLayout(
-        sizeof(void *),
+              (void *)&Pipe, PipeMemoryReader_queryDataLayout,
-        PipeMemoryReader_freeBytes,
+              PipeMemoryReader_freeBytes, PipeMemoryReader_readBytes,
-        PipeMemoryReader_readBytes,
+              PipeMemoryReader_getStringLength,
-        PipeMemoryReader_getStringLength,
+              PipeMemoryReader_getSymbolAddress);
-        PipeMemoryReader_getSymbolAddress);
 
       uint8_t PointerSize = PipeMemoryReader_getPointerSize((void*)&Pipe);
       if (PointerSize != sizeof(uintptr_t))