Fix process launch failure on FreeBSD after r365761

[dim]

Summary

After rLLDB365761, and with LLVM_ENABLE_ABI_BREAKING_CHECKS enabled, launching
any process on FreeBSD crashes lldb with:

Expected<T> must be checked before access or destruction.
Expected<T> value was in success state. (Note: Expected<T> values in success mode must still be checked prior to being destroyed).

This is because m_operation_thread and m_monitor_thread were wrapped in
llvm::Expected<>, but this requires the objects to be correctly initialized
before accessing them.

To fix the crashes, revert the wrapping, and use local variables to store the
return values of LaunchThread and StartMonitoringChildProcess. Then, only
assign to the member variables after checking if the return values indicated
success.

[dim]

This comment was removed by dim.

[MaskRay]

This was probably due to:

  Expected(Error Err)
      : HasError(true)
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
        // Expected is unchecked upon construction in Debug builds.
        , Unchecked(true)
#endif
  {
    assert(Err && "Cannot create Expected<T> from Error success value.");
    new (getErrorStorage()) error_type(Err.takePayload());
  }

The member initialization makes the members unchecked m_operation_thread(nullptr). Adding something like (void)!m_operation_thread before assignment to m_operation_thread is probably better.

[devnexen]

source/Plugins/Process/FreeBSD/ProcessMonitor.cpp

733

nit: Would prefer obvious typing but can go along with it, just a detail.

[devnexen]

LGTM otherwise.

[MaskRay]

In D68723#1703051, @devnexen wrote:

LGTM otherwise.

I don't think this change should be reverted. It can just be repaired by adding some (void)!xxx;

[labath]

I don't think having Expected<T> members is a reasonable thing to do. If we want to keep the notion of validity explicit (which is consistent with the direction lldb is moving in), then I think the members should be Optional<T>. The initialization code can take the Expected<T> result, do something with the error, and then emplace the result into the Optional<T> member in case of success.

[devnexen]

In D68723#1703098, @MaskRay wrote:

In D68723#1703051, @devnexen wrote:

LGTM otherwise.

I don't think this change should be reverted. It can just be repaired by adding some (void)!xxx;

In D68723#1703098, @MaskRay wrote:

In D68723#1703051, @devnexen wrote:

LGTM otherwise.

I don't think this change should be reverted. It can just be repaired by adding some (void)!xxx;

I would prefer this approach indeed but either way I m in that s a real issue at the moment since LLVM runtime check is on in the FreeBSD's system.

[devnexen]

@dim I LGTMed this but would it a real issue for you to take on a less disruptive approach proposed by MaskRay ?

[MaskRay]

In D68723#1703322, @devnexen wrote:

@dim I LGTMed this but would it a real issue for you to take on a less disruptive approach proposed by MaskRay ?

I think we should do what @labath suggested (changing Expected to Optional) if that is not very difficult.

[devnexen]

In D68723#1703379, @MaskRay wrote:

In D68723#1703322, @devnexen wrote:

@dim I LGTMed this but would it a real issue for you to take on a less disruptive approach proposed by MaskRay ?

I think we should do what @labath suggested (changing Expected to Optional) if that is not very difficult.

Good point, I missed it. Should be alright to.

[dim]

Convert m_(monitor|operation)_thread to llvm::Optional<>.

source/Plugins/Process/FreeBSD/ProcessMonitor.h

public:
   void StopMonitor();
 
   // Waits for the initial stop message from a new thread.
   bool WaitForInitialTIDStop(lldb::tid_t tid);
 
 private:
   ProcessFreeBSD *m_process;
 
-  llvm::Expected<lldb_private::HostThread> m_operation_thread;
-  llvm::Expected<lldb_private::HostThread> m_monitor_thread;
+  lldb_private::HostThread m_operation_thread;
+  lldb_private::HostThread m_monitor_thread;
   lldb::pid_t m_pid;
 
   int m_terminal_fd;
 
   // current operation which must be executed on the privileged thread
   Operation *m_operation;
   std::mutex m_operation_mutex;
 

source/Plugins/Process/FreeBSD/ProcessMonitor.cpp

 ProcessMonitor::ProcessMonitor(
     ProcessFreeBSD *process, Module *module, const char *argv[],
     Environment env, const FileSpec &stdin_file_spec,
     const FileSpec &stdout_file_spec, const FileSpec &stderr_file_spec,
     const FileSpec &working_dir,
     const lldb_private::ProcessLaunchInfo & /* launch_info */,
     lldb_private::Status &error)
     : m_process(static_cast<ProcessFreeBSD *>(process)),
-      m_operation_thread(nullptr), m_monitor_thread(nullptr), m_pid(LLDB_INVALID_PROCESS_ID), m_terminal_fd(-1), m_operation(0) {
+      m_operation_thread(), m_monitor_thread(), m_pid(LLDB_INVALID_PROCESS_ID), m_terminal_fd(-1), m_operation(0) {
   using namespace std::placeholders;
 
   std::unique_ptr<LaunchArgs> args(
       new LaunchArgs(this, module, argv, std::move(env), stdin_file_spec,
                      stdout_file_spec, stderr_file_spec, working_dir));
 
   sem_init(&m_operation_pending, 0, 0);
   sem_init(&m_operation_done, 0, 0);
ProcessMonitor::ProcessMonitor(
   // Check that the launch was a success.
   if (!args->m_error.Success()) {
     StopOpThread();
     error = args->m_error;
     return;
   }
 
   // Finally, start monitoring the child process for change in state.
-  m_monitor_thread = Host::StartMonitoringChildProcess(
+  auto monitor_thread = Host::StartMonitoringChildProcess(
       std::bind(&ProcessMonitor::MonitorCallback, this, _1, _2, _3, _4),
       GetPID(), true);
-  if (!m_monitor_thread->IsJoinable()) {
+  if (!monitor_thread || !monitor_thread->IsJoinable()) {
     error.SetErrorToGenericError();
     error.SetErrorString("Process launch failed.");
     return;
   }
+  m_monitor_thread = *monitor_thread;
 }
 
 ProcessMonitor::ProcessMonitor(ProcessFreeBSD *process, lldb::pid_t pid,
                                lldb_private::Status &error)
     : m_process(static_cast<ProcessFreeBSD *>(process)),
-      m_operation_thread(nullptr), m_monitor_thread(nullptr), m_pid(pid), m_terminal_fd(-1), m_operation(0) {
+      m_operation_thread(), m_monitor_thread(), m_pid(pid), m_terminal_fd(-1), m_operation(0) {
   using namespace std::placeholders;
 
   sem_init(&m_operation_pending, 0, 0);
   sem_init(&m_operation_done, 0, 0);
 
   std::unique_ptr<AttachArgs> args(new AttachArgs(this, pid));
 
   StartAttachOpThread(args.get(), error);
   if (!error.Success())
     return;
 
   if (llvm::sys::RetryAfterSignal(-1, sem_wait, &args->m_semaphore) == -1) {
     error.SetErrorToErrno();
     return;
   }
 
   // Check that the attach was a success.
   if (!args->m_error.Success()) {
     StopOpThread();
     error = args->m_error;
     return;
   }
 
   // Finally, start monitoring the child process for change in state.
-  m_monitor_thread = Host::StartMonitoringChildProcess(
+  auto monitor_thread = Host::StartMonitoringChildProcess(
       std::bind(&ProcessMonitor::MonitorCallback, this, _1, _2, _3, _4),
       GetPID(), true);
-  if (!m_monitor_thread->IsJoinable()) {
+  if (!monitor_thread || !monitor_thread->IsJoinable()) {
     error.SetErrorToGenericError();
     error.SetErrorString("Process attach failed.");
     return;
   }
+  m_monitor_thread = *monitor_thread;
 }
 
 ProcessMonitor::~ProcessMonitor() { StopMonitor(); }
 
 // Thread setup and tear down.
 void ProcessMonitor::StartLaunchOpThread(LaunchArgs *args, Status &error) {
   static const char *g_thread_name = "lldb.process.freebsd.operation";
 
-  if (m_operation_thread->IsJoinable())
+  if (m_operation_thread.IsJoinable())
     return;
 
-  m_operation_thread =
+  auto operation_thread =
       ThreadLauncher::LaunchThread(g_thread_name, LaunchOpThread, args);
-  if (!m_operation_thread)
-     error = m_operation_thread.takeError();
+  if (operation_thread)
+    m_operation_thread = *operation_thread;
+  else
+    error = operation_thread.takeError();
 }
 
 void *ProcessMonitor::LaunchOpThread(void *arg) {
   LaunchArgs *args = static_cast<LaunchArgs *>(arg);
 
   if (!Launch(args)) {
     sem_post(&args->m_semaphore);
     return NULL;

 FINISH:
   return args->m_error.Success();
 }
 
 void ProcessMonitor::StartAttachOpThread(AttachArgs *args,
                                          lldb_private::Status &error) {
   static const char *g_thread_name = "lldb.process.freebsd.operation";
 
-  if (m_operation_thread->IsJoinable())
+  if (m_operation_thread.IsJoinable())
     return;
 
-  m_operation_thread =
+  auto operation_thread =
       ThreadLauncher::LaunchThread(g_thread_name, AttachOpThread, args);
-
-  if (!m_operation_thread)
-  error = m_operation_thread.takeError();
+  if (operation_thread)
+    m_operation_thread = *operation_thread;
+  else
+    error = operation_thread.takeError();
 }
 
 void *ProcessMonitor::AttachOpThread(void *arg) {
   AttachArgs *args = static_cast<AttachArgs *>(arg);
 
   Attach(args);
 
   ServeOperation(args);
bool ProcessMonitor::DupDescriptor(const FileSpec &file_spec, int fd,
 
   if (dup2(target_fd, fd) == -1)
     return false;
 
   return (close(target_fd) == -1) ? false : true;
 }
 
 void ProcessMonitor::StopMonitoringChildProcess() {
-  if (m_monitor_thread->IsJoinable()) {
-    m_monitor_thread->Cancel();
-    m_monitor_thread->Join(nullptr);
-    m_monitor_thread->Reset();
+  if (m_monitor_thread.IsJoinable()) {
+    m_monitor_thread.Cancel();
+    m_monitor_thread.Join(nullptr);
+    m_monitor_thread.Reset();
   }
 }
 
 void ProcessMonitor::StopMonitor() {
   StopMonitoringChildProcess();
   StopOpThread();
   sem_destroy(&m_operation_pending);
   sem_destroy(&m_operation_done);

 // logically needed.
 //
 // We really should figure out what actually happens on FreeBSD and move the
 // Linux-specific logic out of ProcessPOSIX as needed.
 
 bool ProcessMonitor::WaitForInitialTIDStop(lldb::tid_t tid) { return true; }
 
 void ProcessMonitor::StopOpThread() {
-  if (!m_operation_thread->IsJoinable())
+  if (!m_operation_thread.IsJoinable())
     return;
 
-  m_operation_thread->Cancel();
-  m_operation_thread->Join(nullptr);
-  m_operation_thread->Reset();
+  m_operation_thread.Cancel();
+  m_operation_thread.Join(nullptr);
+  m_operation_thread.Reset();
 }