Fixed use of -o and -k in LLDB under Windows when statically compiled with vcruntime.

[PatriosTheGreat]

Summary

Right now if the LLDB is compiled under the windows with static vcruntime library, the -o and -k commands will not work.

The problem is that the LLDB create FILE* in lldb.exe and pass it to liblldb.dll which is an object from CRT.
Since the CRT is statically linked each of these module has its own copy of the CRT with it's own global state and the LLDB should not share CRT objects between them.

In this change I moved the logic of creating FILE* out of commands stream from Driver class to SBDebugger.
To do this I added new method: SBError SBDebugger::SetInputStream(SBStream &stream)

Command to build the LLDB:
cmake -G Ninja -DLLVM_ENABLE_PROJECTS="clang;lldb;libcxx" -DLLVM_USE_CRT_RELEASE="MT" -DLLVM_USE_CRT_MINSIZEREL="MT" -DLLVM_USE_CRT_RELWITHDEBINFO="MT" -DP
YTHON_HOME:FILEPATH=C:/Python38 -DCMAKE_C_COMPILER:STRING=cl.exe -DCMAKE_CXX_COMPILER:STRING=cl.exe ../llvm

Command which will fail:
lldb.exe -o help

See discord discussion for more details: https://discord.com/channels/636084430946959380/636732809708306432/854629125398724628
This revision is for the further discussion.

[teemperor]

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.

lldb/source/API/SBDebugger.cpp

364

I think this is unspecified behaviour for a default constructed SBStream, so please make an early-exit at the top when !stream.IsValid()

394

no {} because it's just one line.

[shafik]

lldb/source/API/SBDebugger.cpp

392

What case is this catching?

[PatriosTheGreat]

Hi everyone.
Thanks for review.
In order to move new method code to Debugger class I also had to move there SetInputFile(lldb::FileSP file) method.
After I did that -- the reproducer tests started to fail.
If I understand correctly there is a problem to deserialize SBStream object during the reply session. In previous patch the replay tests pass since I was calling SetInputFile API method from SetInputStream.
To fix that in this patch I replaced SetInputStream method with SetInputData (const char* data, size_t size); which takes a raw chat array instead of SBStream.

[PatriosTheGreat]

Should I send this somehow back to review?

[teemperor]

In D104413#2952057, @PatriosTheGreat wrote:

Should I send this somehow back to review?

Nope, this is (as it should be) in the review queue but I just didn't get around to this yet. Sorry for the delay, I'll try to take a look at this today. Feel free to ping sooner in the future, LLVM policy says a week delay justifies a ping :)

[teemperor]

Sorry for the delay on this! Feel free to ping sooner if this gets stuck again.

I think just passing a string here seems fine, but maybe we could let the function just take a C-string and call it SetInputString (Both in SBDebugger and Debugger)? The rest of the code anyway doesn't handle 0 bytes so we might as well just use a C-string for that function and avoid the whole string length thing (that just seems awkward in Python scripts).

Beside that this seems fine. Some documentation for the SB API part would be appreciated but I won't insist on it.

lldb/include/lldb/Core/Debugger.h

181 ↗

(On Diff #353875)

Please add a note that this automatically sets up a DataRecorder depending on the current reproducer mode.

lldb/source/Core/Debugger.cpp

833 ↗

(On Diff #353875)

I think I could have added more info on my previous inline comment: write(PIPE, ..., 0) is unspecified so that's why I asked for that early exit on the previous version with SBStream. So now just early-exiting when data is null or size is 0 would workaround that.

[PatriosTheGreat]

Sorry for delay from my side also

[PatriosTheGreat]

Friendly ping =)

[teemperor]

LGTM, thanks!

lldb/test/API/python_api/file_handle/TestFileHandle.py

920 ↗

(On Diff #371002)

Can you drop this line? Downstream tends to patch the version output with their own branding. Maybe help command alias and then check for Define a custom command in terms of an existing command.

Also I think you can use self.assertIn('Show a list of all debugger commands', output) here.

[PatriosTheGreat]

Thanks for review!

Could you or someone else take this commit to master?
I don't have a commit permissions.

lldb/include/lldb/API/SBDebugger.h

 //===-- SBDebugger.h --------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_API_SBDEBUGGER_H
 #define LLDB_API_SBDEBUGGER_H
 
 #include <cstdio>
 
 #include "lldb/API/SBDefines.h"
 #include "lldb/API/SBPlatform.h"
 
 namespace lldb {
 
 class LLDB_API SBInputReader {
 public:
   SBInputReader() = default;
   ~SBInputReader() = default;
public:
   void SetErrorFileHandle(FILE *f, bool transfer_ownership);
 
   FILE *GetInputFileHandle();
 
   FILE *GetOutputFileHandle();
 
   FILE *GetErrorFileHandle();
 
+  SBError SetInputStream(SBStream &stream);
+
   SBError SetInputFile(SBFile file);
 
   SBError SetOutputFile(SBFile file);
 
   SBError SetErrorFile(SBFile file);
 
   SBError SetInputFile(FileSP file);
 

lldb/source/API/SBDebugger.cpp

 #include "lldb/Target/TargetList.h"
 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/State.h"
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DynamicLibrary.h"
 #include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/Process.h"
+
+// Includes for pipe()
+#if defined(_WIN32)
+#include <fcntl.h>
+#include <io.h>
+#else
+#include <unistd.h>
+#endif
 
 using namespace lldb;
 using namespace lldb_private;
 
 static llvm::sys::DynamicLibrary LoadPlugin(const lldb::DebuggerSP &debugger_sp,
                                             const FileSpec &spec,
                                             Status &error) {
   llvm::sys::DynamicLibrary dynlib =
if (dynlib.isValid()) {
     else
       error.SetErrorString("no such file");
   }
   return llvm::sys::DynamicLibrary();
 }
 
 static llvm::ManagedStatic<SystemLifetimeManager> g_debugger_lifetime;
 
+static inline int OpenPipe(int fds[2], std::size_t size) {
+#ifdef _WIN32
+  return _pipe(fds, size, O_BINARY);
+#else
+  (void)size;
+  return pipe(fds);
+#endif
+}
+
 SBError SBInputReader::Initialize(
     lldb::SBDebugger &sb_debugger,
     unsigned long (*callback)(void *, lldb::SBInputReader *,
                               lldb::InputReaderAction, char const *,
                               unsigned long),
     void *a, lldb::InputReaderGranularity b, char const *c, char const *d,
     bool e) {
   LLDB_RECORD_DUMMY(

 }
 
 void SBDebugger::SetInputFileHandle(FILE *fh, bool transfer_ownership) {
   LLDB_RECORD_METHOD(void, SBDebugger, SetInputFileHandle, (FILE *, bool), fh,
                      transfer_ownership);
   SetInputFile((FileSP)std::make_shared<NativeFile>(fh, transfer_ownership));
 }
 
+SBError SBDebugger::SetInputStream(SBStream &stream) {
+  LLDB_RECORD_METHOD(SBError, SBDebugger, SetInputStream, (SBStream &), stream);
+
+  SBError result;
+  enum PIPES { READ, WRITE }; // Indexes for the read and write fds
+  int fds[2] = {-1, -1};
+
+  if (OpenPipe(fds, stream.GetSize()) != 0) {
+    result.SetErrorString(
+        "can't create pipe file descriptors for LLDB commands");
+    return result;
+  }
+
+  ssize_t nrwr = write(fds[WRITE], stream.GetData(), stream.GetSize());
+  if (size_t(nrwr) != stream.GetSize()) {
+    result.SetErrorStringWithFormat(
+        "write(%i, %p, %" PRIu64
+        ") failed (errno = %i) when trying to open LLDB commands pipe",
+        fds[WRITE], static_cast<const void *>(stream.GetData()),
+        static_cast<uint64_t>(stream.GetSize()), errno);
+
+    llvm::sys::Process::SafelyCloseFileDescriptor(fds[READ]);
+    llvm::sys::Process::SafelyCloseFileDescriptor(fds[WRITE]);
+    return result;
+  }
+
+  // Close the write end of the pipe, so that the command interpreter will exit
+  // when it consumes all the data.
+  llvm::sys::Process::SafelyCloseFileDescriptor(fds[WRITE]);
+
+  // Open the read file descriptor as a FILE * that we can return as an input
+  // handle.
+  FILE *commands_file = fdopen(fds[READ], "rb");
+  if (commands_file == nullptr) {
+    result.SetErrorStringWithFormat("fdopen(%i, \"rb\") failed (errno = %i) "
+                                    "when trying to open LLDB commands pipe",
+                                    fds[READ], errno);
+    llvm::sys::Process::SafelyCloseFileDescriptor(fds[READ]);
+    return result;
+  }
+
+  if (result.Fail()) {
+    return result;
+  }
+
+  return LLDB_RECORD_RESULT(
+      SetInputFile((FileSP)std::make_shared<NativeFile>(commands_file, true)));
+}
+
 SBError SBDebugger::SetInputFile(FileSP file_sp) {
   LLDB_RECORD_METHOD(SBError, SBDebugger, SetInputFile, (FileSP), file_sp);
   return LLDB_RECORD_RESULT(SetInputFile(SBFile(file_sp)));
 }
 
 // Shouldn't really be settable after initialization as this could cause lots
 // of problems; don't want users trying to switch modes in the middle of a
 // debugging session.
template <> void RegisterMethods<SBDebugger>(Registry &R) {
   LLDB_REGISTER_STATIC_METHOD(void, SBDebugger, Destroy, (lldb::SBDebugger &));
   LLDB_REGISTER_STATIC_METHOD(void, SBDebugger, MemoryPressureDetected, ());
   LLDB_REGISTER_METHOD_CONST(bool, SBDebugger, IsValid, ());
   LLDB_REGISTER_METHOD_CONST(bool, SBDebugger, operator bool,());
   LLDB_REGISTER_METHOD(void, SBDebugger, SetAsync, (bool));
   LLDB_REGISTER_METHOD(bool, SBDebugger, GetAsync, ());
   LLDB_REGISTER_METHOD(void, SBDebugger, SkipLLDBInitFiles, (bool));
   LLDB_REGISTER_METHOD(void, SBDebugger, SkipAppInitFiles, (bool));
+  LLDB_REGISTER_METHOD(SBError, SBDebugger, SetInputStream, (SBStream &));
   LLDB_REGISTER_METHOD(void, SBDebugger, SetInputFileHandle, (FILE *, bool));
   LLDB_REGISTER_METHOD(FILE *, SBDebugger, GetInputFileHandle, ());
   LLDB_REGISTER_METHOD(FILE *, SBDebugger, GetOutputFileHandle, ());
   LLDB_REGISTER_METHOD(FILE *, SBDebugger, GetErrorFileHandle, ());
   LLDB_REGISTER_METHOD(SBFile, SBDebugger, GetInputFile, ());
   LLDB_REGISTER_METHOD(SBFile, SBDebugger, GetOutputFile, ());
   LLDB_REGISTER_METHOD(SBFile, SBDebugger, GetErrorFile, ());
   LLDB_REGISTER_METHOD(void, SBDebugger, SaveInputTerminalState, ());

lldb/tools/driver/Driver.cpp

 #include "lldb/API/SBReproducer.h"
 #include "lldb/API/SBStream.h"
 #include "lldb/API/SBStringList.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Path.h"
-#include "llvm/Support/Process.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include <algorithm>
 #include <atomic>
 #include <bitset>
 #include <csignal>
 #include <string>
 #include <thread>
 #include <utility>
 
 #include <climits>
 #include <cstdio>
 #include <cstdlib>
 #include <cstring>
 #include <fcntl.h>
 
-// Includes for pipe()
-#if defined(_WIN32)
-#include <fcntl.h>
-#include <io.h>
-#else
-#include <unistd.h>
-#endif
-
 #if !defined(__APPLE__)
 #include "llvm/Support/DataTypes.h"
 #endif
 
 using namespace lldb;
 using namespace llvm;
 
 namespace {
if (python_file_spec.IsValid()) {
       llvm::outs() << "<COULD NOT FIND PATH>\n";
     exiting = true;
     return error;
   }
 
   return error;
 }
 
-static inline int OpenPipe(int fds[2], std::size_t size) {
-#ifdef _WIN32
-  return _pipe(fds, size, O_BINARY);
-#else
-  (void)size;
-  return pipe(fds);
-#endif
-}
-
-static ::FILE *PrepareCommandsForSourcing(const char *commands_data,
-                                          size_t commands_size) {
-  enum PIPES { READ, WRITE }; // Indexes for the read and write fds
-  int fds[2] = {-1, -1};
-
-  if (OpenPipe(fds, commands_size) != 0) {
-    WithColor::error()
-        << "can't create pipe file descriptors for LLDB commands\n";
-    return nullptr;
-  }
-
-  ssize_t nrwr = write(fds[WRITE], commands_data, commands_size);
-  if (size_t(nrwr) != commands_size) {
-    WithColor::error()
-        << format(
-               "write(%i, %p, %" PRIu64
-               ") failed (errno = %i) when trying to open LLDB commands pipe",
-               fds[WRITE], static_cast<const void *>(commands_data),
-               static_cast<uint64_t>(commands_size), errno)
-        << '\n';
-    llvm::sys::Process::SafelyCloseFileDescriptor(fds[READ]);
-    llvm::sys::Process::SafelyCloseFileDescriptor(fds[WRITE]);
-    return nullptr;
-  }
-
-  // Close the write end of the pipe, so that the command interpreter will exit
-  // when it consumes all the data.
-  llvm::sys::Process::SafelyCloseFileDescriptor(fds[WRITE]);
-
-  // Open the read file descriptor as a FILE * that we can return as an input
-  // handle.
-  ::FILE *commands_file = fdopen(fds[READ], "rb");
-  if (commands_file == nullptr) {
-    WithColor::error() << format("fdopen(%i, \"rb\") failed (errno = %i) "
-                                 "when trying to open LLDB commands pipe",
-                                 fds[READ], errno)
-                       << '\n';
-    llvm::sys::Process::SafelyCloseFileDescriptor(fds[READ]);
-    return nullptr;
-  }
-
-  // 'commands_file' now owns the read descriptor.
-  return commands_file;
-}
-
 std::string EscapeString(std::string arg) {
   std::string::size_type pos = 0;
   while ((pos = arg.find_first_of("\"\\", pos)) != std::string::npos) {
     arg.insert(pos, 1, '\\');
     pos += 2;
   }
   return '"' + arg + '"';
 }
#endif
   }
 
   const bool handle_events = true;
   const bool spawn_thread = false;
 
   // Check if we have any data in the commands stream, and if so, save it to a
   // temp file
   // so we can then run the command interpreter using the file contents.
-  const char *commands_data = commands_stream.GetData();
-  const size_t commands_size = commands_stream.GetSize();
-
   bool go_interactive = true;
-  if ((commands_data != nullptr) && (commands_size != 0u)) {
-    FILE *commands_file =
-        PrepareCommandsForSourcing(commands_data, commands_size);
-
-    if (commands_file == nullptr) {
-      // We should have already printed an error in PrepareCommandsForSourcing.
+  if ((commands_stream.GetData() != nullptr) &&
+      (commands_stream.GetSize() != 0u)) {
+    SBError error = m_debugger.SetInputStream(commands_stream);
+    if (error.Fail()) {
+      WithColor::error() << error.GetCString() << '\n';
       return 1;
     }
 
-    m_debugger.SetInputFileHandle(commands_file, true);
-
     // Set the debugger into Sync mode when running the command file. Otherwise
     // command files that run the target won't run in a sensible way.
     bool old_async = m_debugger.GetAsync();
     m_debugger.SetAsync(false);
 
     SBCommandInterpreterRunOptions options;
     options.SetAutoHandleEvents(true);
     options.SetSpawnThread(false);
if (m_option_data.m_batch &&
       return 1;
 
     if (m_option_data.m_batch &&
         results.GetResult() == lldb::eCommandInterpreterResultInferiorCrash &&
         !m_option_data.m_after_crash_commands.empty()) {
       SBStream crash_commands_stream;
       WriteCommandsForSourcing(eCommandPlacementAfterCrash,
                                crash_commands_stream);
-      const char *crash_commands_data = crash_commands_stream.GetData();
-      const size_t crash_commands_size = crash_commands_stream.GetSize();
-      commands_file =
-          PrepareCommandsForSourcing(crash_commands_data, crash_commands_size);
-      if (commands_file != nullptr) {
-        m_debugger.SetInputFileHandle(commands_file, true);
+      SBError error = m_debugger.SetInputStream(crash_commands_stream);
+      if (error.Success()) {
         SBCommandInterpreterRunResult local_results =
             m_debugger.RunCommandInterpreter(options);
         if (local_results.GetResult() ==
             lldb::eCommandInterpreterResultQuitRequested)
           go_interactive = false;
 
         // When running in batch mode and an error occurred while sourcing
         // the crash commands, exit with a non-zero exit status.