Page MenuHomePhabricator

Fix race condition in Target::Launch
ClosedPublic

Authored by zturner on Mar 23 2015, 1:14 PM.

Details

Summary

Based on the message at http://lists.cs.uiuc.edu/pipermail/lldb-dev/2015-March/007046.html, it looks like the global event listener is racing with the event thread to pull the process stop event in the case of synchronous execution. This patch addresses this by *always* hijacking process events, and not restoring them until after *both* waits are complete.

With this patch, I can successfully use .lldbinit file to run a process. It still does not fix the issue with the stack trace not displaying as discussed in http://lists.cs.uiuc.edu/pipermail/lldb-dev/2015-March/007033.html, but it seems to be a different but related issue, so I don't attempt to address it here.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 22510.Mar 23 2015, 1:14 PM
zturner retitled this revision from to Fix race condition in Target::Launch.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added a reviewer: jingham.
zturner added a subscriber: Unknown Object (MLST).
zturner updated this revision to Diff 22524.Mar 23 2015, 3:29 PM

Rebase this against ToT. Ilia K committed another patch recently which fixed a race condition for the stop at entry case, but it did nothing for the case of not stopping at entry.

ki.stfu added inline comments.
source/Target/Target.cpp
2605–2606 ↗(On Diff #22524)

Can we use a mixed code (for sync and async execution) to avoid copy-paste?

2627 ↗(On Diff #22524)

it's really needed?

zturner added inline comments.Mar 23 2015, 3:50 PM
source/Target/Target.cpp
2605–2606 ↗(On Diff #22524)

I'm not crazy about the copy/paste either, but I thought this separation made the logic easier to follow. Let's see what Jim or Greg says, I don't feel very strongly, so if there's a consensus one way or the other I'll just go with the consensus.

2627 ↗(On Diff #22524)

I'm still learning this section of the code, but if we hijack the events, surely we need to restore them?

ki.stfu added inline comments.Mar 23 2015, 4:02 PM
source/Target/Target.cpp
2605–2606 ↗(On Diff #22524)

Ok, I don't mind but I'd prefer to keep as is.

2627 ↗(On Diff #22524)

Right, but in this case we do that on line #2688

2688 ↗(On Diff #22524)

here

ping, Greg or Jim would you mind taking a look?

source/Target/Target.cpp
2688 ↗(On Diff #22524)

Thanks, I'll fix this.

clayborg accepted this revision.Mar 25 2015, 2:24 PM
clayborg edited edge metadata.

Looks good to me, but Jim also give the OK just to make sure.

This revision is now accepted and ready to land.Mar 25 2015, 2:24 PM

Jim, thoughts?

jingham edited edge metadata.Mar 26 2015, 10:15 AM

I prefer treating the synchronous & async cases separately. It seems a little easier to read to me, and we're more likely to have to modify one or the other branch, so keeping them separate is slightly preferable from that standpoint.

Is that SyncIOHandler hack needed in the synchronous case?

Jim

I prefer treating the synchronous & async cases separately. It seems a little easier to read to me, and we're more likely to have to modify one or the other branch, so keeping them separate is slightly preferable from that standpoint.

Is that SyncIOHandler hack needed in the synchronous case?

Jim

Not sure, you tell me? :) I guess probably not though, so I can remove it (which also makes the argument for keeping the sync and async cases separate a no-brainer) since at that point there's very little duplication left. Is this good to go after removing the SyncIOHandler hack?

Todd originally put that in, but only in the async case. It is just some dopey little delay in the hope of avoiding a race. The synchronous case is going to the trouble of waiting for a real stop to happen and get reported, that's going to take longer than this little delay.

Of course, it would be better to see how to avoid whatever this delay is papering over, but I don't have the time to open that can of worms right now...

Jim

This revision was automatically updated to reflect the committed changes.