[Lldb-commits] [PATCH] D68723: Fix process launch failure on FreeBSD after r365761

[Dimitry Andric via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa42942e0ecd6: Fix process launch failure on FreeBSD after 
r365761 (authored by dim).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D68723?vs=224447=224460#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68723

Files:
  lldb/source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
  lldb/source/Plugins/Process/FreeBSD/ProcessMonitor.h

Index: lldb/source/Plugins/Process/FreeBSD/ProcessMonitor.h
===
--- lldb/source/Plugins/Process/FreeBSD/ProcessMonitor.h
+++ lldb/source/Plugins/Process/FreeBSD/ProcessMonitor.h
@@ -183,8 +183,8 @@
 private:
   ProcessFreeBSD *m_process;
 
-  llvm::Expected m_operation_thread;
-  llvm::Expected m_monitor_thread;
+  llvm::Optional m_operation_thread;
+  llvm::Optional m_monitor_thread;
   lldb::pid_t m_pid;
 
   int m_terminal_fd;
Index: lldb/source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
===
--- lldb/source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
+++ lldb/source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
@@ -703,7 +703,7 @@
 const lldb_private::ProcessLaunchInfo & /* launch_info */,
 lldb_private::Status )
 : m_process(static_cast(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 args(
@@ -730,20 +730,22 @@
   }
 
   // Finally, start monitoring the child process for change in state.
-  m_monitor_thread = Host::StartMonitoringChildProcess(
+  llvm::Expected monitor_thread =
+Host::StartMonitoringChildProcess(
   std::bind(::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 )
 : m_process(static_cast(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(_operation_pending, 0, 0);
@@ -768,14 +770,16 @@
   }
 
   // Finally, start monitoring the child process for change in state.
-  m_monitor_thread = Host::StartMonitoringChildProcess(
+  llvm::Expected monitor_thread =
+Host::StartMonitoringChildProcess(
   std::bind(::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(); }
@@ -784,13 +788,15 @@
 void ProcessMonitor::StartLaunchOpThread(LaunchArgs *args, Status ) {
   static const char *g_thread_name = "lldb.process.freebsd.operation";
 
-  if (m_operation_thread->IsJoinable())
+  if (m_operation_thread && m_operation_thread->IsJoinable())
 return;
 
-  m_operation_thread =
-  ThreadLauncher::LaunchThread(g_thread_name, LaunchOpThread, args);
-  if (!m_operation_thread)
- error = m_operation_thread.takeError();
+  llvm::Expected operation_thread =
+ThreadLauncher::LaunchThread(g_thread_name, LaunchOpThread, args);
+  if (operation_thread)
+m_operation_thread = *operation_thread;
+  else
+error = operation_thread.takeError();
 }
 
 void *ProcessMonitor::LaunchOpThread(void *arg) {
@@ -952,14 +958,15 @@
  lldb_private::Status ) {
   static const char *g_thread_name = "lldb.process.freebsd.operation";
 
-  if (m_operation_thread->IsJoinable())
+  if (m_operation_thread && m_operation_thread->IsJoinable())
 return;
 
-  m_operation_thread =
-  ThreadLauncher::LaunchThread(g_thread_name, AttachOpThread, args);
-
-  if (!m_operation_thread)
-	error = m_operation_thread.takeError();
+  llvm::Expected operation_thread =
+ThreadLauncher::LaunchThread(g_thread_name, AttachOpThread, args);
+  if (operation_thread)
+m_operation_thread = *operation_thread;
+  else
+error = operation_thread.takeError();
 }
 
 void *ProcessMonitor::AttachOpThread(void *arg) {
@@ -1374,7 +1381,7 @@
 }
 
 void ProcessMonitor::StopMonitoringChildProcess() {
-  if 

[Lldb-commits] [PATCH] D68723: Fix process launch failure on FreeBSD after r365761

[Dimitry Andric via Phabricator via lldb-commits]

dim updated this revision to Diff 224447.
dim added a comment.
Herald added a subscriber: jfb.

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


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

https://reviews.llvm.org/D68723

Files:
  source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
  source/Plugins/Process/FreeBSD/ProcessMonitor.h

Index: source/Plugins/Process/FreeBSD/ProcessMonitor.h
===
--- source/Plugins/Process/FreeBSD/ProcessMonitor.h
+++ source/Plugins/Process/FreeBSD/ProcessMonitor.h
@@ -183,8 +183,8 @@
 private:
   ProcessFreeBSD *m_process;
 
-  llvm::Expected m_operation_thread;
-  llvm::Expected m_monitor_thread;
+  llvm::Optional m_operation_thread;
+  llvm::Optional m_monitor_thread;
   lldb::pid_t m_pid;
 
   int m_terminal_fd;
Index: source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
===
--- source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
+++ source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
@@ -703,7 +703,7 @@
 const lldb_private::ProcessLaunchInfo & /* launch_info */,
 lldb_private::Status )
 : m_process(static_cast(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 args(
@@ -730,20 +730,22 @@
   }
 
   // Finally, start monitoring the child process for change in state.
-  m_monitor_thread = Host::StartMonitoringChildProcess(
+  llvm::Expected monitor_thread =
+Host::StartMonitoringChildProcess(
   std::bind(::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 )
 : m_process(static_cast(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(_operation_pending, 0, 0);
@@ -768,14 +770,16 @@
   }
 
   // Finally, start monitoring the child process for change in state.
-  m_monitor_thread = Host::StartMonitoringChildProcess(
+  llvm::Expected monitor_thread =
+Host::StartMonitoringChildProcess(
   std::bind(::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(); }
@@ -784,13 +788,15 @@
 void ProcessMonitor::StartLaunchOpThread(LaunchArgs *args, Status ) {
   static const char *g_thread_name = "lldb.process.freebsd.operation";
 
-  if (m_operation_thread->IsJoinable())
+  if (m_operation_thread && m_operation_thread->IsJoinable())
 return;
 
-  m_operation_thread =
-  ThreadLauncher::LaunchThread(g_thread_name, LaunchOpThread, args);
-  if (!m_operation_thread)
- error = m_operation_thread.takeError();
+  llvm::Expected operation_thread =
+ThreadLauncher::LaunchThread(g_thread_name, LaunchOpThread, args);
+  if (operation_thread)
+m_operation_thread = *operation_thread;
+  else
+error = operation_thread.takeError();
 }
 
 void *ProcessMonitor::LaunchOpThread(void *arg) {
@@ -952,14 +958,15 @@
  lldb_private::Status ) {
   static const char *g_thread_name = "lldb.process.freebsd.operation";
 
-  if (m_operation_thread->IsJoinable())
+  if (m_operation_thread && m_operation_thread->IsJoinable())
 return;
 
-  m_operation_thread =
-  ThreadLauncher::LaunchThread(g_thread_name, AttachOpThread, args);
-
-  if (!m_operation_thread)
-	error = m_operation_thread.takeError();
+  llvm::Expected operation_thread =
+ThreadLauncher::LaunchThread(g_thread_name, AttachOpThread, args);
+  if (operation_thread)
+m_operation_thread = *operation_thread;
+  else
+error = operation_thread.takeError();
 }
 
 void *ProcessMonitor::AttachOpThread(void *arg) {
@@ -1374,7 +1381,7 @@
 }
 
 void ProcessMonitor::StopMonitoringChildProcess() {
-  if (m_monitor_thread->IsJoinable()) {
+  if (m_monitor_thread && m_monitor_thread->IsJoinable()) {
 m_monitor_thread->Cancel();
 m_monitor_thread->Join(nullptr);
 m_monitor_thread->Reset();
@@ -1412,10 +1419,9 @@
 bool 

[Lldb-commits] [PATCH] D68723: Fix process launch failure on FreeBSD after r365761

[David CARLIER via Phabricator via lldb-commits]

devnexen added a comment.

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.


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

https://reviews.llvm.org/D68723



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

[Lldb-commits] [PATCH] D68723: Fix process launch failure on FreeBSD after r365761

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

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.


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

https://reviews.llvm.org/D68723



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

[Lldb-commits] [PATCH] D68723: Fix process launch failure on FreeBSD after r365761

[David CARLIER via Phabricator via lldb-commits]

devnexen added a comment.

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


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

https://reviews.llvm.org/D68723



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

[Lldb-commits] [PATCH] D68723: Fix process launch failure on FreeBSD after r365761

[David CARLIER via Phabricator via lldb-commits]

devnexen added a comment.

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.


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

https://reviews.llvm.org/D68723



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

[Lldb-commits] [PATCH] D68723: Fix process launch failure on FreeBSD after r365761

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I don't think having Expected 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. 
The initialization code can take the Expected result, do something with the 
error, and then emplace the result into the Optional member in case of 
success.


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

https://reviews.llvm.org/D68723



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

[Lldb-commits] [PATCH] D68723: Fix process launch failure on FreeBSD after r365761

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

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;`


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

https://reviews.llvm.org/D68723



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

[Lldb-commits] [PATCH] D68723: Fix process launch failure on FreeBSD after r365761

[David CARLIER via Phabricator via lldb-commits]

devnexen accepted this revision.
devnexen added a comment.
This revision is now accepted and ready to land.

LGTM otherwise.


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

https://reviews.llvm.org/D68723



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

[Lldb-commits] [PATCH] D68723: Fix process launch failure on FreeBSD after r365761

[David CARLIER via Phabricator via lldb-commits]

devnexen added inline comments.



Comment at: source/Plugins/Process/FreeBSD/ProcessMonitor.cpp:733
   // Finally, start monitoring the child process for change in state.
-  m_monitor_thread = Host::StartMonitoringChildProcess(
+  auto monitor_thread = Host::StartMonitoringChildProcess(
   std::bind(::MonitorCallback, this, _1, _2, _3, _4),

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


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

https://reviews.llvm.org/D68723



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

[Lldb-commits] [PATCH] D68723: Fix process launch failure on FreeBSD after r365761

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

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 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.


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

https://reviews.llvm.org/D68723



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

[Lldb-commits] [PATCH] D68723: Fix process launch failure on FreeBSD after r365761

[Dimitry Andric via Phabricator via lldb-commits]

dim added a comment.

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

https://reviews.llvm.org/D68723



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