This is an archive of the discontinued LLVM Phabricator instance.

Use non-blocking waitpid in NativeProcessLinux
ClosedPublic

Authored by labath on Apr 16 2015, 2:27 AM.

Details

Summary

This is the first phase of the merging of Monitor and Operation threads in NativeProcessLinux
(which is necessary since the two threads race inside Linux kernel). Here, I reimplement the
Monitor thread do use non-blocking waitpid calls, which enables later addition of code from the
operation thread.

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 23832.Apr 16 2015, 2:27 AM
labath retitled this revision from to Use non-blocking waitpid in NativeProcessLinux.
labath updated this object.
labath edited the test plan for this revision. (Show Details)
labath added reviewers: vharron, ovyalov, tberghammer.
labath added a subscriber: Unknown Object (MLST).
labath updated this revision to Diff 23833.Apr 16 2015, 2:33 AM

Close monitor fd in case monitor initialization fails.

ovyalov edited edge metadata.Apr 16 2015, 7:18 AM

Please see my comments.

source/Plugins/Process/Linux/NativeProcessLinux.cpp
1158 ↗(On Diff #23833)

There is no likelihood of partial read here, right?

1173 ↗(On Diff #23833)

Could you split this method into 2 smaller methods - for example, ReadSignal and ProcessSignal?

1248 ↗(On Diff #23833)

Should we return from this call if size == 0?

1250 ↗(On Diff #23833)

Could you use 'x' to check for exit command?

1275 ↗(On Diff #23833)

s/NativeProcessLinux::/NativeProcessLinux::Monitor

1283 ↗(On Diff #23833)

What do you think about handling commands prior to signals? Otherwise, if there is constant stream of signals exit may take time.

4007 ↗(On Diff #23833)

s/NULL/nullptr

4011 ↗(On Diff #23833)

"Process attach" - is it still relevant in this context?

4021 ↗(On Diff #23833)

You may return here if !IsJoinable().

tberghammer added inline comments.Apr 16 2015, 8:42 AM
source/Plugins/Process/Linux/NativeProcessLinux.cpp
1248–1254 ↗(On Diff #23833)

Why do we need this switch here?

1270 ↗(On Diff #23833)

(nit: ::select())

3998 ↗(On Diff #23833)

Please use a unique_ptr and do a release when we pass ownership to the other thread. I would suggest to use the same semantics for closing monitro_fd also.

source/Plugins/Process/Linux/NativeProcessLinux.h
200 ↗(On Diff #23833)

Do we need to define this class in the header file?

labath added inline comments.Apr 16 2015, 10:41 AM
source/Plugins/Process/Linux/NativeProcessLinux.cpp
1158 ↗(On Diff #23833)

Reading from the fd actually handles the signal, so it would be very strange to "partially" handle it. I have added this logging statement just in case...

1173 ↗(On Diff #23833)

Done.

1248 ↗(On Diff #23833)

I have changed this to use end-of-file signal (size==0) as the exit signal.

1248–1254 ↗(On Diff #23833)

That was me preparing for more commands in later patches. In any case, it is gone now...

1250 ↗(On Diff #23833)

I will use end-of-file as exit command. This means there are no supported commands (yet). Changed to log if we receive an unknown command.

1270 ↗(On Diff #23833)

What is the policy on prefixing stuff with :: ?
I am using a ton of system calls from the global namespace here... Should I prefix all of them??

1275 ↗(On Diff #23833)

Good catch.

1283 ↗(On Diff #23833)

Sounds unlikely, but I can do that just in case.

3998 ↗(On Diff #23833)

That's not as easy as it looks like. Let's say I put the monitor in a unique_ptr and I call LaunchThread(..., monitor_up.release(), ...).
What happens if the thread creation fails? LaunchThread will not delete raw pointer, and since I have just nuked my last pointer to the monitor by calling release, I cannot delete it myself either.
There is a couple of ways to avoid that I can think of, but all involve holding a raw pointer of the object at some point and are more complicated that doing it with a raw pointer from the start. I would be interested in seeing a counter-example though. :)

I was actually looking for a file descriptor RAII class, but I could not find any. LLVM has ScopedHandle for windows file handles, but there is no linux/posix equivalent. The best solution would be to add one to llvm, but I did not want to go that way at the moment. :(

4007 ↗(On Diff #23833)

Done.

4011 ↗(On Diff #23833)

Good point, rephrasing.

4021 ↗(On Diff #23833)

Refactored.

source/Plugins/Process/Linux/NativeProcessLinux.h
200 ↗(On Diff #23833)

It needs access to private methods of NativeProcessLinux (MonitorCallback, at least).

labath updated this revision to Diff 23866.Apr 16 2015, 10:43 AM
labath edited edge metadata.
  • respond to review comments
  • make pipe fds synchronous (I never relied on them async in the first place)
tberghammer added inline comments.Apr 16 2015, 11:08 AM
source/Plugins/Process/Linux/NativeProcessLinux.cpp
1270 ↗(On Diff #23833)

I don't know (I usually use them with :: to avoid any name conflict in the future but I think we don't have any policy).

3998 ↗(On Diff #23833)

I would use a unique_ptr like this (it is cleaner for me but you can leave it as it is now if you like it more):

std::unique_ptr<Monitor> monitor(new Monitor(...))
...
 m_monitor_thread = ThreadLauncher::LaunchThread("monitor", Monitor::RunMonitor, monitor.get(), NULL);
if (m_monitor_thread.IsJoinable())
    monitor.release();
else
{
    ...
}

For the file handle I would suggest to create a RAII based on std::unique_ptr with a lambda here and use it next to the monitor unique_ptr.
Something like this (not tested):

auto fd_deleter = [](int* fd){close(*fd); *fd = -1;}
std::unique_ptr<int, decltype(fd_deleter)> monitor_fd_scope(&m_monitor_fd);
labath added inline comments.Apr 17 2015, 5:35 AM
source/Plugins/Process/Linux/NativeProcessLinux.cpp
3998 ↗(On Diff #23833)

I have redesigned this a bit. Now the Monitor is owned (via a unique_ptr) by the NativeProcessLinux object, and all the destruction (threads, file descriptors...) happens in the Monitor destructor, which sounds more RAII. What do you make of this?

labath updated this revision to Diff 23917.Apr 17 2015, 5:36 AM

redesign the monitor class to make it more RAII

tberghammer accepted this revision.Apr 17 2015, 6:39 AM
tberghammer edited edge metadata.

Looks good, thanks for rewriting it in RAII

This revision is now accepted and ready to land.Apr 17 2015, 6:39 AM
ovyalov accepted this revision.Apr 17 2015, 7:00 AM
ovyalov edited edge metadata.

LGTM

This revision was automatically updated to reflect the committed changes.