Page MenuHomePhabricator

[LLDB-MI] Escape MI output in a more consistent manner
Needs RevisionPublic

Authored by enlight on Oct 29 2016, 10:21 PM.

Details

Reviewers
abidh
ki.stfu
Summary

While attempting to run lldb-mi on Windows it became apparent that it doesn't consistently escape output, for instance error messages contained paths with unescaped backslashes, and unescaped double-quotes. To address this issue without spending a ridiculous amount of time trying to track down every case where output isn't properly escaped I've opted to move all escaping into CMICmnMIValueConst. CMICmnMIValueConst is the basic building block of all output emitted by lldb-mi, so handling escaping in a consistent manner here will help to ensure all output is properly escaped. This change required removing any existing escaping of output to prevent double-escaping.

I've also removed the spacing code from CMICmnMIValueTuple and CMICmnMIValueResult, it was only misused to format composite values in CMICmnLLDBUtilSBValue::GetCompositeValue(). The format of composite values isn't specified by the GDB-MI spec, and as such these values shouldn't be built using CMICmnMIValueTuple, CMICmnMIValueResult, and CMICmnMIValueConst.

Tested on Ubuntu 14.10 x64 VM

Diff Detail

Repository
rL LLVM

Event Timeline

enlight updated this revision to Diff 76324.Oct 29 2016, 10:21 PM
enlight retitled this revision from to [LLDB-MI] Escape MI output in a more consistent manner.
enlight updated this object.
enlight added reviewers: ki.stfu, abidh.
enlight set the repository for this revision to rL LLVM.
enlight added a subscriber: Restricted Project.
enlight updated this object.Oct 29 2016, 10:26 PM
abidh accepted this revision.Oct 31 2016, 6:36 AM
abidh edited edge metadata.

I have a quick look and it seems ok. Apart from fixing the issue, it is a useful clean up too. If testcases are ok then please free to commit.

If you have noted that a function (AddSlashes?) is not longer needed after this then you may like to remove that too.

tools/lldb-mi/MICmnMIValueConst.cpp
48

So the Escape call here makes sure that we are escaping in all cases.

This revision is now accepted and ready to land.Oct 31 2016, 6:36 AM

Thanks for the review. AddSlashes is still used in a couple of places (but not for escaping MI output), so I'll leave it be for now.

@ki.stfu Are you OK with this going in?

ki.stfu requested changes to this revision.Nov 27 2016, 5:58 AM
ki.stfu edited edge metadata.

@enlight , thanks for giving a chance to review it, and sorry that it took to long from my side.

Most of these changes look good to me, especially removing of meaningless temporal variables during construction of Result values. But I can't agree with that:

I've also removed the spacing code from CMICmnMIValueTuple and CMICmnMIValueResult, it was only misused to format composite values in CMICmnLLDBUtilSBValue::GetCompositeValue(). The format of composite values isn't specified by the GDB-MI spec, and as such these values shouldn't be built using CMICmnMIValueTuple, CMICmnMIValueResult, and CMICmnMIValueConst.

I'm not sure about extra spaces in tuple or result records (vbUseSpacing opt in CMICmnMIValueResult and CMICmnMIValueTuple respectively), but regarding CMICmnLLDBUtilSBValue::GetCompositeValue I'd like to leave "as is". Construction of tuples by hands (see my inline comment) looks terrible for me while using of CMICmnMIValueTuple looked very naturally.

Regarding your note about the format of composite values, the[[ https://sourceware.org/gdb/onlinedocs/gdb/GDB_002fMI-Output-Syntax.html#GDB_002fMI-Output-Syntax | GDB-MI spec ]] clearly says that:

result ==> variable "=" value
variable ==> string 
value ==> const | tuple | list 
const ==> c-string 
tuple ==> "{}" | "{" result ( "," result )* "}"

So, the aggregates are those whose root values are represented by tuples.

tools/lldb-mi/MICmnLLDBUtilSBValue.cpp
197–208

here

This revision now requires changes to proceed.Nov 27 2016, 5:58 AM