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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteKill.py:17
     def attach_commandline_kill_after_initial_stop(self):
+        reg_expr = r"^\$X[0-9a-fA-F]+([^#]*)#[0-9A-Fa-f]{2}"
+        triple = self.dbg.GetSelectedPlatform().GetTriple()
----------------
labath wrote:
> 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)
> ```
Just make one if there can be two letters?:

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



================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteKill.py:20-23
+        # No signal support on Windwos. Only W* response is sent.
+        if re.match(".*-.*-windows", triple):
+            reg_expr = r"^\$W[0-9a-fA-F]+([^#]*)#[0-9A-Fa-f]{2}"
+
----------------
Remove


================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteModuleInfo.py:22
 
+        module_path = lldbutil.append_to_process_working_directory(self, "a.out")
+
----------------
Use json.dumps:
```
module_path = json.dumps(lldbutil.append_to_process_working_directory(self, "a.out"))
```


================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteModuleInfo.py:24-27
+        # Replace path separators in the json string either with "\\\\" or "/" on Windows.
+        triple = self.dbg.GetSelectedPlatform().GetTriple()
+        if re.match(".*-.*-windows", triple):
+            module_path = module_path.replace(os.path.sep, '/')
----------------
labath wrote:
> 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.
Remove


================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteSingleStep.py:23
 
+    @skipIfWindows # No pty support to test any inferior std -i/e/o
     @llgs_test
----------------
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.


================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemote_qThreadStopInfo.py:59-61
+        # On Windows, there could be more threads spawned. For example, DebugBreakProcess will
+        # create a new thread from the debugged process to handle an exception event. So here we
+        # assert 'GreaterEqual' condition.
----------------
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).


================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemote_vCont.py:108
 
+    @skipIfWindows # No pty support to test O* & I* notification packets.
     @llgs_test
----------------
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.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D61687