Skip to content

only selectively kill child, not entire process group #2918

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

Merged
merged 1 commit into from
Sep 18, 2018

Conversation

kroening
Copy link
Member

@kroening kroening commented Sep 7, 2018

This is a new approach to addressing the problem explained in PR #319.

It is enabled by the fact that SMT solvers are now executed with run(), as opposed to system().

Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So ... does it actually work as expected? One experiment is invoking CBMC from Java, then sending a TERM signal to the CBMC process -> the Java process should still terminate ok and not be killed.

src/util/run.cpp Outdated
@@ -202,6 +207,8 @@ int run(
return 1;
}

unregister_child();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asking for an unrelated fix: could the while loop above please get braces around its body? The jump in indentation looked pretty awkward at first sight.


#ifdef _WIN32
#else
std::vector<pid_t> pids_of_children;
pid_t child_pid = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#include <vector> is probably redundant now. Actually more of the includes could be cleaned up (we don't need anything on Windows, csignal is included by the header file already).

@tautschnig
Copy link
Collaborator

It is enabled by the fact that SMT solvers are now executed with run(), as opposed to system().

Note that this is only part of the story: we still use system in places, as well as popen. All of those should be changed to run as well.

Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passed Diffblue compatibility checks (cbmc commit: c93180a).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/84146654

pid_t child_pid = 0;

void register_child(pid_t pid)
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to include an invariant ensuring that only one child is registered.
If multiple child processes are actually supported here, then some kind of warning would be useful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added preconditions

Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passed Diffblue compatibility checks (cbmc commit: 1c8fc56).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/84692107

@kroening kroening merged commit 38af220 into develop Sep 18, 2018
@kroening kroening deleted the child_management branch September 18, 2018 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants