Page MenuHomePhabricator

Update Python tests for lldb-server on Windows
ClosedPublic

Authored by asmith on May 8 2019, 8:44 AM.

Diff Detail

Event Timeline

asmith created this revision.May 8 2019, 8:44 AM
Herald added a project: Restricted Project. · View Herald Transcript
labath added inline comments.May 9 2019, 3:45 AM
packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteKill.py
17–23

If I understand correctly, the regexes differ only in the first letter. If that's the case, then you could just factor out the rest of the regex and just compute the letter differently:

stop_type = 'W' if windows else 'X'
reg_expr = r"^\${}...".format(stop_type)
packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteModuleInfo.py
24–27

It sounds like you could just unconditionally replace all backslashes with double-backslashes here. That would result in us also correctly handling posix paths that happen to contain a backslash.

packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteProcessInfo.py
178–191

I'm not sure why the original test was made linux-specific, but I think you can just drop the os name from the test and have a single test which checks for both the presence of parent-pid and triple. No @skipUnless needed as all platforms that currently have a working lldb-server should support these fields.

packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py
208–212

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).

packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemote_qThreadStopInfo.py
146–153

I think this assertion should just be deleted. If the assertion below (that one thread has a stop reason) is true, then it trivially true that the rest of the threads don't have one. Whether or not a platforms spawns an extra thread does not seem to be relevant for this test.

packages/Python/lldbsuite/test/tools/lldb-server/inferior-crash/TestGdbRemoteAbort.py
40

Do you have any plans for changing this? Given that windows does not support signals, maybe it should stay 0...

clayborg requested changes to this revision.May 9 2019, 8:23 AM
clayborg added a subscriber: clayborg.
clayborg added inline comments.
packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteKill.py
17

Just make one if there can be two letters?:

reg_expr = r"^\$[XW][0-9a-fA-F]+([^#]*)#[0-9A-Fa-f]{2}"

20–23

Remove

packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteModuleInfo.py
22

Use json.dumps:

module_path = json.dumps(lldbutil.append_to_process_working_directory(self, "a.out"))
24–27

Remove

packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteSingleStep.py
23

We probably don't need pty support just to support STDIO redirection to a child process we spawn. That is how it is done for linux, but doens't need to be how it is done on windows. Be great if we can fix this.

packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemote_qThreadStopInfo.py
59–61

Will this always be "thread_count+1" for window? Or can there be more threads.

This leads to a higher level question: Does the user ever want to see the "DebugBreakProcess" thread in the IDE? Should we just never report this thread from lldb-server and avoid these mismatch issues? We should be consistent with what other windows debuggers do (show DebugBreakProcess or not).

packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemote_vCont.py
108

We probably don't need pty support just to support STDIO redirection to a child process we spawn. That is how it is done for linux, but doens't need to be how it is done on windows. Be great if we can fix this.

This revision now requires changes to proceed.May 9 2019, 8:23 AM
Hui added a subscriber: Hui.May 13 2019, 8:58 PM
Hui added inline comments.
packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteModuleInfo.py
24–27

The 'jModulesInfo' packet is a json string. I tested with json.loads as follows.
It seemed to me that module_path needs to be escaped, i.e. 'd:\\\\abc' or be 'd:/abc'.

not-working case:

>>> module_path = 'd:\abc'
>>> json.dumps(module_path)
'"d:\\u0007bc"'
>>> json.loads('[{"[file":"%s"}]' % json.dumps(module_path))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/json/__init__.py", line 339, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python2.7/json/decoder.py", line 364, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib/python2.7/json/decoder.py", line 380, in raw_decode
    obj, end = self.scan_once(s, idx)
ValueError: Expecting , delimiter: line 1 column 13 (char 12)

working case:

>>> module_path = 'd:\\\\abc'
>>> json.loads('[{"[file":"%s"}]' % module_path)
[{u'[file': u'd:\\abc'}]
labath added inline comments.May 14 2019, 12:10 AM
packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteModuleInfo.py
24–27

There are multiple levels of quoting happening here, and I believe you're getting them mixed up:

>>> module_path = 'd:\abc'

This is already wrong, because python will interpret the \a as the ASCII BEL character, resulting in the string consisting of: 'd', ':', BEL, 'b', 'c'

>>> "d:\abc"
'd:\x07bc'

The easiest fix is to use "raw" python strings:

r"d:\abc"
'd:\\abc'

Note that the '\' in the output string it's not doubled, it is just how the python dumper makes sure the output is unambiguous. You can for instance check that with: len(r"d:\abc") which returns 6 as expected.

After that, your not-working case works almost fine:

>>> json.loads('[{"[file":"%s"}]' % json.dumps(module_path))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python3.6/json/__init__.py", line 354, in loads
    return _default_decoder.decode(s)
  File "/usr/lib64/python3.6/json/decoder.py", line 339, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib64/python3.6/json/decoder.py", line 355, in raw_decode
    obj, end = self.scan_once(s, idx)
json.decoder.JSONDecodeError: Expecting ',' delimiter: line 1 column 13 (char 12)
>>> json.loads('[{"[file":%s}]' % json.dumps(module_path))
[{'[file': 'd:\\abc'}]

Note I removed the superfluous quotes in the second attempt. However, if we're going to be using the json package for this, then there's an even simpler way to write this:

>>> json.dumps([{"file": module_path}])
'[{"file": "d:\\\\abc"}]'
>>> json.loads(json.dumps([{"file": module_path}]))
[{'file': 'd:\\abc'}]
asmith updated this revision to Diff 199554.May 14 2019, 9:43 PM
asmith marked 13 inline comments as done.May 14 2019, 9:46 PM
labath added inline comments.May 15 2019, 2:25 AM
packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteProcessInfo.py
213–225

These two tests can also be merged and made @skipIfDarwin as that is the only platform using the cpu(sub)type fields.

packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py
208–212

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?

packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemote_qThreadStopInfo.py
146–153

This comment doesn't appear to be "done".

packages/Python/lldbsuite/test/tools/lldb-server/inferior-crash/TestGdbRemoteAbort.py
40

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.

clayborg added inline comments.May 15 2019, 9:29 AM
packages/Python/lldbsuite/test/dotest.py
1313

Update comment

rnk removed a reviewer: rnk.May 15 2019, 4:36 PM
Hui added inline comments.May 15 2019, 6:28 PM
packages/Python/lldbsuite/test/tools/lldb-server/inferior-crash/TestGdbRemoteAbort.py
40

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)

Hui added inline comments.May 15 2019, 8:24 PM
packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py
208–212

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

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.

packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py
208–212

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.

packages/Python/lldbsuite/test/tools/lldb-server/inferior-crash/TestGdbRemoteAbort.py
40

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" ?

asmith updated this revision to Diff 200160.May 18 2019, 5:03 PM
asmith marked 4 inline comments as done.May 18 2019, 5:07 PM
labath accepted this revision.May 20 2019, 12:38 AM

Ok, let's give this a shot.

asmith updated this revision to Diff 214985.Aug 13 2019, 5:00 PM
asmith edited the summary of this revision. (Show Details)

Any more comments on the tests for Windows?

This revision was not accepted when it landed; it landed in state Needs Review.Aug 13 2019, 5:15 PM
Closed by commit rL368776: Update Python tests for lldb-server on Windows (authored by asmith, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 5:15 PM