[Runtime] Create an external generic metadata builder. #70771

mikeash · 2024-01-08T19:56:45Z

Create a version of the metadata specialization code which is abstracted so that it can work in different contexts, such as building specialized metadata from dylibs on disk rather than from inside a running process.

The GenericMetadataBuilder class is templatized on a ReaderWriter. The ReaderWriter abstracts out everything that's different between in-process and external construction of this data. Instead of reading and writing pointers directly, the builder calls the ReaderWriter to resolve and write pointers. The ReaderWriter also handles symbol lookups and looking up other Swift types by name.

This is accompanied by a simple implementation of the ReaderWriter which works in-process. The abstracted calls to resolve and write pointers are implemented using standard pointer dereferencing.

A new SWIFT_DEBUG_VALIDATE_EXTERNAL_GENERIC_METADATA_BUILDER environment variable uses the in-process ReaderWriter to validate the builder by running it in parallel with the existing metadata builder code in the runtime. When enabled, the GenericMetadataBuilder is used to build a second copy of metadata built by the runtime, and the two are compared to ensure that they match. When this environment variable is not set, the new builder code is inactive.

The builder is incomplete, and this initial version only works on structs. Any unsupported type produces an error, and skips the validation.

rdar://116592420

include/swift/Runtime/GenericMetadataBuilder.h

al45tair · 2024-01-09T12:08:45Z

include/swift/Runtime/GenericMetadataBuilder.h

+  /// This helper constructor takes a callable which is passed a char**, and
+  /// places the error string into that pointer. This is intended to be used
+  /// with a lambda that calls asprintf, and exists for the use of the
+  /// MAKE_ERROR macro.
+  template <typename Fn>
+  BuilderError(const Fn &makeString) {
+    char *string = nullptr;
+    makeString(&string);
+    if (string)
+      errorString = string;
+    else
+      errorString = "<could not create error string>";
+    free(string);
+  }


Rather than doing this and using the MAKE_ERROR macro, how about having a variadic constructor, e.g.

Suggested change

/// This helper constructor takes a callable which is passed a char**, and

/// places the error string into that pointer. This is intended to be used

/// with a lambda that calls asprintf, and exists for the use of the

/// MAKE_ERROR macro.

template <typename Fn>

BuilderError(const Fn &makeString) {

char *string = nullptr;

makeString(&string);

if (string)

errorString = string;

else

errorString = "<could not create error string>";

free(string);

}

template <class... Args>

[[gnu::format(printf, 1, 2)]]

BuilderError(const char *fmt, Args&&... args) {

char *string = nullptr;

asprintf(&string, fmt, std::forward<Args>(args)...);

if (string)

errorString = string;

else

errorString = "<could not create error string>";

std::free(string);

}

(That might generate a GCC compatibility warning, in which case maybe we want to turn off -Wgcc-compat, or rewrite it to be variadic instead of using a parameter pack.)

It does indeed complain, so I wrote it as a regular variadic function instead.

No idea why I didn't just write a variadic constructor in the first place. The mysteries of the human brain....

Maybe you were thinking of adding __FILE__ and __LINE__ to it or something.

include/swift/Runtime/GenericMetadataBuilder.h

stdlib/public/runtime/GenericMetadataBuilder.cpp

al45tair · 2024-01-09T15:13:53Z

stdlib/public/runtime/GenericMetadataBuilder.cpp

+              .DefaultInstantiationPattern.get());
+      auto extraDataSize = genericValueDataExtraSize(valueDescriptor, pattern);
+      if (auto *error = extraDataSize.getError()) {
+        printf("Failed to get extra data size: %s\n", error->cStr());


Do these want to go to stdout, or stderr, or some logging thing? And should they have some prefix to indicate where they're coming from?

I've redone this stuff to be a little more sensible. They all go to stderr now, and they all go through a validationLog function which prints a prefix on everything, with the exception of the metadata dumper which expects to be able to print partial lines and so doesn't like prefixes much.

I also decided that validation and the various logs aren't quite the same thing, so I changed it to make SWIFT_DEBUG_VALIDATE_EXTERNAL_GENERIC_METADATA_BUILDER an integer, where 1 does the validation silently unless there's an error, and 2 does full logging along with it.

stdlib/public/runtime/GenericMetadataBuilder.cpp

mikeash · 2024-01-09T21:26:58Z

Updated with review feedback. Also conditionalized uses of dladdr/dlsym on SWIFT_STDLIB_HAS_DLADDR.

mikeash · 2024-01-09T21:34:23Z

@swift-ci please test

mikeash · 2024-01-09T22:04:02Z

It would help if I actually added the new MathUtils.h file.

mikeash · 2024-01-09T22:04:09Z

@swift-ci please test

al45tair · 2024-01-10T11:48:41Z

stdlib/public/runtime/GenericMetadataBuilder.cpp

+  template <typename T = char>
+  BuilderErrorOr<Buffer<const T>> getSymbolPointer(const char *name) {
+#if SWIFT_STDLIB_HAS_DLADDR
+    void *ptr = dlsym(RTLD_SELF, name);


Linux doesn't have RTLD_SELF, but does have dlsym(), dladdr() and so on. They don't work as well as on Darwin, mind — they'll only find exported symbols (and they won't work at all if someone statically links with Musl, because in that situation while the APIs exist, they do nothing).

I changed this to check for #ifdef RTLD_SELF and use RTLD_DEFAULT otherwise. Only finding exported symbols is OK for our purposes. Not working at all on Musl is not, but we don't need this to work there at the moment. If we do, we replace the dlsym with a list of the functions we need to be able to find.

al45tair · 2024-01-10T11:51:02Z

stdlib/public/runtime/GenericMetadataBuilder.cpp

+#include "swift/ABI/TargetLayout.h"
+#include "swift/Runtime/EnvironmentVariables.h"
+#include "swift/Runtime/Metadata.h"
+#include <dlfcn.h>


Should probably remove this line

Suggested change

#include <dlfcn.h>

(There's a conditional include of it just below.)

Oops! Fixed.

al45tair · 2024-01-10T11:51:44Z

include/swift/Runtime/GenericMetadataBuilder.h

+#include "swift/Basic/MathUtils.h"
+#include "swift/Demangling/TypeLookupError.h"
+#include "llvm/Support/Casting.h"
+#include <dlfcn.h>


<dlfcn.h> doesn't exist on Windows. We should probably conditionalise this.

Suggested change

#include <dlfcn.h>

#if SWIFT_STDLIB_HAS_DLADDR

#include <dlfcn.h>

#endif

It's actually just a leftover, the only use of dl* is in the .cpp file. Removed.

al45tair · 2024-01-10T11:56:22Z

The other problem looks to be some test that checks the symbols exported from libswiftCore.

mikeash · 2024-01-10T15:35:52Z

@swift-ci please test

mikeash · 2024-01-10T19:32:05Z

SWIFT_STDLIB_HAS_DLADDR is true when building for Windows, apparently. Rather than wrangle with the complexity of correcting that in the build system, I've added a conditional for __has_include(<dlfcn.h>) and we can figure out the build side separately. Windows also doesn't like the use of vasprintf, which is why we have our own swift_vasprintf which I've now switched to here.

mikeash · 2024-01-10T19:32:30Z

@swift-ci please test

mikeash · 2024-01-10T23:19:52Z

Windows doesn't like flockfile. It's not really needed here so I removed the uses.

mikeash · 2024-01-10T23:19:57Z

@swift-ci please test

Create a version of the metadata specialization code which is abstracted so that it can work in different contexts, such as building specialized metadata from dylibs on disk rather than from inside a running process. The GenericMetadataBuilder class is templatized on a ReaderWriter. The ReaderWriter abstracts out everything that's different between in-process and external construction of this data. Instead of reading and writing pointers directly, the builder calls the ReaderWriter to resolve and write pointers. The ReaderWriter also handles symbol lookups and looking up other Swift types by name. This is accompanied by a simple implementation of the ReaderWriter which works in-process. The abstracted calls to resolve and write pointers are implemented using standard pointer dereferencing. A new SWIFT_DEBUG_VALIDATE_EXTERNAL_GENERIC_METADATA_BUILDER environment variable uses the in-process ReaderWriter to validate the builder by running it in parallel with the existing metadata builder code in the runtime. When enabled, the GenericMetadataBuilder is used to build a second copy of metadata built by the runtime, and the two are compared to ensure that they match. When this environment variable is not set, the new builder code is inactive. The builder is incomplete, and this initial version only works on structs. Any unsupported type produces an error, and skips the validation. rdar://116592420

mikeash · 2024-01-11T14:15:41Z

Need to disable the test on Windows since the lack of dlsym there causes validation to fail. We can complete Windows support separately.

mikeash · 2024-01-11T14:16:01Z

@swift-ci please smoke test

mikeash · 2024-01-11T17:43:08Z

Hit the Concurrency/flow_isolation.swift failure, test disabled in #70844. Retrying.

mikeash · 2024-01-11T17:43:12Z

@swift-ci please smoke test

mikeash requested a review from al45tair as a code owner January 8, 2024 19:56

al45tair reviewed Jan 9, 2024

View reviewed changes

mikeash force-pushed the generic-metadata-builder branch 2 times, most recently from 0eb2b8e to 249870c Compare January 9, 2024 21:26

mikeash force-pushed the generic-metadata-builder branch from 249870c to 60fc209 Compare January 9, 2024 22:03

al45tair approved these changes Jan 10, 2024

View reviewed changes

al45tair reviewed Jan 10, 2024

View reviewed changes

mikeash force-pushed the generic-metadata-builder branch from 60fc209 to 9aa0300 Compare January 10, 2024 15:32

mikeash requested a review from a team as a code owner January 10, 2024 15:32

mikeash force-pushed the generic-metadata-builder branch from 9aa0300 to aa10c91 Compare January 10, 2024 19:30

mikeash force-pushed the generic-metadata-builder branch from aa10c91 to 7b368ef Compare January 10, 2024 23:19

mikeash force-pushed the generic-metadata-builder branch from 7b368ef to 29c350e Compare January 11, 2024 14:15

mikeash merged commit feae5f1 into swiftlang:main Jan 11, 2024

[Runtime] Create an external generic metadata builder. #70771

[Runtime] Create an external generic metadata builder. #70771

Uh oh!

Conversation

mikeash commented Jan 8, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikeash commented Jan 9, 2024

Uh oh!

mikeash commented Jan 9, 2024

Uh oh!

mikeash commented Jan 9, 2024

Uh oh!

mikeash commented Jan 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

al45tair commented Jan 10, 2024

Uh oh!

mikeash commented Jan 10, 2024

Uh oh!

mikeash commented Jan 10, 2024

Uh oh!

mikeash commented Jan 10, 2024

Uh oh!

mikeash commented Jan 10, 2024

Uh oh!

mikeash commented Jan 10, 2024

Uh oh!

mikeash commented Jan 11, 2024

Uh oh!

mikeash commented Jan 11, 2024

Uh oh!

mikeash commented Jan 11, 2024

Uh oh!

mikeash commented Jan 11, 2024

Uh oh!

Uh oh!

al45tair Jan 10, 2024 •

edited

Loading