Page MenuHomePhabricator

[lldb] Make TestDeletedExecutable more reliable
ClosedPublic

Authored by teemperor on Wed, Jul 10, 10:51 PM.

Details

Summary

It seems that calling Popen can return to the caller before the started process has read all the needed information
from its executable. This means that in case we delete the executable while the process is still starting up,
this test will create a zombie process which in turn leads to a failing test. On my macOS system this happens quite frequently.

This patch fixes this by letting the test synchronize with the inferior after it has started up.

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor created this revision.Wed, Jul 10, 10:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Jul 10, 10:51 PM
friss added a subscriber: friss.Thu, Jul 11, 7:23 AM

This might mitigate the issue, but timeouts like this are bound to fail in some circumstances (machine load, ...). It's more work, but can we instead have the inferior produce an observable side effect (eg, print some output) and synchronize on this?

Yeah, I'd rather have an explicit communication between debugger and debuggee.
We tried to put sleeps in the code [for e.g. lldb-mi tests in the past] and they end up failing anyway sporadically, which makes a pain to track the problem down.

labath added a subscriber: labath.Thu, Jul 11, 8:03 AM

This might mitigate the issue, but timeouts like this are bound to fail in some circumstances (machine load, ...). It's more work, but can we instead have the inferior produce an observable side effect (eg, print some output) and synchronize on this?

There's a lldbutil.wait_for_file_on_target, which was created exactly for this purpose.

teemperor updated this revision to Diff 209231.Thu, Jul 11, 8:35 AM
teemperor edited the summary of this revision. (Show Details)
  • Timeout -> file synchronization.
  • Made the test process run longer just in case.
davide accepted this revision.Thu, Jul 11, 8:36 AM
This revision is now accepted and ready to land.Thu, Jul 11, 8:36 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jul 11, 12:27 PM