This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ki.stfu on Feb 24 2015, 7:39 AM.

Details

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\"\\\""

Diff Detail

Event Timeline

ki.stfu updated this revision to Diff 20596.Feb 24 2015, 7:39 AM
ki.stfu retitled this revision from to Fix process's output to stdout/stderr (MI).
ki.stfu updated this object.
ki.stfu edited the test plan for this revision. (Show Details)
ki.stfu added reviewers: abidh, clayborg, emaste.
ki.stfu added subscribers: abidh, clayborg, emaste, Unknown Object (MLST).
ki.stfu updated this revision to Diff 20597.Feb 24 2015, 7:41 AM

Revert CMIUtilString::IsNumber method

clayborg requested changes to this revision.Feb 24 2015, 9:58 AM
clayborg edited edge metadata.

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
863

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);

}

933–938

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

This revision now requires changes to proceed.Feb 24 2015, 9:58 AM
ki.stfu updated this object.Feb 24 2015, 11:43 PM
ki.stfu edited edge metadata.
ki.stfu updated this revision to Diff 20656.Feb 25 2015, 12:08 AM
ki.stfu edited edge metadata.

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

ki.stfu updated this object.Feb 25 2015, 12:11 AM
ki.stfu edited edge metadata.
ki.stfu updated this revision to Diff 20658.Feb 25 2015, 2:12 AM

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

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 updated this revision to Diff 20660.Feb 25 2015, 2:34 AM

Merge CMIUtilString::WrapIntoQuotes and CMIUtilString::Escape

ki.stfu updated this object.Feb 25 2015, 2:35 AM

@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 updated this revision to Diff 20662.Feb 25 2015, 2:59 AM

Add test_lldbmi_process_output test; Move MiSyntaxTestCase to syntax folder.

clayborg requested changes to this revision.Feb 25 2015, 10:44 AM
clayborg edited edge metadata.

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.

This revision now requires changes to proceed.Feb 25 2015, 10:44 AM

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.

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[]>());

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 added inline comments.Feb 25 2015, 11:33 AM
tools/lldb-mi/MIUtilString.cpp
859

Use PRI macros for the correct size here?

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 updated this revision to Diff 20748.Feb 26 2015, 5:16 AM
ki.stfu edited edge metadata.

Use PRIx8; Fix shared_ptr

clayborg accepted this revision.Feb 26 2015, 9:42 AM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Feb 26 2015, 9:42 AM
ki.stfu closed this revision.Feb 26 2015, 10:16 AM

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

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