[Tools] Add a library to build specialized generic metadata out of process. #70900

mikeash · 2024-01-12T23:31:48Z

This library uses GenericMetadataBuilder with a ReaderWriter that can read data and resolve pointers from MachO files, and emit a JSON representation of a dylib containing the built metadata.

We use LLVM's binary file readers to parse the MachO files and resolve fixups so we can follow pointers. This code is somewhat MachO specific, but could be generalized to other formats that LLVM supports.

rdar://116592577

mikeash · 2024-01-17T04:21:52Z

@swift-ci please test

mikeash · 2024-01-17T22:17:56Z

@swift-ci please test

mikeash · 2024-01-18T00:30:38Z

@swift-ci please test

mikeash · 2024-01-18T03:11:26Z

@swift-ci please test

mikeash · 2024-01-18T14:42:01Z

@swift-ci please test

mikeash · 2024-01-18T17:59:25Z

@swift-ci please test

mikeash · 2024-01-18T21:43:50Z

@swift-ci please test windows platform

mikeash · 2024-01-19T00:08:59Z

@swift-ci please test

mikeash · 2024-01-19T15:27:06Z

@swift-ci please test

mikeash · 2024-01-19T19:48:28Z

@swift-ci please test

mikeash · 2024-01-22T19:03:55Z

@swift-ci please test

mikeash · 2024-01-23T14:28:30Z

@swift-ci please test

al45tair · 2024-01-22T17:35:34Z

include/swift/ExternalGenericMetadataBuilder/ExternalGenericMetadataBuilder.h

+    struct SwiftExternalMetadataBuilder *);
+
+// Returns an error string if the dylib could not be added
+// The builder owns the string, so the caller does not have to free it


Is it just the last error string that's valid? (Maybe the comment should say something about that?)

al45tair · 2024-01-22T17:44:59Z

include/swift/Runtime/GenericMetadataBuilder.h

+        if (!VWT_Bi8_)
+          return *VWT_Bi8_.getError();


Should be VWT_Bi16_.

Suggested change

if (!VWT_Bi8_)

return *VWT_Bi8_.getError();

if (!VWT_Bi16_)

return *VWT_Bi16_.getError();

Ouch, major copy/pasta fail there. Good catch.

al45tair · 2024-01-22T17:46:22Z

include/swift/Runtime/GenericMetadataBuilder.h

+        if (!VWT_Bi8_)
+          return *VWT_Bi8_.getError();


Should be VWT_Bi32_.

Suggested change

if (!VWT_Bi8_)

return *VWT_Bi8_.getError();

if (!VWT_Bi32_)

return *VWT_Bi32_.getError();

al45tair · 2024-01-22T17:47:01Z

include/swift/Runtime/GenericMetadataBuilder.h

+        if (!VWT_Bi8_)
+          return *VWT_Bi8_.getError();


Should be VWT_Bi64_.

Suggested change

if (!VWT_Bi8_)

return *VWT_Bi8_.getError();

if (!VWT_Bi64_)

return *VWT_Bi64_.getError();

al45tair · 2024-01-22T17:47:24Z

include/swift/Runtime/GenericMetadataBuilder.h

+        if (!VWT_Bi8_)
+          return *VWT_Bi8_.getError();


Should be VWT_Bi128_.

Suggested change

if (!VWT_Bi8_)

return *VWT_Bi8_.getError();

if (!VWT_Bi128_)

return *VWT_Bi128_.getError();

al45tair · 2024-01-23T14:30:57Z

stdlib/public/ExternalGenericMetadataBuilder/ExternalGenericMetadataBuilder.cpp

+      auto contents = section.getContents();
+      if (!contents) {
+        consumeError(contents.takeError());
+        continue;


I wonder whether we should expose the error that occurred here somehow? Even just mentioning it in the message below might be useful if it happens, e.g.

if (hadErrors) { return BuilderError("Pointer %p is in file %s but is not in any section (we had errors fetching section contents)", ptr, file->path.c_str()); }

That's a good idea. The MachO implementation can never return an error, but it'll be good in case of future expansion to other formats.

al45tair · 2024-01-23T14:32:59Z

stdlib/public/ExternalGenericMetadataBuilder/ExternalGenericMetadataBuilder.cpp

+      while (cursor) {
+        switch (cursor->getKind()) {
+        case Node::Kind::BoundGenericClass:
+        case Node::Kind::BoundGenericStructure:
+        case Node::Kind::BoundGenericEnum:
+        case Node::Kind::BoundGenericProtocol:
+        case Node::Kind::BoundGenericTypeAlias:
+        case Node::Kind::BoundGenericFunction:
+        case Node::Kind::BoundGenericOtherNominalType:
+          if (foundDepth == depth) {
+            auto typeList = cursor->getChild(1);
+            if (!typeList)
+              return BuilderError(
+                  "Requested generic parameter at depth=%" PRIu64
+                  " index=%" PRIu64
+                  ", BoundGeneric node does not have two children",
+                  depth, index);
+            if (typeList->getKind() != Node::Kind::TypeList)
+              return BuilderError(
+                  "Requested generic parameter at depth=%" PRIu64
+                  " index=%" PRIu64
+                  ", child 1 of BoundGeneric is %s, not TypeList",
+                  depth, index, nodeKindString(typeList->getKind()));
+
+            auto child = typeList->getChild(index);
+            if (!child)
+              return BuilderError(
+                  "Requested generic parameter at depth=%" PRIu64
+                  " index=%" PRIu64
+                  ", but bound generic TypeList only has %zu children",
+                  depth, index, typeList->getNumChildren());
+
+            return child;
+          }
+          foundDepth++;
+          break;
+
+        default:
+          break;
+        }
+      }


This loop doesn't appear to advance cursor at all.

Oops. Looks like we don't yet have enough support for nested types for this to matter. I added code to walk the tree here, and we'll end up testing it as part of testing expanded support for nested types.

al45tair · 2024-01-23T14:55:43Z

stdlib/public/ExternalGenericMetadataBuilder/ExternalGenericMetadataBuilder.cpp

+ExternalGenericMetadataBuilderContext<Runtime>::addImagesInPath(
+    std::string path) {
+  auto forEachFile = [&](std::string filepath) {
+    auto error = addImageAtPath(filepath);


Should we check the file extension at least, to avoid trying to load everything in sight? Or maybe only generate the warning about not being able to read a dylib if the extension is .dylib, .so or empty? (We expect something with the extension .txt to fail, after all, and in some ways it'd be more interesting if it didn't.)

I'm not sure what the right approach is here. It doesn't matter too much, as swift_externalMetadataBuilder_addDylib is intended to be the primary interface, and this is a convenience. For the convenience interface, I'd rather it try load everything in sight rather than have it potentially skip something we want to keep if somebody puts a weird extension on their dylib.

al45tair · 2024-01-23T15:02:24Z

stdlib/public/ExternalGenericMetadataBuilder/ExternalGenericMetadataBuilder.cpp

+    if (command.C.cmd == llvm::MachO::LC_LOAD_DYLIB ||
+        command.C.cmd == llvm::MachO::LC_LOAD_WEAK_DYLIB ||
+        command.C.cmd == llvm::MachO::LC_REEXPORT_DYLIB ||
+        command.C.cmd == llvm::MachO::LC_LOAD_UPWARD_DYLIB) {


Do we need LC_LAZY_LOAD_DYLIB?

It seems not. LC_LAZY_LOAD_DYLIB seems to be obsolete, and this matches the behavior of dyld.

al45tair · 2024-01-23T15:15:08Z

test/ExternalGenericMetadataBuilder/ExternalGenericMetadataBuilder.swift

+  let readJSONError = readJSONErrorCStr.map { String(cString: $0) }
+  expectNil(readJSONError)
+
+  // Find libswiftCore dynamically since we don't know where we might have loaded it from.


swift_getRuntimeLibraryPath() will tell you this, FWIW.

Thanks, that's much nicer.

mikeash · 2024-01-23T21:39:17Z

Addressed review feedback and moved the code to lib to join its library/tool friends. I also improved the test to be a bit more comprehensive and test the full JSON output rather than just looking for key strings.

mikeash · 2024-01-23T21:39:31Z

@swift-ci please test

mikeash · 2024-01-24T20:33:00Z

@swift-ci please test

mikeash · 2024-01-24T22:10:09Z

@swift-ci please clean test Windows platform

…ocess. This library uses GenericMetadataBuilder with a ReaderWriter that can read data and resolve pointers from MachO files, and emit a JSON representation of a dylib containing the built metadata. We use LLVM's binary file readers to parse the MachO files and resolve fixups so we can follow pointers. This code is somewhat MachO specific, but could be generalized to other formats that LLVM supports. rdar://116592577

mikeash · 2024-01-25T01:46:10Z

@swift-ci please test

mikeash · 2024-01-25T14:40:45Z

@swift-ci please test macos platform

mikeash force-pushed the swift-generic-metadata-builder-out-of-process branch 2 times, most recently from 0ca8dec to 632ee03 Compare January 17, 2024 04:21

mikeash force-pushed the swift-generic-metadata-builder-out-of-process branch 2 times, most recently from 613974c to 30e17f9 Compare January 17, 2024 22:16

mikeash force-pushed the swift-generic-metadata-builder-out-of-process branch from 30e17f9 to 54f6b3f Compare January 18, 2024 00:30

mikeash force-pushed the swift-generic-metadata-builder-out-of-process branch from 54f6b3f to 599e9f0 Compare January 18, 2024 03:10

mikeash force-pushed the swift-generic-metadata-builder-out-of-process branch from 599e9f0 to 13af9a2 Compare January 18, 2024 14:41

mikeash marked this pull request as ready for review January 18, 2024 14:41

mikeash requested review from al45tair and a team as code owners January 18, 2024 14:41

mikeash changed the title ~~[WIP][Tools] Add a library to build specialized generic metadata out of process.~~ [Tools] Add a library to build specialized generic metadata out of process. Jan 18, 2024

mikeash force-pushed the swift-generic-metadata-builder-out-of-process branch from 13af9a2 to 6ac4da3 Compare January 18, 2024 17:56

mikeash force-pushed the swift-generic-metadata-builder-out-of-process branch from 6ac4da3 to 15f471c Compare January 18, 2024 21:43

mikeash force-pushed the swift-generic-metadata-builder-out-of-process branch from 15f471c to 6171779 Compare January 19, 2024 00:08

mikeash force-pushed the swift-generic-metadata-builder-out-of-process branch from 6171779 to ef41fbb Compare January 19, 2024 15:26

mikeash force-pushed the swift-generic-metadata-builder-out-of-process branch from ef41fbb to 9bca2a8 Compare January 19, 2024 19:48

mikeash force-pushed the swift-generic-metadata-builder-out-of-process branch from 9bca2a8 to db61333 Compare January 22, 2024 19:02

mikeash force-pushed the swift-generic-metadata-builder-out-of-process branch from db61333 to f62bbd5 Compare January 23, 2024 14:28

al45tair reviewed Jan 23, 2024

View reviewed changes

mikeash force-pushed the swift-generic-metadata-builder-out-of-process branch from f62bbd5 to ad253bb Compare January 23, 2024 21:38

mikeash force-pushed the swift-generic-metadata-builder-out-of-process branch from ad253bb to 9f54982 Compare January 24, 2024 20:32

mikeash force-pushed the swift-generic-metadata-builder-out-of-process branch from 9f54982 to 4341102 Compare January 25, 2024 01:45

mikeash merged commit 4bc7726 into swiftlang:main Jan 26, 2024

[Tools] Add a library to build specialized generic metadata out of process. #70900

[Tools] Add a library to build specialized generic metadata out of process. #70900

Uh oh!

Conversation

mikeash commented Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikeash commented Jan 17, 2024

Uh oh!

mikeash commented Jan 17, 2024

Uh oh!

mikeash commented Jan 18, 2024

Uh oh!

mikeash commented Jan 18, 2024

Uh oh!

mikeash commented Jan 18, 2024

Uh oh!

mikeash commented Jan 18, 2024

Uh oh!

mikeash commented Jan 18, 2024

Uh oh!

mikeash commented Jan 19, 2024

Uh oh!

mikeash commented Jan 19, 2024

Uh oh!

mikeash commented Jan 19, 2024

Uh oh!

mikeash commented Jan 22, 2024

Uh oh!

mikeash commented Jan 23, 2024

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

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

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!

mikeash commented Jan 23, 2024

Uh oh!

mikeash commented Jan 23, 2024

Uh oh!

mikeash commented Jan 24, 2024

Uh oh!

mikeash commented Jan 24, 2024

Uh oh!

mikeash commented Jan 25, 2024

Uh oh!

mikeash commented Jan 25, 2024

Uh oh!

Uh oh!

mikeash commented Jan 12, 2024 •

edited

Loading