[Lldb-commits] [PATCH] D80112: Check if thread was suspended during previous stop added.

Mon Jun 8 12:08:19 PDT 2020

jingham added a comment.

In D80112#2078574 <https://reviews.llvm.org/D80112#2078574>, @fallkrum wrote:

> In D80112#2075097 <https://reviews.llvm.org/D80112#2075097>, @jingham wrote:
>
> > If I understand the problem you are describing, it is that you suspended a thread as a user-level suspend, so it's stop reason got stuck.  That seems okay, its probably useful to keep the stop reason of the thread at what it was when you suspended it.  But that means  when gets asked to do its action again, the action is stale and does the wrong thing.  If that's what's going wrong,
>
>
> Yes, you understood right, that's exactly what was going wrong.
>
> > Actually, you can more easily do that. The thread iteration where we call PerformAction etc. works on a copy of the thread list (since one thread's action could cause the thread list to change). So if you just change the copy operation to only copy over threads which aren't user-suspended, then you should be set.
>
> I don't see that's a copy, it seems that's a reference on the actual thread list:
>
>   ThreadList &curr_thread_list = process_sp->GetThreadList();

Ah, right.  We store aside the list of thread indexes, not the list of threads, and only use it to check for missing threads.

>    So I decided to filter out all suspended thread as you recommended.
>    
>   > It would be good to write a end-to-end test that mirrors this behavior.  It would be pretty straightforward to write an API test that runs a process, sets a breakpoint, hits it on one thread, suspends that thread, then hits it on another thread and make sure the action didn't run twice on the second stop.
>    
>    Don't see it is possible to write such a test due to StopInfoBreakpoint::PerformAction implementation. This method executes only once and saves results of execution so the next time we call PerformAction it will not execute breakpoint's  callback and we have no chance to catch action. Wrote unit tests for ProcessEventData that mirror behaviour. 

I was suggesting something like having a Python breakpoint action that increments a Python variable.  Hit the breakpoint first on thread A.  That should increment it by one.  Then suspend thread A and run to hit the breakpoint on thread B.  If your fix is right, then only the thread B action is run, so the python variable will have a value of 2.  But if the suspended thread action also runs, the variable will have the value of 3.  That seems like a pretty easy test to write, and will ensure that the correct behavior is produced.

This patch has gotten hard to read because the latest version seems to have lots of unrelated changes, maybe from running clang-format over code you aren't actually changing?  Can you remove all these irrelevant changes and repost the patch?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80112/new/

https://reviews.llvm.org/D80112