[Lldb-commits] [PATCH] D41070: llgs: Propagate the environment when launching the inferior from command line

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:46
 
-  //------------------------------------------------------------------
-  /// Specify the program to launch and its arguments.
-  ///
-  /// @param[in] args
-  ///     The command line to launch.
-  ///
-  /// @param[in] argc
-  ///     The number of elements in the args array of cstring pointers.
-  ///
-  /// @return
-  ///     An Status object indicating the success or failure of making
-  ///     the setting.
-  //------------------------------------------------------------------
-  Status SetLaunchArguments(const char *const args[], int argc);
-
-  //------------------------------------------------------------------
-  /// Specify the launch flags for the process.
-  ///
-  /// @param[in] launch_flags
-  ///     The launch flags to use when launching this process.
-  ///
-  /// @return
-  ///     An Status object indicating the success or failure of making
-  ///     the setting.
-  //------------------------------------------------------------------
-  Status SetLaunchFlags(unsigned int launch_flags);
+  void SetLaunchInfo(const ProcessLaunchInfo &info);
 
----------------
clayborg wrote:
> We might want a "ProcessLaunchInfo &" accessor so we can modify the flags after they have initially been set?
> 
> ```
> ProcessLaunchInfo &GetLaunchInfo() { return m_process_launch_info; }
> ```
> 
> If we don't need it, we shouldn't add it yet, just thinking out loud here.
We don't need that at the moment. The launching process is a fire-and-forget kind of thing right now.


================
Comment at: tools/lldb-server/lldb-gdbserver.cpp:181-182
+  ProcessLaunchInfo info;
+  info.GetFlags().Set(eLaunchFlagStopAtEntry | eLaunchFlagDebug |
+                      eLaunchFlagDisableASLR);
+  info.SetArguments(const_cast<const char **>(argv), true);
----------------
clayborg wrote:
> How many places do we set these flags like this? Wondering if we should add a method like:
> 
> ```
> info.SetFlagsForDebugging();
> ```
> 
> That way if we add new flags that must be set later, it will be easier than updating all sites that modify these flags for debugging a process.
I've found two places that blindly set the launch flags: here and SBLaunchInfo (the other launches do the dance of consulting the target property for the value). I can make a `eLaunchFlagStandardDebug` which would include `Debug` and `DisableASLR` flags (since we can assume that one normally wants to disable aslr when debugging). However, the StopAtEntry flag could not be there, as it's appropriateness depends on the environment the launch is happening in (here we want it because we need to wait for the client to connect, but this is not appropriate as the SBLaunchInfo default).


https://reviews.llvm.org/D41070