[Lldb-commits] [PATCH] D104413: Fixed use of -o and -k in LLDB under Windows when statically compiled with vcruntime.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

Could you move this function into the `Debugger` class and just make `SBDebugger` forward to that function? We usually keep the `SB*` classes as thin wrappers that only contain binding specific logic and have the actual implementation in the sibling class without the `SB` prefix.

The only other thing that's missing is adding this new function to the `./lldb/bindings/interface/SBDebugger.i` interface file (which SWIG is using to generate Python bindings) and add a `/API/` test for it. It's enough to pipe something like `version\nhelp\n` into the `self.dbg` instance and check the output (and then try with a default-constructed SBStream too).

Beside that this looks fine. Thanks for fixing this, that is a really nasty bug. I'll make a patch that adds a notice to all the existing `fd`/`FILE` based SB APIs so that the next person that will be hit by this doesn't have to track this down to this wonky behaviour.



================
Comment at: lldb/source/API/SBDebugger.cpp:364
+
+  ssize_t nrwr = write(fds[WRITE], stream.GetData(), stream.GetSize());
+  if (size_t(nrwr) != stream.GetSize()) {
----------------
I think this is unspecified behaviour for a default constructed `SBStream`, so please make an early-exit at the top when `!stream.IsValid()`


================
Comment at: lldb/source/API/SBDebugger.cpp:394
+    return result;
+  }
+
----------------
no `{}` because it's just one line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104413