[lldb-vscode] Convert launch_info and attach_info to local variables

[anton.kolesov]

Summary

Those fields inside of the global variable can be local variables because
they are used in only inside of one function: request_launch for launch_info
and request_attach for attach_info.

To avoid confusion an already existing local variable attach_info of
request_attach has been renamed to better reflect its purpose.

[labath]

Sounds like a good idea. Could you do the same for attach_info (it looks like it should be possible)? Otherwise, one is left wondering about what's the difference...

[anton.kolesov]

In D76593#1936337, @labath wrote:

Sounds like a good idea. Could you do the same for attach_info (it looks like it should be possible)? Otherwise, one is left wondering about what's the difference...

I have that as my next patch - it seems like something that should be in a separate commit, but in the same pull request, but I don't know how to properly submit patch series for review at Phabricator - only through "related revisions", I presume?

[labath]

In D76593#1936757, @anton.kolesov wrote:

In D76593#1936337, @labath wrote:

Sounds like a good idea. Could you do the same for attach_info (it looks like it should be possible)? Otherwise, one is left wondering about what's the difference...

I have that as my next patch - it seems like something that should be in a separate commit, but in the same pull request, but I don't know how to properly submit patch series for review at Phabricator - only through "related revisions", I presume?

You can (and should) use the "related revisions" functionality to denote the relationship between patches, but the purpose of this relationship is to make it easier to review by explaining what depends on what. We don't really have a concept of a "pull request" consisting of multiple patches, which get all committed as a group. Normally each patch should make sense on its own, even if it's not immediately useful (e.g. because it introduces an interface that is not yet used by anyone). The "related" patches can then explain/demonstrate how it's going to be used, but the preceding patches don't need to wait for a green light on the entire group to be committed. If there's some deviation from this (e.g. a preparatory refactoring patch makes some code substantially more complicated and so it may not be desired if the latter patch is not accepted, or there is some uncertainty about the overall direction of the patchset), it should be called out explicitly.

But none of this is really relevant for this patch, which is so simple you could have done it in one commit (and it could be argued that it *should* be done in a single commit, as doing it partway introduces a somewhat inconsistent state, which is undesirable).

[clayborg]

Yes, this works now that we are creating the target in this function.

[anton.kolesov]

Added attach_info conversion into the same revision.

[labath]

still lgtm, just make sure to clang-format before committing.

lldb/tools/lldb-vscode/VSCode.h

 
 enum VSCodeBroadcasterBits { eBroadcastBitStopEventThread = 1u << 0 };
 
 struct VSCode {
   InputStream input;
   OutputStream output;
   lldb::SBDebugger debugger;
   lldb::SBTarget target;
-  lldb::SBAttachInfo attach_info;
-  lldb::SBLaunchInfo launch_info;
   lldb::SBValueList variables;
   lldb::SBBroadcaster broadcaster;
   int64_t num_regs;
   int64_t num_locals;
   int64_t num_globals;
   std::thread event_thread;
   std::unique_ptr<std::ofstream> log;
   llvm::DenseMap<lldb::addr_t, int64_t> addr_to_source_ref;

lldb/tools/lldb-vscode/VSCode.cpp

 
 using namespace lldb_vscode;
 
 namespace lldb_vscode {
 
 VSCode g_vsc;
 
 VSCode::VSCode()
-    : launch_info(nullptr), variables(), broadcaster("lldb-vscode"),
+    : variables(), broadcaster("lldb-vscode"),
       num_regs(0), num_locals(0), num_globals(0), log(),
       exception_breakpoints(
           {{"cpp_catch", "C++ Catch", lldb::eLanguageTypeC_plus_plus},
            {"cpp_throw", "C++ Throw", lldb::eLanguageTypeC_plus_plus},
            {"objc_catch", "Objective C Catch", lldb::eLanguageTypeObjC},
            {"objc_throw", "Objective C Throw", lldb::eLanguageTypeObjC},
            {"swift_catch", "Swift Catch", lldb::eLanguageTypeSwift},
            {"swift_throw", "Swift Throw", lldb::eLanguageTypeSwift}}),

lldb/tools/lldb-vscode/lldb-vscode.cpp

 //     "description": "Response to 'attach' request. This is just an
 //     acknowledgement, so no body field is required."
 //   }]
 // }
 void request_attach(const llvm::json::Object &request) {
   llvm::json::Object response;
   lldb::SBError error;
   FillResponse(request, response);
+  lldb::SBAttachInfo attach_info;
   auto arguments = request.getObject("arguments");
   const lldb::pid_t pid =
       GetUnsigned(arguments, "pid", LLDB_INVALID_PROCESS_ID);
   if (pid != LLDB_INVALID_PROCESS_ID)
-    g_vsc.attach_info.SetProcessID(pid);
+    attach_info.SetProcessID(pid);
   const auto wait_for = GetBoolean(arguments, "waitFor", false);
-  g_vsc.attach_info.SetWaitForLaunch(wait_for, false /*async*/);
+  attach_info.SetWaitForLaunch(wait_for, false /*async*/);
   g_vsc.init_commands = GetStrings(arguments, "initCommands");
   g_vsc.pre_run_commands = GetStrings(arguments, "preRunCommands");
   g_vsc.stop_commands = GetStrings(arguments, "stopCommands");
   g_vsc.exit_commands = GetStrings(arguments, "exitCommands");
   auto attachCommands = GetStrings(arguments, "attachCommands");
   g_vsc.stop_at_entry = GetBoolean(arguments, "stopOnEntry", false);
   const auto debuggerRoot = GetString(arguments, "debuggerRoot");
 
if (status.Fail()) {
     g_vsc.SendJSON(llvm::json::Value(std::move(response)));
     return;
   }
 
   // Run any pre run LLDB commands the user specified in the launch.json
   g_vsc.RunPreRunCommands();
 
   if (pid == LLDB_INVALID_PROCESS_ID && wait_for) {
-    char attach_info[256];
-    auto attach_info_len =
-        snprintf(attach_info, sizeof(attach_info),
+    char attach_msg[256];
+    auto attach_msg_len =
+        snprintf(attach_msg, sizeof(attach_msg),
                  "Waiting to attach to \"%s\"...",
                  g_vsc.target.GetExecutable().GetFilename());
-    g_vsc.SendOutput(OutputType::Console, llvm::StringRef(attach_info,
-                                                          attach_info_len));
+    g_vsc.SendOutput(OutputType::Console, llvm::StringRef(attach_msg,
+                                                          attach_msg_len));
   }
   if (attachCommands.empty()) {
     // No "attachCommands", just attach normally.
     // Disable async events so the attach will be successful when we return from
     // the launch call and the launch will happen synchronously
     g_vsc.debugger.SetAsync(false);
-    g_vsc.target.Attach(g_vsc.attach_info, error);
+    g_vsc.target.Attach(attach_info, error);
     // Reenable async events
     g_vsc.debugger.SetAsync(true);
   } else {
     // We have "attachCommands" that are a set of commands that are expected
     // to execute the commands after which a process should be created. If there
     // is no valid process after running these commands, we have failed.
     g_vsc.RunLLDBCommands("Running attachCommands:", attachCommands);
     // The custom commands might have created a new target so we should use the
void request_launch(const llvm::json::Object &request) {
   if (status.Fail()) {
     response["success"] = llvm::json::Value(false);
     EmplaceSafeString(response, "message", status.GetCString());
     g_vsc.SendJSON(llvm::json::Value(std::move(response)));
     return;
   }
 
   // Instantiate a launch info instance for the target.
-  g_vsc.launch_info = g_vsc.target.GetLaunchInfo();
+  auto launch_info = g_vsc.target.GetLaunchInfo();
 
   // Grab the current working directory if there is one and set it in the
   // launch info.
   const auto cwd = GetString(arguments, "cwd");
   if (!cwd.empty())
-    g_vsc.launch_info.SetWorkingDirectory(cwd.data());
+    launch_info.SetWorkingDirectory(cwd.data());
 
   // Extract any extra arguments and append them to our program arguments for
   // when we launch
   auto args = GetStrings(arguments, "args");
   if (!args.empty())
-    g_vsc.launch_info.SetArguments(MakeArgv(args).data(), true);
+    launch_info.SetArguments(MakeArgv(args).data(), true);
 
   // Pass any environment variables along that the user specified.
   auto envs = GetStrings(arguments, "env");
   if (!envs.empty())
-    g_vsc.launch_info.SetEnvironmentEntries(MakeArgv(envs).data(), true);
+    launch_info.SetEnvironmentEntries(MakeArgv(envs).data(), true);
 
-  auto flags = g_vsc.launch_info.GetLaunchFlags();
+  auto flags = launch_info.GetLaunchFlags();
 
   if (GetBoolean(arguments, "disableASLR", true))
     flags |= lldb::eLaunchFlagDisableASLR;
   if (GetBoolean(arguments, "disableSTDIO", false))
     flags |= lldb::eLaunchFlagDisableSTDIO;
   if (GetBoolean(arguments, "shellExpandArguments", false))
     flags |= lldb::eLaunchFlagShellExpandArguments;
   const bool detatchOnError = GetBoolean(arguments, "detachOnError", false);
-  g_vsc.launch_info.SetDetachOnError(detatchOnError);
-  g_vsc.launch_info.SetLaunchFlags(flags | lldb::eLaunchFlagDebug |
-                                   lldb::eLaunchFlagStopAtEntry);
+  launch_info.SetDetachOnError(detatchOnError);
+  launch_info.SetLaunchFlags(flags | lldb::eLaunchFlagDebug |
 
   // Run any pre run LLDB commands the user specified in the launch.json
   g_vsc.RunPreRunCommands();
   if (launchCommands.empty()) {
     // Disable async events so the launch will be successful when we return from
     // the launch call and the launch will happen synchronously
     g_vsc.debugger.SetAsync(false);
-    g_vsc.target.Launch(g_vsc.launch_info, error);
+    g_vsc.target.Launch(launch_info, error);
     g_vsc.debugger.SetAsync(true);
   } else {
     g_vsc.RunLLDBCommands("Running launchCommands:", launchCommands);
     // The custom commands might have created a new target so we should use the
     // selected target after these commands are run.
     g_vsc.target = g_vsc.debugger.GetSelectedTarget();
   }