[Macros] In-process plugin server library tied to compiler host, not target #74785

drodriguez · 2024-06-27T16:15:49Z

PR #73725 introduced the in-process plugin server library, but the selection of the library depends on the selected toolchain, which depends on the compiler target, not the host. When cross-compiling (for example from macOS to a embedded Unix target), the compiler will incorrectly chose the .so file, not find it, and fail to compile things like the @debugDescription macro.

Move the in-process plugin server library code from the platform toolchains into the parent type, and code it so it uses the right name depending on the compiler host at compilation time. This discards the target and only relies on the compiler host for selecting the right library.

drodriguez · 2024-06-27T16:16:00Z

@swift-ci please test

drodriguez · 2024-06-27T16:19:34Z

lib/Driver/WindowsToolChains.cpp

-  SmallString<261> InProcPluginServerPath = LibraryPath;
-  llvm::sys::path::append(InProcPluginServerPath,
-                          "SwiftInProcPluginServer.dll");
-  Arguments.push_back("-in-process-plugin-server-path");
-  Arguments.push_back(Args.MakeArgString(InProcPluginServerPath));
+  addInProcPluginServerPath(LibraryPath, Args, Arguments);


@rintaro what I was commenting in #74740 (comment) about "not fully sure if the code around it is completely correct" is about the calculation of PluginPath and LibraryPath. The code right now is part of the *ToolChains.cpp, but I am not sure if it should move to a more central place as well, since I think it depends on the compiler host, not the target as well.

I can do the change, but I am not sure about its correctness, so I will wait until someone with more knowledge can say if this is the right place or it should be moved out of the *ToolChains.cpp and into the ToolChains.cpp file as well.

drodriguez · 2024-06-27T22:34:21Z

@swift-ci please test

rintaro · 2024-06-27T23:10:27Z

lib/Driver/ToolChains.cpp

@@ -1468,6 +1468,25 @@ void ToolChain::addLinkRuntimeLib(const ArgList &Args, ArgStringList &Arguments,
  Arguments.push_back(Args.MakeArgString(P));
 }

+void ToolChain::addInProcPluginServerPath(const StringRef PluginPath,


I feel this is still incorrect. Say when building in macOS targeting Windows, WindowsToolchain is used, and it passes in the bin directory. But in macOS hosts this function expects lib/swift.

Yes, as you said the entire addPluginArguments looks not doing the right thing.

Yes. That's what I was asking in the comment above. It looks like more than this needs to move outside the *ToolChains.cpp and into ToolChains.cpp and make it compile conditionally depending on the platform, but I was not completely sure.

If I see things correctly I think this is what is happening:

In addPluginArguments

Calculate a base path.

Darwin and Unix: Use computeRuntimeResourcePathFromExecutablePath

Windows: Use executable path (?)

Add the -in-process-plugin-server-path (this PR)

Darwin and Unix: Use a "host" directory. Each one with its platform extension.

Windows: Don't use extra directories. No "lib" prefix and ".dll" extension.

Add a -plugin-path

Darwin and Unix: Uses base path + "host/plugins"

Windows: Uses the executable path (?)

Add a local plugin path

Darwin and Unix: Add a "../../local/lib/swift/host/plugins" with an extra -plugin-path

Windows: nothing

In addExternalPluginFrontendArgs and addPlatformSpecificPluginFrontendArgs

Only exist on Darwin

Does this need to be in fact for the Darwin compiler, and not for the Darwin targets?

I think all the code in addPluginArguments should be tied to the compiler host, and not the target, but I wanted to first have confirmation that this is in fact necessary. There might be something that I don't completely understand about the plugin compilation module. If you agree with the above and can answer about addPlatformSpecificPluginFrontendArgs I can try to do the changes. I knew this works in the case I was experiencing, probably because the addPluginArguments ended up being the same for Darwin and Unix after my changes, but I imagine Windows -> Unix will have its own share of problems as well.

Thank you for doing this Daniel!

As for addPluginArguments, I believe everything you mentioned is correct. In Windows host, everything must be in the same directory as the getDriver().getSwiftProgramPath().

As for addExternalPluginFrontendArgs(), this is for users of the macOS/iOS/etc SDKs, so it's basically tied to the target, and I believe it's fine this is in DarwinToolchain. That said, since Apple doesn't provide swift-plugin-server that runs other host platforms, maybe we should wrap this function's body with #if defined(__APPLE__), but I don't think it's really necessary.

I hopefully matched the code as close as possible to the one in the swift-driver. Hopefully it is what you were asking for.

drodriguez · 2024-06-28T03:46:12Z

@swift-ci please smoke test

rintaro · 2024-06-28T03:47:05Z

lib/Driver/ToolChains.cpp

+  auto swiftProgramPath = getDriver().getSwiftProgramPath();
+#if defined(_WIN32)
+  SmallString<261> pluginPathBase = StringRef(swiftProgramPath);
+  llvm::sys::path::remove_filename(pluginPathBase); // Remove `swift`
+#elif defined(__APPLE__) || defined(__unix__)
+  SmallString<64> pluginPathBase;
+  CompilerInvocation::computeRuntimeResourcePathFromExecutablePath(
+      swiftProgramPath, /*shared=*/true, pluginPathBase);
+#else


I checked swift-driver implementation
https://github.com/apple/swift-driver/blob/6d1e5d2bd8e67432176cd843704bd2277616376f/Sources/SwiftDriver/Jobs/FrontendJobHelpers.swift#L793-L802
Since it's all based on the parent of the executable directory. Let's align with it.

And yes, adding pluginPathRoot.localPluginPath in Windows is wrong 😅
https://github.com/apple/swift-driver/blob/6d1e5d2bd8e67432176cd843704bd2277616376f/Sources/SwiftDriver/Jobs/FrontendJobHelpers.swift#L810

drodriguez · 2024-06-29T17:20:34Z

@swift-ci please smoke test

drodriguez · 2024-06-30T16:57:52Z

@swift-ci please smoke test

…target PR swiftlang#73725 introduced the in-process plugin server library, but the selection of the library depends on the selected toolchain, which depends on the compiler target, not the host. When cross-compiling (for example from macOS to a embedded Unix target), the compiler will incorrectly chose the `.so` file, not find it, and fail to compile things like the `@debugDescription` macro. Move the in-process plugin server library code from the platform toolchains into the parent type, and code it so it uses the right name depending on the compiler host at compilation time. This discards the target and only relies on the compiler host for selecting the right library.

drodriguez · 2024-07-01T00:20:59Z

@swift-ci please smoke test

drodriguez · 2024-07-01T15:07:03Z

@rintaro seems that I got it right this time. Please review the final version and let me know if I should merge.

rintaro · 2024-07-01T15:48:46Z

Thank you!

drodriguez requested a review from rintaro June 27, 2024 16:15

drodriguez requested a review from artemcm as a code owner June 27, 2024 16:15

drodriguez commented Jun 27, 2024

View reviewed changes

rintaro self-assigned this Jun 27, 2024

drodriguez force-pushed the inproc-plugin-server-right-library branch from 65ab3f3 to 979c98b Compare June 27, 2024 22:33

rintaro reviewed Jun 27, 2024

View reviewed changes

drodriguez force-pushed the inproc-plugin-server-right-library branch from 979c98b to 104ac44 Compare June 28, 2024 03:45

rintaro reviewed Jun 28, 2024

View reviewed changes

rintaro removed their assignment Jun 28, 2024

drodriguez force-pushed the inproc-plugin-server-right-library branch 2 times, most recently from ed88c2b to 177d342 Compare June 29, 2024 17:14

drodriguez force-pushed the inproc-plugin-server-right-library branch from 177d342 to eb3cf18 Compare June 30, 2024 16:57

drodriguez force-pushed the inproc-plugin-server-right-library branch from eb3cf18 to 55d9e74 Compare July 1, 2024 00:20

rintaro approved these changes Jul 1, 2024

View reviewed changes

drodriguez merged commit 5280cea into swiftlang:main Jul 1, 2024
3 checks passed

drodriguez deleted the inproc-plugin-server-right-library branch July 1, 2024 15:51

rintaro mentioned this pull request Jul 1, 2024

[6.0][Macros] In-process plugin server #74740

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Macros] In-process plugin server library tied to compiler host, not target #74785

[Macros] In-process plugin server library tied to compiler host, not target #74785

drodriguez commented Jun 27, 2024

drodriguez commented Jun 27, 2024

drodriguez Jun 27, 2024

drodriguez commented Jun 27, 2024

rintaro Jun 27, 2024 •

edited

Loading

drodriguez Jun 27, 2024

rintaro Jun 28, 2024 •

edited

Loading

drodriguez Jun 29, 2024

drodriguez commented Jun 28, 2024

rintaro Jun 28, 2024 •

edited

Loading

rintaro Jun 28, 2024 •

edited

Loading

drodriguez commented Jun 29, 2024

drodriguez commented Jun 30, 2024

drodriguez commented Jul 1, 2024

drodriguez commented Jul 1, 2024

rintaro commented Jul 1, 2024

[Macros] In-process plugin server library tied to compiler host, not target #74785

[Macros] In-process plugin server library tied to compiler host, not target #74785

Conversation

drodriguez commented Jun 27, 2024

drodriguez commented Jun 27, 2024

drodriguez Jun 27, 2024

Choose a reason for hiding this comment

drodriguez commented Jun 27, 2024

rintaro Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

drodriguez Jun 27, 2024

Choose a reason for hiding this comment

rintaro Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

drodriguez Jun 29, 2024

Choose a reason for hiding this comment

drodriguez commented Jun 28, 2024

rintaro Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

rintaro Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

drodriguez commented Jun 29, 2024

drodriguez commented Jun 30, 2024

drodriguez commented Jul 1, 2024

drodriguez commented Jul 1, 2024

rintaro commented Jul 1, 2024

rintaro Jun 27, 2024 •

edited

Loading

rintaro Jun 28, 2024 •

edited

Loading

rintaro Jun 28, 2024 •

edited

Loading

rintaro Jun 28, 2024 •

edited

Loading