Cleanup errors that come out of commands and make sure they all have newlines

_only_ in the resulting stream, not in the error objects (lldb_private::Error).
lldb_private::Error objects should always just have an error string with no 
terminating newline characters or periods.

Fixed an issue with GDB remote packet detection that could end up deadlocking
if a full packet wasn't received in one chunk. Also modified the packet 
checking function to properly toss one or more bytes when it detects bad
data. 

llvm-svn: 134357

3 files changed, 85 insertions, 23 deletions

diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp
index d561183b64c..8ccdebb9b6c 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -366,7 +366,7 @@ public:
         }
         else
         {
-            result.AppendErrorWithFormat ("Process launch failed: %s", error.AsCString());
+            result.AppendErrorWithFormat ("process launch failed: %s", error.AsCString());
             result.SetStatus (eReturnStatusFailed);
         }
 
@@ -1832,18 +1832,18 @@ CommandObjectMultiwordProcess::CommandObjectMultiwordProcess (CommandInterpreter
                             "A set of commands for operating on a process.",
                             "process <subcommand> [<subcommand-options>]")
 {
-    LoadSubCommand ("attach",      CommandObjectSP (new CommandObjectProcessAttach (interpreter)));
-    LoadSubCommand ("launch",      CommandObjectSP (new CommandObjectProcessLaunch (interpreter)));
-    LoadSubCommand ("continue",    CommandObjectSP (new CommandObjectProcessContinue (interpreter)));
-    LoadSubCommand ("connect",     CommandObjectSP (new CommandObjectProcessConnect (interpreter)));
-    LoadSubCommand ("detach",      CommandObjectSP (new CommandObjectProcessDetach (interpreter)));
-    LoadSubCommand ("load",        CommandObjectSP (new CommandObjectProcessLoad (interpreter)));
-    LoadSubCommand ("unload",      CommandObjectSP (new CommandObjectProcessUnload (interpreter)));
-    LoadSubCommand ("signal",      CommandObjectSP (new CommandObjectProcessSignal (interpreter)));
-    LoadSubCommand ("handle",      CommandObjectSP (new CommandObjectProcessHandle (interpreter)));
-    LoadSubCommand ("status",      CommandObjectSP (new CommandObjectProcessStatus (interpreter)));
+    LoadSubCommand ("attach",      CommandObjectSP (new CommandObjectProcessAttach    (interpreter)));
+    LoadSubCommand ("launch",      CommandObjectSP (new CommandObjectProcessLaunch    (interpreter)));
+    LoadSubCommand ("continue",    CommandObjectSP (new CommandObjectProcessContinue  (interpreter)));
+    LoadSubCommand ("connect",     CommandObjectSP (new CommandObjectProcessConnect   (interpreter)));
+    LoadSubCommand ("detach",      CommandObjectSP (new CommandObjectProcessDetach    (interpreter)));
+    LoadSubCommand ("load",        CommandObjectSP (new CommandObjectProcessLoad      (interpreter)));
+    LoadSubCommand ("unload",      CommandObjectSP (new CommandObjectProcessUnload    (interpreter)));
+    LoadSubCommand ("signal",      CommandObjectSP (new CommandObjectProcessSignal    (interpreter)));
+    LoadSubCommand ("handle",      CommandObjectSP (new CommandObjectProcessHandle    (interpreter)));
+    LoadSubCommand ("status",      CommandObjectSP (new CommandObjectProcessStatus    (interpreter)));
     LoadSubCommand ("interrupt",   CommandObjectSP (new CommandObjectProcessInterrupt (interpreter)));
-    LoadSubCommand ("kill",        CommandObjectSP (new CommandObjectProcessKill (interpreter)));
+    LoadSubCommand ("kill",        CommandObjectSP (new CommandObjectProcessKill      (interpreter)));
 }
 
 CommandObjectMultiwordProcess::~CommandObjectMultiwordProcess ()
diff --git a/lldb/source/Interpreter/CommandReturnObject.cpp b/lldb/source/Interpreter/CommandReturnObject.cpp
index 9d32279edab..eb6fae4ce1b 100644
--- a/lldb/source/Interpreter/CommandReturnObject.cpp
+++ b/lldb/source/Interpreter/CommandReturnObject.cpp
@@ -18,6 +18,29 @@
 using namespace lldb;
 using namespace lldb_private;
 
+static void
+DumpStringToStreamWithNewline (Stream &strm, const std::string &s, bool add_newline_if_empty)
+{
+    bool add_newline = false;
+    if (s.empty())
+    {
+        add_newline = add_newline_if_empty;
+    }
+    else
+    {
+        // We already checked for empty above, now make sure there is a newline
+        // in the error, and if there isn't one, add one.
+        strm.Write(s.c_str(), s.size());
+
+        const char last_char = *s.rbegin();
+        add_newline = last_char != '\n' && last_char != '\r';
+
+    }
+    if (add_newline)
+        strm.EOL();
+}
+
+
 CommandReturnObject::CommandReturnObject () :
     m_out_stream (),
     m_err_stream (),
@@ -39,7 +62,14 @@ CommandReturnObject::AppendErrorWithFormat (const char *format, ...)
     sstrm.PrintfVarArg(format, args);
     va_end (args);
 
-    GetErrorStream().Printf("error: %s", sstrm.GetData());
+    
+    const std::string &s = sstrm.GetString();
+    if (!s.empty())
+    {
+        Stream &error_strm = GetErrorStream();
+        error_strm.PutCString ("error: ");
+        DumpStringToStreamWithNewline (error_strm, s, false);
+    }
 }
 
 void
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index 12c912da02f..4525bc8eed2 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -194,8 +194,7 @@ GDBRemoteCommunication::WaitForPacketWithTimeoutMicroSecondsNoLock (StringExtrac
     if (CheckForPacket (NULL, 0, packet))
         return packet.GetStringRef().size();
 
-    bool timed_out = false;
-    while (IsConnected() && !timed_out)
+    while (IsConnected())
     {
         lldb::ConnectionStatus status;
         size_t bytes_read = Read (buffer, sizeof(buffer), timeout_usec, status, &error);
@@ -209,6 +208,7 @@ GDBRemoteCommunication::WaitForPacketWithTimeoutMicroSecondsNoLock (StringExtrac
             switch (status)
             {
             case eConnectionStatusSuccess:
+            case eConnectionStatusTimedOut:
                 break;
                 
             case eConnectionStatusEndOfFile:
@@ -217,11 +217,10 @@ GDBRemoteCommunication::WaitForPacketWithTimeoutMicroSecondsNoLock (StringExtrac
             case eConnectionStatusError:
                 Disconnect();
                 break;
-            
-            case eConnectionStatusTimedOut:
-                timed_out = true;
-                break;
             }
+            
+            // Get out of the while loop we we finally timeout without getting any data
+            break;
         }
     }
     packet.Clear ();    
@@ -233,11 +232,21 @@ GDBRemoteCommunication::CheckForPacket (const uint8_t *src, size_t src_len, Stri
 {
     // Put the packet data into the buffer in a thread safe fashion
     Mutex::Locker locker(m_bytes_mutex);
+    
+    LogSP log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet (GDBR_LOG_PACKETS));
+
     if (src && src_len > 0)
+    {
+        if (log)
+        {
+            StreamString s;
+            log->Printf ("GDBRemoteCommunication::%s adding %zu bytes: %s\n",__FUNCTION__, src_len, src);
+        }
         m_bytes.append ((const char *)src, src_len);
+    }
 
     // Parse up the packets into gdb remote packets
-    while (!m_bytes.empty())
+    if (!m_bytes.empty())
     {
         // end_idx must be one past the last valid packet byte. Start
         // it off with an invalid value that is the same as the current
@@ -246,7 +255,6 @@ GDBRemoteCommunication::CheckForPacket (const uint8_t *src, size_t src_len, Stri
         size_t content_length = 0;
         size_t total_length = 0;
         size_t checksum_idx = std::string::npos;
-        LogSP log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet (GDBR_LOG_PACKETS));
 
         switch (m_bytes[0])
         {
@@ -283,9 +291,33 @@ GDBRemoteCommunication::CheckForPacket (const uint8_t *src, size_t src_len, Stri
 
             default:
                 {
+                    // We have an unexpected byte and we need to flush all bad 
+                    // data that is in m_bytes, so we need to find the first
+                    // byte that is a '+' (ACK), '-' (NACK), \x03 (CTRL+C interrupt),
+                    // or '$' character (start of packet header) or of course,
+                    // the end of the data in m_bytes...
+                    const size_t bytes_len = m_bytes.size();
+                    bool done = false;
+                    uint32_t idx;
+                    for (idx = 1; !done && idx < bytes_len; ++idx)
+                    {
+                        switch (m_bytes[idx])
+                        {
+                        case '+':
+                        case '-':
+                        case '\x03':
+                        case '$':
+                            done = true;
+                            break;
+                                
+                        default:
+                            break;
+                        }
+                    }
                     if (log)
-                        log->Printf ("GDBRemoteCommunication::%s tossing junk byte at %c",__FUNCTION__, m_bytes[0]);
-                    m_bytes.erase(0, 1);
+                        log->Printf ("GDBRemoteCommunication::%s tossing %u junk bytes: '%.*s'",
+                                     __FUNCTION__, idx, idx, m_bytes.c_str());
+                    m_bytes.erase(0, idx);
                 }
                 break;
         }