[Macros] Recovery after executable plugin crash #64555
Conversation
rintaro
requested review from
hborla,
slavapestov,
xedin,
zoecarver and
CodaFi
as code owners
|
@swift-ci Please smoke test |
rintaro
force-pushed
the
macros-plugins-crashrecover
branch
from
114e373 to
c2d27b2
Compare
|
@swift-ci Please smoke test |
rintaro
force-pushed
the
macros-plugins-crashrecover
branch
from
c2d27b2 to
7de1669
Compare
|
@swift-ci Please smoke test |
rintaro
changed the title
rintaro
force-pushed
the
macros-plugins-crashrecover
branch
from
7de1669 to
ade92be
Compare
|
@swift-ci Please smoke test |
| @@ -38,9 +38,6 @@ | |||
| #include <io.h> | |||
| #endif | |||
|
|
|||
| extern "C" const void *swift_ASTGen_getCompilerPluginCapability(void *handle); | |||
| extern "C" void swift_ASTGen_destroyCompilerPluginCapability(void *value); | |||
There was a problem hiding this comment.
These were unused declarations accidentally added in 0e31393
lib/Sema/TypeCheckMacros.cpp
Outdated
| executablePlugin, resolvedLibraryPath.c_str(), moduleName.str().data(), | ||
| executablePlugin, resolvedLibraryPathStr.data(), moduleNameStr.data(), |
There was a problem hiding this comment.
s/data()/c_str()/g. Ditto for the callback.
include/swift/AST/PluginRegistry.h
Outdated
| void lock() { mtx.lock(); } | ||
| void unlock() { mtx.unlock(); } | ||
|
|
||
| // Launch the plugin if it's not already running, or it's stale. Return an | ||
| // error if |
There was a problem hiding this comment.
error if ... isn't finished
lib/AST/PluginRegistry.cpp
Outdated
| @@ -94,8 +91,35 @@ PluginRegistry::loadExecutablePlugin(StringRef path) { | |||
| "not executable"); | |||
| } | |||
|
|
|||
| plugin = std::unique_ptr<LoadedExecutablePlugin>( | |||
There was a problem hiding this comment.
You could have this as a local instead and only set it after it's valid (ie. after the spawnIfNeeded). Not crucial, but nice to have it not set unless it's valid IMO.
lib/AST/PluginRegistry.cpp
Outdated
| @@ -179,6 +211,7 @@ ssize_t LoadedExecutablePlugin::write(const void *buf, size_t nbyte) const { | |||
| } | |||
|
|
|||
| llvm::Error LoadedExecutablePlugin::sendMessage(llvm::StringRef message) const { | |||
| assert(isAlive()); | |||
There was a problem hiding this comment.
Someone could theoretically kill the process between where we spawn and sendMessage. It's handled below (ie. write would fail), so I'd probably just remove this.
| let s1: String = #stringify(a + b).1 | ||
| print(s1) | ||
|
|
||
| let s2: String = #evil(42) // expected-error {{failedToReceiveMessage (from macro 'evil')}} |
There was a problem hiding this comment.
We should probably change this message + write out a reproducer for the failure on read. On write... I'm not really sure what to add 😅. Not for this PR though
|
|
||
| // Remove the callback and deallocate it when this ASTContext is destructed. | ||
| ctx.addCleanup([executablePlugin, callback]() { | ||
| executablePlugin->removeOnReconnect(callback); |
There was a problem hiding this comment.
This is not safe when the PluginRegistry is owned by ASTContext. Currently when ASTContext is to own a PluginRegistry, it adds a cleanup function like this->addCleanup([registry]{ delete registry; }), and cleanup functions are called in the order they are added. That means that the plugin registry and its plugins would be deleted before this callback is called. To resolve this:
- Instead of using cleanup callback for the plugin registry, add a field
std::unique_ptr<PluginRegistry> OwnedPluginRegistrytoASTContext::Implementationso it'd be alive until these callbacks are called - or, call
ASTContextcleanup callbacks in reverse order, like a stack
@bnbarham wdyt?
There was a problem hiding this comment.
Went with the former because I think we should reduce the use of addCleanup whenever possible.
There was a problem hiding this comment.
Sounds good to me. I'm all for more managed pointers :). IMO the latter also makes sense as a general change.
When executable plugins crashed or somehow decided to exit, the compiler should relaunch the plugin executable before sending another message.
rintaro
force-pushed
the
macros-plugins-crashrecover
branch
from
ade92be to
a49ab25
Compare
|
@swift-ci Please smoke test |
When executable plugins crashed or somehow decided to exit, the compiler
should relaunch the plugin executable before sending another message.
LoadedExecutablePluginis now a process manager of the plugin executable.LoadedExecutabePlugin::PluginProcessrepresents the actual plugin processLoadedExecutablePlugin::spawnIfNeeded()