Fix process's output to stdout/stderr (MI)

[ki.stfu]

Summary

• Add CMIUtilString::Escape/Unescape methods (MI)
• Fix process's output to stdout/stderr (MI):

lldb-mi escapes process's output to show it in the following format:

~"..."

But previously not all characters were escaped by CMICmnLLDBDebuggerHandleEvents::ConvertPrintfCtrlCodeToString and output of

printf("'\n` - it's \\n\x12\"\\\"")

looked like:

~"'\r\n` - it's \n"\""

This patch fixes it by using CMIUtilString::Escape method and now it looks like:

~"'\r\n` - it's \\n\x12\"\\\""

[ki.stfu]

Revert CMIUtilString::IsNumber method

[clayborg]

We need to support all characters and any test should include all characters from 0 - 255 all in a string to ensure we can decode things properly on the other end.

tools/lldb-mi/MIUtilString.cpp

864

Most of our escape code functions actually do a switch on all characters instead of checking bEscapeCharNotFound. Just do the switch and in the default case do:

if (::isprint(cUnescapedChar))

strNew.push_back(cUnescapedChar);

else
{

char hex_escape[8];
int hex_escape_len = snprintf(hex_escape, sizeof(hex_escape), "\\x%02x", cUnescapedChar);
strNew.append(hex_escape, hex_escape_len);

}

934–939

Handle \xHH escaped characters here if we add support above using the isprint stuff above.

[ki.stfu]

Add hex suppot in CMIUtilString::Escape; remove CMIUtilString::Unescape/ms_strEscapeCharacters; Use large dynamic buffer in CMICmnLLDBDebuggerHandleEvents::GetProcessStdout/GetProcessStderr

[ki.stfu]

Forgot to wrap quotes in output; add CMIUtilString::WrapIntoQuotes

[ki.stfu]

I don't know exactly, whether Escape() should process quotes or not, but I gonna to merge CMIUtilString::WrapIntoQuotes and CMIUtilString::Escape functions, because WrapIntoQuotes should process quotes and backslashes and therefore Escape().WrapIntoQuotes() doubles amount of slashes.

[ki.stfu]

Merge CMIUtilString::WrapIntoQuotes and CMIUtilString::Escape

[ki.stfu]

@clayborg, @abidh

Please, take a look again.

Can I set the vbEscapeQuotes default value to "true" or remove this argument at all and handle quotes always?

[ki.stfu]

Add test_lldbmi_process_output test; Move MiSyntaxTestCase to syntax folder.

[clayborg]

See inlined comments on your use of shared_ptr.

tools/lldb-mi/MICmnLLDBDebuggerHandleEvents.cpp

1383

Any reason you don't just use a stack based string "char spStdoutBuffer[1024];"?

Also are you making a shared pointer to a char and then feeding it something that needs to be deleted with "delete[]" but it won't, it will get deleted with just "delete"?

1416

See above comment on spStdoutBuffer and apply same fix here.

[ki.stfu]

In D7858#129802, @clayborg wrote:

Any reason you don't just use a stack based string "char spStdoutBuffer[1024];"?

We discussed it with @zturner that big buffers shouldn't be allocated on the stack.

[clayborg]

So then my other comment stands: I believe you using std::shared_ptr<char> incorrectly unless the compiler does some magic. I believe your arrays should be declared like:

std::shared_ptr<char> spStdoutBuffer(new char[1024], std::default_delete<char[]>());

[ki.stfu]

In D7858#129828, @clayborg wrote:

So then my other comment stands: I believe you using std::shared_ptr<char> incorrectly unless the compiler does some magic. I believe your arrays should be declared like:

std::shared_ptr<char> spStdoutBuffer(new char[1024], std::default_delete<char[]>());

Agree. I'll fix it.

But what about Escape function?? (See my questions above)

[clayborg]

tools/lldb-mi/MIUtilString.cpp

860

Use PRI macros for the correct size here?

[clayborg]

I don't know enough to answer if vbEscapeQuotes can be removed. It really depends on what MI needs out of this. If MI wants arguments that are already pulled apart, those arguments should match what a shell would commonly do IMHO, but again, I don't know what MI expects.

[ki.stfu]

Use PRIx8; Fix shared_ptr

[clayborg]

Looks good.

[clayborg]

I don't know enough to answer if vbEscapeQuotes can be removed. It really depends on what MI needs out of this. If MI wants arguments that are already pulled apart, those arguments shoul

[clayborg]

I don't know enough to answer if vbEscapeQuotes can be removed. It really depends on what MI needs out of this. If MI wants arguments that are already pulled apart, those arguments shoul

[ki.stfu]

ok

test/tools/lldb-mi/syntax/Makefile

+LEVEL = ../../../make
+
+CXX_SOURCES := main.cpp
+
+include $(LEVEL)/Makefile.rules

test/tools/lldb-mi/syntax/TestMiSyntax.py

 """
 Test that the lldb-mi driver understands MI command syntax.
 """
 
+# adjust path for lldbmi_testcase.py
+import sys, os.path
+sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), '..')))
+
 import lldbmi_testcase
 from lldbtest import *
 import unittest2
 
 class MiSyntaxTestCase(lldbmi_testcase.MiTestCaseBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
def test_lldbmi_specialchars(self):
             self.runCmd("-exec-run")
             self.expect("\^running")
             self.expect("\*stopped,reason=\"breakpoint-hit\"")
 
         finally:
             # Clean up
             os.unlink(complicated_myexe)
 
+    @lldbmi_test
+    @expectedFailureWindows("llvm.org/pr22274: need a pexpect replacement for windows")
+    @skipIfFreeBSD # llvm.org/pr22411: Failure presumably due to known thread races
+    def test_lldbmi_process_output(self):
+        """Test that 'lldb-mi --interpreter' wraps process output correctly."""
+
+        self.spawnLldbMi(args = None)
+
+        # Load executable
+        self.runCmd("-file-exec-and-symbols %s" % self.myexe)
+        self.expect("\^done")
+
+        # Run
+        self.runCmd("-exec-run")
+        self.expect("\^running")
+
+        # Test that a process output is wrapped correctly
+        self.expect("\~\"'\\\\r\\\\n` - it's \\\\\\\\n\\\\x12\\\\\"\\\\\\\\\\\\\"")
+
 if __name__ == '__main__':
     unittest2.main()

test/tools/lldb-mi/syntax/main.cpp

+//===-- main.cpp ------------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include <cstdio>
+
+int
+main(int argc, char const *argv[]) 
+{
+    printf("'\n` - it's \\n\x12\"\\\"");
+    return 0;
+}

tools/lldb-mi/MICmnLLDBDebuggerHandleEvents.h

private:
     bool HandleProcessEventStateSuspended(const lldb::SBEvent &vEvent);
     bool MiHelpGetCurrentThreadFrame(CMICmnMIValueTuple &vwrMiValueTuple);
     bool MiResultRecordToStdout(const CMICmnMIResultRecord &vrMiResultRecord);
     bool MiOutOfBandRecordToStdout(const CMICmnMIOutOfBandRecord &vrMiResultRecord);
     bool MiStoppedAtBreakPoint(const MIuint64 vBrkPtId, const lldb::SBBreakpoint &vBrkPt);
     bool TextToStdout(const CMIUtilString &vrTxt);
     bool TextToStderr(const CMIUtilString &vrTxt);
     bool UpdateSelectedThread(void);
-    bool ConvertPrintfCtrlCodeToString(const MIchar vCtrl, CMIUtilString &vwrStrEquivalent);
 
     // Overridden:
   private:
     // From CMICmnBase
     /* dtor */ virtual ~CMICmnLLDBDebuggerHandleEvents(void);
     void InitializeSignals();
     bool m_bSignalsInitialized;
     MIuint64 m_SIGINT;
     MIuint64 m_SIGSTOP;
     MIuint64 m_SIGSEGV;
     MIuint64 m_SIGTRAP;
 };

tools/lldb-mi/MICmnLLDBDebuggerHandleEvents.cpp

 // Args:    None.
 // Return:  MIstatus::success - Functionality succeeded.
 //          MIstatus::failure - Functionality failed.
 // Throws:  None.
 //--
 bool
 CMICmnLLDBDebuggerHandleEvents::GetProcessStdout(void)
 {
-    bool bOk = MIstatus::success;
-
-    char c;
-    size_t nBytes = 0;
     CMIUtilString text;
+    std::unique_ptr<char[]> apStdoutBuffer(new char[1024]);
     lldb::SBProcess process = CMICmnLLDBDebugSessionInfo::Instance().GetDebugger().GetSelectedTarget().GetProcess();
-    while (process.GetSTDOUT(&c, 1) > 0)
+    while (1)
     {
-        CMIUtilString str;
-        if (ConvertPrintfCtrlCodeToString(c, str))
-            text += str;
-        nBytes++;
-    }
-    if (nBytes > 0)
-    {
-        const CMIUtilString t(CMIUtilString::Format("~\"%s\"", text.c_str()));
-        bOk = TextToStdout(t);
+        const size_t nBytes = process.GetSTDOUT(apStdoutBuffer.get(), 1024);
+        if (nBytes == 0)
+            break;
+
+        text.append(apStdoutBuffer.get(), nBytes);
     }
 
-    return bOk;
+    if (text.empty())
+        return MIstatus::success;
+
+    const bool bEscapeQuotes(true);
+    const CMIUtilString t(CMIUtilString::Format("~\"%s\"", text.Escape(bEscapeQuotes).c_str()));
+    return TextToStdout(t);
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: Drain all stderr so we don't see any output come after we print our prompts.
 //          The process has stuff waiting for stderr; get it and write it out to the
 //          appropriate place.
 // Type:    Method.
 // Args:    None.
 // Return:  MIstatus::success - Functionality succeeded.
 //          MIstatus::failure - Functionality failed.
 // Throws:  None.
 //--
 bool
 CMICmnLLDBDebuggerHandleEvents::GetProcessStderr(void)
 {
-    bool bOk = MIstatus::success;
-
-    char c;
-    size_t nBytes = 0;
     CMIUtilString text;
+    std::unique_ptr<char[]> apStderrBuffer(new char[1024]);
     lldb::SBProcess process = CMICmnLLDBDebugSessionInfo::Instance().GetDebugger().GetSelectedTarget().GetProcess();
-    while (process.GetSTDERR(&c, 1) > 0)
-    {
-        CMIUtilString str;
-        if (ConvertPrintfCtrlCodeToString(c, str))
-            text += str;
-        nBytes++;
-    }
-    if (nBytes > 0)
-    {
-        const CMIUtilString t(CMIUtilString::Format("~\"%s\"", text.c_str()));
-        bOk = TextToStdout(t);
-    }
-
-    return bOk;
-}
-
-//++ ------------------------------------------------------------------------------------
-// Details: Convert text stream control codes to text equivalent.
-// Type:    Method.
-// Args:    vCtrl             - (R) The control code.
-//          vwrStrEquivalent  - (W) The text equivalent.
-// Return:  MIstatus::success - Functionality succeeded.
-//          MIstatus::failure - Functionality failed.
-// Throws:  None.
-//--
-bool
-CMICmnLLDBDebuggerHandleEvents::ConvertPrintfCtrlCodeToString(const MIchar vCtrl, CMIUtilString &vwrStrEquivalent)
-{
-    switch (vCtrl)
+    while (1)
     {
-        case '\033':
-            vwrStrEquivalent = "\\e";
-            break;
-        case '\a':
-            vwrStrEquivalent = "\\a";
-            break;
-        case '\b':
-            vwrStrEquivalent = "\\b";
-            break;
-        case '\f':
-            vwrStrEquivalent = "\\f";
-            break;
-        case '\n':
-            vwrStrEquivalent = "\\n";
-            break;
-        case '\r':
-            vwrStrEquivalent = "\\r";
-            break;
-        case '\t':
-            vwrStrEquivalent = "\\t";
-            break;
-        case '\v':
-            vwrStrEquivalent = "\\v";
-            break;
-        default:
-            vwrStrEquivalent = CMIUtilString::Format("%c", vCtrl);
+        const size_t nBytes = process.GetSTDERR(apStderrBuffer.get(), 1024);
+        if (nBytes == 0)
             break;
+
+        text.append(apStderrBuffer.get(), nBytes);
     }
 
+    if (text.empty())
         return MIstatus::success;
+
+    const bool bEscapeQuotes(true);
+    const CMIUtilString t(CMIUtilString::Format("~\"%s\"", text.Escape(bEscapeQuotes).c_str()));
+    return TextToStdout(t);
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: Asynchronous event function check for state changes.
 // Type:    Method.
 // Args:    None.
 // Return:  MIstatus::success - Functionality succeeded.
 //          MIstatus::failure - Functionality failed.

tools/lldb-mi/MIUtilString.h

public:
     CMIUtilString StripCREndOfLine(void) const;
     CMIUtilString StripCRAll(void) const;
     CMIUtilString Trim(void) const;
     CMIUtilString Trim(const MIchar vChar) const;
     MIuint FindFirst(const CMIUtilString &vrPattern, const MIuint vnPos = 0) const;
     MIuint FindFirst(const CMIUtilString &vrPattern, const bool vbSkipQuotedText, bool &vrwbNotFoundClosedQuote,
                      const MIuint vnPos = 0) const;
     MIuint FindFirstNot(const CMIUtilString &vrPattern, const MIuint vnPos = 0) const;
+    CMIUtilString Escape(const bool vbEscapeQuotes = false) const;
     //
     CMIUtilString &operator=(const MIchar *vpRhs);
     CMIUtilString &operator=(const std::string &vrRhs);
 
     // Overrideable:
   public:
     /* dtor */ virtual ~CMIUtilString(void);
 

tools/lldb-mi/MIUtilString.cpp

 // Environment: Compilers:  Visual C++ 12.
 //                          gcc (Ubuntu/Linaro 4.8.1-10ubuntu9) 4.8.1
 //              Libraries:  See MIReadmetxt.
 //
 // Copyright:   None.
 //--
 
 // Third party headers
-#include <memory>   // std::unique_ptr
-#include <stdarg.h> // va_list, va_start, var_end
-#include <sstream>  // std::stringstream
-#include <string.h> // for strncmp
-#include <limits.h> // for ULONG_MAX
+#include <inttypes.h> // for PRIx8
+#include <limits.h>   // for ULONG_MAX
+#include <memory>     // std::unique_ptr
+#include <sstream>    // std::stringstream
+#include <stdarg.h>   // va_list, va_start, var_end
+#include <string.h>   // for strncmp
 
 // In-house headers:
 #include "MIUtilString.h"
 
 //++ ------------------------------------------------------------------------------------
 // Details: CMIUtilString constructor.
 // Type:    Method.
 // Args:    None.
do
 
         // Skip 2 characters: First is '\', second is that which is escaped by '\'
         nPos = nBckSlashPos + 2;
     }
     while (nPos < nLen);
 
     return (MIuint)std::string::npos;
 }
+
+//++ ------------------------------------------------------------------------------------
+// Details: Get escaped string from *this string.
+// Type:    Method.
+// Args:    None.
+// Return:  CMIUtilString - The escaped version of the initial string.
+// Throws:  None.
+//--
+CMIUtilString
+CMIUtilString::Escape(const bool vbEscapeQuotes /* = false */) const
+{
+    const MIuint nLen(length());
+    CMIUtilString strNew;
+    strNew.reserve(nLen);
+    for (MIuint nIndex(0); nIndex < nLen; ++nIndex)
+    {
+        const MIchar cUnescapedChar((*this)[nIndex]);
+        switch (cUnescapedChar)
+        {
+            case '\a':
+                strNew.append("\\a");
+                break;
+            case '\b':
+                strNew.append("\\b");
+                break;
+            case '\t':
+                strNew.append("\\t");
+                break;
+            case '\n':
+                strNew.append("\\n");
+                break;
+            case '\v':
+                strNew.append("\\v");
+                break;
+            case '\f':
+                strNew.append("\\f");
+                break;
+            case '\r':
+                strNew.append("\\r");
+                break;
+            case '\033':
+                strNew.append("\\e");
+                break;
+            case '\\':
+                strNew.append("\\\\");
+                break;
+            case '\"':
+                if (vbEscapeQuotes)
+                {
+                    strNew.append("\\\"");
+                    break;
+                }
+                // FALLTHROUGH
+            default:
+                if (::isprint(cUnescapedChar))
+                    strNew.push_back(cUnescapedChar);
+                else
+                {
+                    char strEscapedChar[sizeof("\\xXX")];
+                    ::sprintf(strEscapedChar, "\\x%02" PRIx8, cUnescapedChar);
+                    strNew.append(strEscapedChar);
+                }
+                break;
+        }
+    }
+    return strNew;
+}