ravitheja added a comment.
Uploaded SVN Revision Number - 307773 to fix Android Builder.
https://reviews.llvm.org/D34945
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
clayborg added a comment.
Note that if you put "Differential Revision: " followed by the URL for this:
Differential Revision: https://reviews.llvm.org/D34945
in your source control commit message it will automatically close this for you
and add the SVN revision number into this as a message.
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Wait for an OK from Pavel as well.
https://reviews.llvm.org/D34945
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llv
ravitheja added inline comments.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:32-35
+ RegisterPacketHandler(
+ StringExtractorGDBRemote::eServerPacketType_QEnableErrorStrings,
+ [this](StringExtractorGDBRemote packet, Status &error, b
clayborg added a comment.
Looks good. Just one question on why we register a packet handler.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:32-35
+ RegisterPacketHandler(
+ StringExtractorGDBRemote::eServerPacketType_QEnableErrorStrings,
+
ravitheja updated this revision to Diff 105395.
ravitheja added a comment.
Correcting mistakes.
https://reviews.llvm.org/D34945
Files:
docs/lldb-gdb-remote.txt
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient
labath added a comment.
I am generally happy with this, just a couple of things I noticed below:
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3329
} else {
- error.SetError(response.GetError(), eErrorTypeGeneric);
+error = re
ravitheja updated this revision to Diff 105380.
ravitheja added a comment.
Support for Hex encoded strings and more error checking.
https://reviews.llvm.org/D34945
Files:
docs/lldb-gdb-remote.txt
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb
clayborg added inline comments.
Comment at: source/Utility/StringExtractorGDBRemote.cpp:467
+if (str_index != std::string::npos)
+ error_messg = m_packet.substr(++str_index);
+
clayborg wrote:
> hex encode
There is also a hex decode that will need to be
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
See inlined comments.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3176
+ EnableErrorStringInPacket();
StreamGDBRemote
labath added inline comments.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3176
+ EnableErrorStringInPacket();
StreamGDBRemote escaped_packet;
I don't like how every packet function needs to enable this manually. Every
f
ravitheja updated this revision to Diff 105273.
ravitheja added a comment.
Forgot this in the doc.
https://reviews.llvm.org/D34945
Files:
docs/lldb-gdb-remote.txt
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationCli
ravitheja updated this revision to Diff 105272.
ravitheja added a comment.
Updating Doc and changing feature to be enabled by client.
https://reviews.llvm.org/D34945
Files:
docs/lldb-gdb-remote.txt
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/g
clayborg added a comment.
I would think we would try to enable this using something like:
QEnableErrorStrings
And if the the server responds with "OK" then we know it is enabled. By default
the server should not enable any fancy features without being asked. We would
like lldb-server to stay c
ravitheja added a comment.
With this patch, I see the extra packet to query from server is probably
unnecessary. I mean the error code is still there and it should be sufficient
for the client to detect an error. As long as it does not mistake it for
something else it should not be a problem ?
ravitheja marked 4 inline comments as done.
ravitheja added inline comments.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:110
+ packet += ";";
+ packet += std::string(error.AsCString());
+ return SendPacketNoLock(packet);
lab
ravitheja updated this revision to Diff 105150.
ravitheja added a comment.
changes from feedback.
https://reviews.llvm.org/D34945
Files:
docs/lldb-gdb-remote.txt
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClie
labath added a comment.
In https://reviews.llvm.org/D34945#798808, @ravitheja wrote:
> With this patch, I see the extra packet to query from server is probably
> unnecessary. I mean the error code is still there and it should be sufficient
> for the client to detect an error. As long as it does
ravitheja added a comment.
Yeah that would be broken, as long as we make the error packet more than 3
bytes, then it won't work with the older lldb clients.
https://reviews.llvm.org/D34945
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
h
labath added inline comments.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3203
+error.SetError(response.GetError(), eErrorTypeGeneric);
+ LLDB_LOG(log, "Target does not support Tracing , error {0}",
error.AsCString());
} else
20 matches
Mail list logo
