[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3105
+
+    m_arguments_field = AddArgumentsField();
+    m_enviroment_field = AddEnvironmentVariableListField();
----------------
We should fill in any arguments automatically into this field from the target and let the user modify them as needed.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3106
+    m_arguments_field = AddArgumentsField();
+    m_enviroment_field = AddEnvironmentVariableListField();
+
----------------
We should fill in any current environment variables from the target if we aren't already doing this.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3110
+
+    m_stop_at_entry_field = AddBooleanField("Stop at entry point.", false);
+    m_working_directory_field =
----------------
Set default values for all fields in here from the target if/when possible.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3123
+    m_standard_output_field =
+        AddFileField("Standard Output", "", /*need_to_exist=*/false,
+                     /*required=*/false);
----------------
Maybe label as "Redirect Standard Output" or "Standard Output File"?


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3126
+    m_standard_error_field =
+        AddFileField("Standard Error", "", /*need_to_exist=*/false,
+                     /*required=*/false);
----------------
If you change above field, then modify this one as well.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3129
+    m_standard_input_field =
+        AddFileField("Standard Input", "", /*need_to_exist=*/false,
+                     /*required=*/false);
----------------
If you change above field, then modify this one as well.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3140
+      m_stop_at_entry_field->FieldDelegateShow();
+      m_working_directory_field->FieldDelegateShow();
+      m_disable_aslr_field->FieldDelegateShow();
----------------
I'd pull working directory out of advanced settings.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3163
+
+  void SetArguments(ProcessLaunchInfo &launch_info) {
+    TargetSP target = m_debugger.GetSelectedTarget();
----------------
Should this be labeled "GetArguments"? The name seems like it would set the arguments in this class from launch_info, but it is getting them from this class and filling them into "launch_info". Ditto for all accessors that fill in "launch_info" below.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3237-3240
+    } else {
+      action.Open(STDIN_FILENO, dev_null, true, false);
+      launch_info.AppendFileAction(action);
+    }
----------------
We don't need to do anything if this isn't specified as by default the input will be hooked up by the debugging to something valid. If the users wants to redirect to /dev/null, they can just specify "/dev/null" (or the right path for this on their system.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3246-3249
+    } else {
+      action.Open(STDOUT_FILENO, dev_null, false, true);
+      launch_info.AppendFileAction(action);
+    }
----------------
Ditto above, don't do anything if this isn't specified.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3255-3258
+    } else {
+      action.Open(STDERR_FILENO, dev_null, false, true);
+      launch_info.AppendFileAction(action);
+    }
----------------
Ditto above, don't do anything if this isn't specified.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3264-3265
+
+    if (target->GetInheritTCC())
+      launch_info.GetFlags().Set(eLaunchFlagInheritTCCFromParent);
+
----------------
This is an obscure MacOS specific setting, so no need to make a field of it like suggested below.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3267-3268
+
+    if (target->GetDetachOnError())
+      launch_info.GetFlags().Set(eLaunchFlagDetachOnError);
+
----------------
We should make a Boolean input field in the advanced settings, and the default should be set to "target->GetDetachOnError()". Then the user can modify this if needed.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3270-3271
+
+    if (target->GetDisableSTDIO())
+      launch_info.GetFlags().Set(eLaunchFlagDisableSTDIO);
+  }
----------------
This should be moved above all of the STDIO redirection stuff above since it will cause everything to be redirected to /dev/null or equivalent. We should make a boolean setting for this in the advanced stuff, and if this is set to true, then don't show the output redirection fields (m_standard_input_field, m_standard_output_field and m_standard_error_field) in UpdateFieldsVisibility()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107869