Page MenuHomePhabricator

Continue after process exit
AcceptedPublic

Authored by Honsik on Feb 25 2016, 8:01 PM.

Details

Reviewers
zturner
jingham
Summary

When process exits, and SBProcess.Continue() is executed, timeout occurs (5secs). Instead error message "Process is not alive" is returned. Added test for this message.

Fix of breakpoint case sensitivity test on Linux.

Diff Detail

Event Timeline

Honsik updated this revision to Diff 49149.Feb 25 2016, 8:01 PM
Honsik retitled this revision from to Continue after process exit.
Honsik updated this object.
Honsik added a reviewer: zturner.
Honsik added a subscriber: lldb-commits.

It's okay to short-circuit this here, but why was PrivateResume not returning an error when the process was not alive. That error should have gotten propagated to the caller, obviating the need for this short-circuit.

Honsik added a comment.EditedFeb 26 2016, 9:46 AM

I tried to put this check in PrivateResume, but its not that simple because of the public run lock that is acquired in Resume and ResumeSynchronous. I am not that sure if it is safe to always unclock the lock inside PrivateResume.

zturner edited edge metadata.Feb 26 2016, 9:47 AM

It doesn't look like Process::PrivateResume() returns an error if the process is alive unless WillResume() returns an error, which is up to the individual process implementation. So maybe the short circuit needs to happen there. This isn't really my area though so I'll defer to Jim on this review.

jingham requested changes to this revision.Feb 26 2016, 10:02 AM
jingham added a reviewer: jingham.

I agree with Zachary, it would be better to put it in PrivateResume before the call to WillResume. Having this happen in Process::PrivateResume after taking the run lock is okay, that works correctly on OS X.

OTOH, the error reporting isn't correct there:

lldb.process.Continue()

<lldb.SBError; proxy of <Swig Object of type 'lldb::SBError *' at 0x10b9e7c00> >
Process 64883 exited with status = 0 (0x00000000)

error = lldb.process.Continue()
print error

error: Resume timed out.

So this definitely needs fixing generically...

Process::WillResume only gets called in one place (Process::PrivateResume) so it is fine to just put the check there before calling WillResume. When we have generic bits of work we want to do before or after a plugin method X we often make a virtual "DoX" and have that be the plugin method, and then X is not virtual and does the generic work. But that seems overkill in this case, we just want to make sure the process is alive before calling into the plugins.

This revision now requires changes to proceed.Feb 26 2016, 10:02 AM
Honsik updated this revision to Diff 49258.EditedFeb 26 2016, 3:35 PM
Honsik edited edge metadata.

OK I have modified the PrivateResume to add the check and release the public running lock.

The "Resume timed out" is caused by waiting for debugging thread events, but they won't ever come, because process is already dead.

I don't think you can manipulate the public run lock in PrivateResume like this. PrivateResume gets run in a bunch of places (like calling functions) that are way below the level the public run lock. You probably need to catch errors from PrivateResume in Resume and release the lock there.

That's a little ugly, but it is good to have PrivateResume return an accurate error, so I think putting the check there is right.

jingham requested changes to this revision.Feb 26 2016, 4:07 PM
jingham edited edge metadata.
This revision now requires changes to proceed.Feb 26 2016, 4:07 PM
Honsik updated this revision to Diff 50373.Mar 10 2016, 4:06 PM
Honsik edited edge metadata.

Sorry for the delay, I have been on holiday.

I have modified the patch as jingham requested. The public run lock is reset back on error in both Resume and ResumeSynchronous methods.

jingham accepted this revision.Mar 10 2016, 4:36 PM
jingham edited edge metadata.

I marked a comment left over from a previous draft of the patch that isn't needed. Other than that, this looks fine.

source/Target/Process.cpp
3561–3562

Is this comment needed anymore?

This revision is now accepted and ready to land.Mar 10 2016, 4:36 PM

More precisely, the "Public running lock..." part of the comment.

Petr, is this one ok to go in?

Yes please, with that comment change jingham has mentioned.

Do you want me to create new diff?

I can remove that comment for you, no worries.

Looks like patch was not committed.