i#725: add attach for windows

[OrBenPorath]

Adding attach functionality on windows i#725.
Completing this PR.

Main difference from the original PR, is not taking over threads that are terminating (otherwise attach always fails).
Additionally, added the possibility to sleep 1 millisecond between takeover attempts, and controlling the number of attempts.

Feature marked as experimental.

[derekbruening]

OrBenPorath force-pushed the OrBenPorath:add-windows-attach branch 11 times

Please do not force push: it ruins the history, strands the review comments, etc. See https://dynamorio.org/page_code_reviews.html#sec_code_review_non_member and https://dynamorio.org/page_code_reviews.html#autotoc_md114. The final merge will squash it all down into one clean commit.

[derekbruening]

Thank you for the PR. It looks pretty good: just some nits, and then the documentation and testing. Is your plan to add a test as a separate PR? And include some doc additions in this PR?

[OrBenPorath]

Thank you for the PR. It looks pretty good: just some nits, and then the documentation and testing. Is your plan to add a test as a separate PR? And include some doc additions in this PR?

My plan is to finish everything in this PR.

[OrBenPorath]

Ok, so the test is more stable now (I am not seeing the previous errors), but still fails sometimes.

I saw two errors:

1. Nothing happens until the test times out.
2. Test output missing 'MessageBox closed' before 'done':

118: starting attachee
118: thank you for testing attach
118: thread init
118: done

I don't understand these errors, but here are my thoughts:

1. One of the first things attachee does is to write to the output file (that's the whole point), and nothing else is supposed to happen until it does, so how can it be empty?

• Maybe the timeout is now less than the loops timeout, and we are not seeing anything because the process does not start, hence the pidfile is empty, and it is a different manifestation of the previous error.

2. How is the target process terminated?

• The run takes ~12 seconds. and does not output an error before comparing the outputs, so this is not a kill_background_process(ON) call.
• The process does not output 'timed out' or 'MessageBox closed', so none of its threads initiated the termination.
• Maybe the injection thread somehow terminated the process? Maybe it had an exception? I added prints on exception to have some more visibility if that is the case.

Unfortunately, I will not be available to work on this next week (and probably less available afterwards as well).
How do you want to continue?

[OrBenPorath]

2. How is the target process terminated?

• The run takes ~12 seconds. and does not output an error before comparing the outputs, so this is not a kill_background_process(ON) call.
• The process does not output 'timed out' or 'MessageBox closed', so none of its threads initiated the termination.
• Maybe the injection thread somehow terminated the process? Maybe it had an exception? I added prints on exception to have some more visibility if that is the case.

But now it happened again, and there were no prints from the exception function.

What am I missing?

[derekbruening]

Unfortunately, I will not be available to work on this next week (and probably less available afterwards as well).
How do you want to continue?

One way forward is to add this new test to the flaky ignore list in suite/runsuite_wrapper.pl (find the lists for the appropriate Windows bitwidth(s); put #725 in the comment), update #725 to say that the test still needs work and to be removed from the flaky list (o/w nobody will notice if attach gets broken and the test fails every time), and merge this in.

[derekbruening]

add to whitelist

[derekbruening]

run arm tests

[OrBenPorath]

One way forward is to add this new test to the flaky ignore list in suite/runsuite_wrapper.pl (find the lists for the appropriate Windows bitwidth(s); put #725 in the comment), update #725 to say that the test still needs work and to be removed from the flaky list (o/w nobody will notice if attach gets broken and the test fails every time), and merge this in.

Added to flaky list - is it in the correct place?

Should I just comment on #725 something like
"Added in #5075.
Test still requires works, and to be removed from the flaky list."?

[derekbruening]

One way forward is to add this new test to the flaky ignore list in suite/runsuite_wrapper.pl (find the lists for the appropriate Windows bitwidth(s); put #725 in the comment), update #725 to say that the test still needs work and to be removed from the flaky list (o/w nobody will notice if attach gets broken and the test fails every time), and merge this in.

Added to flaky list - is it in the correct place?

Yes, LGTM. (Huh the existing list for Windows is longer than I thought...need more devs to help clean it up.)

Should I just comment on #725 something like
"Added in #5075.
Test still requires works, and to be removed from the flaky list."?

SGTM, maybe pasting in the details of the failures you had above.

[derekbruening]

Merged. Thank you for the PR, and working through all the debugging to get the test as far as it is!

[OrBenPorath]

Merged. Thank you for the PR, and working through all the debugging to get the test as far as it is!

My pleasure :)
And thank you for DynamoRIO, it is amazing, and an honor to contribute to.