Page MenuHomePhabricator

Add --auto-continue to stop-hooks, fix up a few tests

Authored by jingham on Feb 19 2019, 10:54 AM.



Auto-continuing a stop hook by adding a "continue" command to the hook didn't work very well for multi-threaded programs. The first hook in kept all the others from running, so the behavior was unpredictable.

Also, the stop hooks were running in synchronous mode, which meant that 'continue' in a stop hook would start nesting hook execution, which they were not designed to do.

This patch forces Async execution for the stop-hooks, and adds an auto-continue flag.

I also made it possible to add more than one command at a time to the stop hook by repeating the -o command. I can't remember why I thought it was a good idea originally to have only ONE --one-liner option here & in breakpoint commands, etc. It's really convenient to be able to build these on a single line.

Also fixed up a few tests. The plain stop-hook tests were failing for me, even though I didn't change any relevant output. Adding the 'interpreter.echo-comment-commands false' made them start passing again...

I didn't uncomment the UNSUPPORTED on Linux for stop-hook-threads.test, I'd appreciate somebody giving this a whirl there first to see if this works now.

Diff Detail


Event Timeline

jingham created this revision.Feb 19 2019, 10:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2019, 10:54 AM
jingham updated this revision to Diff 187465.Feb 19 2019, 4:09 PM

I wasn't using the right %lldb when running the tests. That provides the 'echo-comment-commands false'. Run correctly you don't need the command in the test files.

Thank you for jumping onto this. I've tried the patch out on linux, and the stop-hook-threads.test now passes. However, the basic test now fails for me because the stop hook isn't fired in the "run" command. it is caused by the "is hijacked" check in ProcessEventData::DoOnRemoval, as removing that condition causes to hook to fire, but that's probably not the right thing to do here. The difference here is probably caused by PlatformLinux hijacking the process events during the launch sequence, but I am not sure where is the error here. I definitely know that there is something fishy going on in PlatformLinux, as e.g. we don't get a "Process XXX stopped" message when launching with the --stop-at-entry flag, so it's quite possible the error is there. If you think that the general behavior is right there, then we can just XFAIL this test for linux, and one of us will get to fixing PlatformLinux eventually.

36 ↗(On Diff #187465)

BTW, this check is fairly dodgy, because in the case of two threads hitting a breakpoint simultaneously, we will get 4 stop reasons instead of two ("thread list" will be executed twice, and each instance will print the stop reason for both threads). It might be better to replace "thread list" with a different command, or add synchronization to the test to prevent two threads from hitting the breakpoint at once, depending on what we want to test.

jingham updated this revision to Diff 187616.Feb 20 2019, 10:31 AM

Trying to guess how thread list output is going to look is indeed a losing game.

I changed the stop hook to set a global variable in the target, then just print the value at the end. If we ran the stop hook the right number of times, the variable will have the right value.

jingham marked 2 inline comments as done.Feb 20 2019, 10:35 AM

Pavel, I think the new behavior w.r.t. launching is better. When the process plugins are managing events to handle a launch or attach, they really don't want random stop hooks getting in their way. On MacOS, if you set a stop hook before launch you always got one stop message at a place you didn't cause the target to stop, which I think was just confusing.

36 ↗(On Diff #187465)

Yes, trying to make a good test scraping thread list output is no good. Instead I made the stop-hook set a variable in the target. Then we can just check that value to see if we hit and ran the stop hooks the right number of times.

labath accepted this revision.Feb 20 2019, 11:58 PM

Ok, LGTM in that case. In terms of the linux decorators, I guess this means that the "unsupported" decorator can be removed on the threaded version of the test, and an "XFAIL" needs to be added to the non-threaded one.

This revision is now accepted and ready to land.Feb 20 2019, 11:58 PM
jingham updated this revision to Diff 187893.Feb 21 2019, 5:14 PM
jingham marked an inline comment as done.

Is this the right way to do it? I don't know why the other was UNSUPPORTED: linux. The only other XFAIL for linux I could see spelled it -linux-?

It seems both linux and system-linux work. I am not sure what's the difference between the two, but it looks like for other OSs we use the system- form, so I'd go with that.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2019, 4:14 PM

Getting now failure for every run:

LLDB :: ExecControl/StopHook/stop-hook.test

I'm not sure why you ended up reverting this, but it looks like the revert wasn't complete (which is why we got the test failures that Jan mentions). I've finished the revert in r354766.

Getting now failure for every run: LLDB :: ExecControl/StopHook/stop-hook.test

It is because after this commit (rL354706 = GIT ff8c7a0947663ce7515f0b8ee52b9d0fe8883bc3) there was its revert (rL354711 = GIT 0513a24d62e1eef4e8179ced177c3672dab21bce) but it did not revert one testcase patch hunk:

 # CHECK: (lldb) run
+# CHECK-NEXT: Process {{.*}} launched:
 # CHECK-NEXT: (void *) $0 = 0x
+# CHECK-NEXT: Process {{.*}} stopped

while on Fedora 29 x86_64 the LLDB output is reversed:

(lldb) run
(void *) $0 = 0x0000000000405260

Process 460013 stopped
* thread #1, name = 'stop-hook.test.', stop reason = breakpoint 1.1
    frame #0: 0x00000000004011c4 stop-hook.test.tmp`b(val=1) at stop-hook.c:29:10
   26  	{
   27  	    int rc = c(val);
   28  	    void *ptr = malloc(1024);
-> 29  	    if (!ptr)  // Set breakpoint here to test target stop-hook.
   30  	        return -1;
   31  	    else
   32  	        printf("ptr=%p\n", ptr); // We should stop here after stepping.

Process 460013 launched: '.../tools/lldb/lit/ExecControl/StopHook/Output/stop-hook.test.tmp' (x86_64)
(lldb) thread step-over