-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Macros] Recovery after executable plugin crash #64555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@swift-ci Please smoke test |
114e373 to
c2d27b2
Compare
|
@swift-ci Please smoke test |
c2d27b2 to
7de1669
Compare
|
@swift-ci Please smoke test |
7de1669 to
ade92be
Compare
|
@swift-ci Please smoke test |
lib/AST/PluginRegistry.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were unused declarations accidentally added in 0e31393
lib/Sema/TypeCheckMacros.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/data()/c_str()/g. Ditto for the callback.
include/swift/AST/PluginRegistry.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error if ... isn't finished
lib/AST/PluginRegistry.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
lib/Sema/TypeCheckMacros.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went with the former because I think we should reduce the use of addCleanup whenever possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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()