Don't prefix error message from process launch with "process launch failed:" boilerplate in Target::Launch

[jasonmolenda]

Summary

I wanted to put this up as a phab to collect any reactions instead of just landing it, I think it's not controversial but I could be wrong. Right now, Target::Launch() will prepend any error message with "process launch failed: ". This is a problem with a driver program using the SB API to launch a process on a remote system -- the wording "process launch failed:" makes some sense for the lldb command line driver, but the UI driving lldb may have a different way of describing the operation to the user -- for instance pressing the Run button in an UI. When we get back a meaningful error message about why the launch failed, prepending boilerplate like this makes it harder for a user to spot the error message.

It's a little annoying to test - on a native run, at least on macOS, our process launching doesn't go through Target::Process, CommandObjectProcessLaunch and SBTarget::Launch go through NativeProcessDarwin, there is no change in the error messaging in this case. I looked over PlatformLinux and PlatformNetBSD, they both seem to prefix their own "process launch failed: " strings; PlatformWindows overwrites the error message.

[labath]

I think this is fine.

I added a "gdb client" test for an attach failure error message a while back, so I was curious if the same thing can be done for launches -- it can and 65e843c9 has the result. I think that patch shows that we really should remove the prefix, since the equivalent "attach" error does not have any additional prefixes.

PS: Don't forget to update the new test before committing this. :)

[clayborg]

fine with me!

[jasonmolenda]

Updated the test to match the new output (how did I miss that??), also for consistency removed the other occurrences of "process launch failed: " across lldb, two of which were only in log messages.

lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp

if (error.Success()) {
     // with llgs).
     int pty_fd = launch_info.GetPTY().ReleaseMasterFileDescriptor();
     if (pty_fd != PseudoTerminal::invalid_fd) {
       process_sp->SetSTDIOFileDescriptor(pty_fd);
       LLDB_LOG(log, "hooked up STDIO pty to process");
     } else
       LLDB_LOG(log, "not using process STDIO pty");
   } else {
-    LLDB_LOG(log, "process launch failed: {0}", error);
+    LLDB_LOG(log, "{0}", error);
     // FIXME figure out appropriate cleanup here.  Do we delete the target? Do
     // we delete the process?  Does our caller do that?
   }
 
   return process_sp;
 }
 
 void PlatformLinux::CalculateTrapHandlerSymbolNames() {

lldb/source/Plugins/Platform/NetBSD/PlatformNetBSD.cpp

if (error.Success()) {
     // with llgs).
     int pty_fd = launch_info.GetPTY().ReleaseMasterFileDescriptor();
     if (pty_fd != PseudoTerminal::invalid_fd) {
       process_sp->SetSTDIOFileDescriptor(pty_fd);
       LLDB_LOG(log, "hooked up STDIO pty to process");
     } else
       LLDB_LOG(log, "not using process STDIO pty");
   } else {
-    LLDB_LOG(log, "process launch failed: {0}", error);
+    LLDB_LOG(log, "{0}", error);
     // FIXME figure out appropriate cleanup here.  Do we delete the target? Do
     // we delete the process?  Does our caller do that?
   }
 
   return process_sp;
 }
 
 void PlatformNetBSD::CalculateTrapHandlerSymbolNames() {

lldb/source/Target/Target.cpp

"launch'."
                                            exit_status);
         }
       } else {
         error.SetErrorStringWithFormat(
             "initial process state wasn't stopped: %s", StateAsCString(state));
       }
     }
     m_process_sp->RestoreProcessEvents();
-  } else {
-    Status error2;
-    error2.SetErrorStringWithFormat("process launch failed: %s",
-                                    error.AsCString());
-    error = error2;
   }
   return error;
 }
 
 Status Target::Attach(ProcessAttachInfo &attach_info, Stream *stream) {
   auto state = eStateInvalid;
   auto process_sp = GetProcessSP();
   if (process_sp) {

lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py

def test_launch_fail(self):
 
         target = self.createTarget("a.yaml")
         process = self.connect(target)
         lldbutil.expect_state_changes(self, self.dbg.GetListener(), process, [lldb.eStateConnected])
 
         error = lldb.SBError()
         target.Launch(lldb.SBListener(), None, None, None, None, None,
                 None, 0, True, error)
-        self.assertEquals("process launch failed: 'A' packet returned an error: 71", error.GetCString())
+        self.assertEquals("'A' packet returned an error: 71", error.GetCString())
 
     def test_read_registers_using_g_packets(self):
         """Test reading registers using 'g' packets (default behavior)"""
         self.dbg.HandleCommand(
                 "settings set plugin.process.gdb-remote.use-g-packet-for-reading true")
         self.addTearDownHook(lambda: 
                 self.runCmd("settings set plugin.process.gdb-remote.use-g-packet-for-reading false"))
         self.server.responder = self.gPacketResponder()

lldb/test/Shell/Process/Windows/launch_failure.yaml

 ## Test that we don't crash when the process fails to launch before reaching
 ## the initial stop. This test exe has a dependency on NonExistent.dll.
 
 # REQUIRES: system-windows
 # REQUIRES: native && target-x86_64
 
 # RUN: yaml2obj %s > %t.exe
 # RUN: %lldb %t.exe -o run 2>&1 | FileCheck %s
 
-# CHECK: error: process launch failed: unknown error
+# CHECK: error: unknown error
 
 --- !COFF
 OptionalHeader:
   AddressOfEntryPoint: 4096
   ImageBase:       1073741824
   SectionAlignment: 4096
   FileAlignment:   512
   MajorOperatingSystemVersion: 6