Page MenuHomePhabricator

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

Authored by jasonmolenda on Thu, Feb 13, 6:35 PM.

Details

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.

Diff Detail

Event Timeline

jasonmolenda created this revision.Thu, Feb 13, 6:35 PM
labath accepted this revision.Thu, Feb 13, 11:52 PM

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

This revision is now accepted and ready to land.Thu, Feb 13, 11:52 PM
clayborg accepted this revision.Fri, Feb 14, 2:33 PM

fine with me!

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.

This revision was automatically updated to reflect the committed changes.