[Lldb-commits] [PATCH] D61687: Update Python tests for lldb-server on Windows

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu May 16 03:00:08 PDT 2019


labath added a comment.

Thanks for explaining. I think this is good to go, sans the merging of the two tests I requested, and the comment fix that @clayborg noticed.



================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py:208-212
+    # In current implementation of llgs on Windows, as a response to '\x03' packet, the debugger
+    # of the native process will trigger a call to DebugBreakProcess that will create a new thread
+    # to handle the exception debug event. So one more stop thread will be notified to the
+    # delegate, e.g. llgs.  So tests below to assert the stop threads number will all fail.
+    @expectedFailureAll(oslist=["windows"])
----------------
Hui wrote:
> labath wrote:
> > labath wrote:
> > > Is this something that we consider to be a bug, or is it just how debugging is supposed to work on windows? If it's a bug then fine, but if not then we might consider adjusting the expectations in the test (or just skipping it).
> > You marked this as done, but it's not obvious how is this comment resolved (or indeed, if it needs to be resolved). Can you elaborate?
> DebugBreakProcess is supposed to spawn a new thread in the debugged process and then the thread exits after the irq is handled.
> 
> See below thread #1 is main thread of the debugged process and thread #2 is the newly spawned.
> json string contains two stopped threads information.
> 
> looks like the stopped threads number is supposed to +1, but in order to be consistent with Visual Studio,
> it shall be possible to only report the thread #1 stop info.
> 
> To modify the python script for Windows is at some cost. Maybe just skip them?
> 
> ```
> (lldb) process interrupt
>  GDBRemoteClientBase::Lock::Lock sent packet: \x03
> ...
>  <  16> send packet: $jThreadsInfo#c1
>  < 354> read packet: $[{"name":"main.exe","reason":"trace","registers":{"16":"dc6b5a9af77f0000","6":"0000000000000000","7":"80fa1e8aca000000"}],"tid":23108}],{"description":"Exception 0x80000003 encountered at address 0x7ffa1701e370","name":"main.exe","reason":"exception","registers":{"16":"71e30117fa7f0000","6":"0000000000000000","7":"28fa4f8aca000000"}],"tid":23716}]]#f0
> 
> Process 27544 stopped
> * thread #1, name = 'main.exe', stop reason = trace
>     frame #0: 0x00007ff79a5a6bdc main.exe`main at main.cpp:7
>    4    {
>    5
>    6      printf("abc");
> -> 7      while(1);
>    8      return 1;
>    9    }
>   thread #2, name = 'main.exe', stop reason = Exception 0x80000003 encountered at address 0x7ffa1701e370
>     frame #0: 0x00007ffa1701e371
> ->  0x7ffa1701e371: retq
>     0x7ffa1701e372: int3
>     0x7ffa1701e373: int3
>     0x7ffa1701e374: int3
> ```
> 
Ok, I see. Hiding the thread sounds like the right thing to do, though I'm not sure if it should be done at lldb-server level. My feeling is it should be done at a higher level, because lldb-server should report the thing that the OS sees, and the OS definitely sees two threads here. It may be better to hide this in the higher levels of the debugger (unless the user has requested some sort of a verbose view).

If we go down that path, then I think it would be worth it to update these tests to have the correct expectation, but it does not have to happen now. We can leave them XFAILed for the time being. Thanks for explaining.


================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/inferior-crash/TestGdbRemoteAbort.py:40
 
+    @skipIfWindows # For now the signo in T* packet is always 0.
     @llgs_test
----------------
Hui wrote:
> labath wrote:
> > labath wrote:
> > > Do you have any plans for changing this? Given that windows does not support signals, maybe it should stay 0...
> > I'd like to hear your thoughts on this...
> > 
> > If the idea is for the signal number to stay zero, then the comment shouldn't say "for now" but instead say something to the effect that abort() does not cause a signal to be raised on windows because windows doesn't have signals. If the idea is to change this later, then I'd like to understand how/why.
> I think the signal number can just stay zero (treated as invalid) unless there is any strategy that will
> need valid ones in the future for Windows user-mode debugging(or for kernel-mode debugging?). 
> The proposed implementation of native process/thread for windows in D56233 is mainly signal free. 
> 
> There will be several slight difference(worth mentioning) by always filling with zero signo
> 
> (1) In remote debugging case, T* packet is to detail the stopped reason for a thread.
> W or w/o a valid signal the messages shown on lldb  are slightly different.
> (StopReason::eStopReasonBreakpoint vs StopReason::eStopReasonException)
> 
> On Linux,
> 
>  thread #1, name = 'a.out', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
> 
> On Windows,
>  thread #1, name = 'a.out', stop reason = Exception 0xc0000005 encountered at address 0x7ff6011c1093
>  
> (2) The GDB remote serial protocol does have several signals related packets,
> like **"vCont;S"** to resume a thread by single stepping with signal or** "vCont;C"** to continue with signal.
> For example, **vCont;S06** to single step with signo=6 (SIGABRT).
> 
> Such packets won't be available on Windows.  The continuation of a stopped thread relies on how
> the exception is dispatched and handled.  This is mentioned at
> 
> https://docs.microsoft.com/en-us/windows/desktop/debug/exception-dispatching
> 
> In this python test case, if a segfault happens, the EXCEPTION_ACCESS_VIOLATION (0xc0000005) is raised.
> Since there is no eh installed in user application, the exception will be dispatched to lldb Debugger for the second time
> where a default eh or ExitProcess will be called.  ( D56233 patch might need such changes)
> 
> 
> thread #1, name = 'a.out', stop reason = Exception 0xc0000005 encountered at address 0x7ff6011c1093
That sounds like the right thing to report, as that is what actually happened.

> vCont;S06
Yeah, I think lldb-server on windows should just not support these packets (return an error or something). I don't know if there is a concept of "injecting" exceptions on windows, but if there is, we can have a discussion later on how to implement that.

So overall I think skipping this test is the right thing to do. I just think the message giving the reason could be a bit better. Maybe just "abort() does not send a signal on windows" ?


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

https://reviews.llvm.org/D61687





More information about the lldb-commits mailing list