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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to