[Lldb-commits] [PATCH] D100146: [lldb] [gdb-remote client] Refactor handling qSupported

[Michał Górny via Phabricator via lldb-commits]

mgorny marked 2 inline comments as done.
mgorny added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:375
+
+for (auto x : server_features) {
+  if (x == "qXfer:auxv:read+")

labath wrote:
> mgorny wrote:
> > labath wrote:
> > > not a big deal, but this probably shouldn't be auto.
> > Could you explain a bit? I thought `auto` is convenient here since the 
> > actual type is visible three lines higher.
> Well.. my reasoning was something like this:
> - llvm guidelines say that auto should be used (only?) when the type is 
> obvious from context
> - "context" is not exactly specified, but I would take that to mean the 
> enclosing expression/statement (not the statement two lines up). The use of 
> `cast` in the example supports this.
> - the actual type is not that complicated (llvm::StringRef), so it e.g. won't 
> cause the line to be split in two
> - given the above, and the generally cautious approach to auto in llvm, its 
> better to err on the side of spelling out the type
> 
> But like I said, it's not a big deal...
Yeah, this makes sense.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100146/new/

https://reviews.llvm.org/D100146

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100146: [lldb] [gdb-remote client] Refactor handling qSupported

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D100146#2685026 , @mgorny wrote:

> In D100146#2684942 , @labath wrote:
>
>> looks great, just fix the build errors :)
>
> Yeah, I'm trying to see if I can reproduce them when building with Clang.

I believe StringRef is not implicitly convertible to std::string these days, 
but all the conversions are in conditionally compiled code. Maybe you don't 
have any of it enabled?

My guess is it would be sufficient to change `avail_name` to a StringRef (and 
fix the errors caused by that).




Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:375
+
+for (auto x : server_features) {
+  if (x == "qXfer:auxv:read+")

mgorny wrote:
> labath wrote:
> > not a big deal, but this probably shouldn't be auto.
> Could you explain a bit? I thought `auto` is convenient here since the actual 
> type is visible three lines higher.
Well.. my reasoning was something like this:
- llvm guidelines say that auto should be used (only?) when the type is obvious 
from context
- "context" is not exactly specified, but I would take that to mean the 
enclosing expression/statement (not the statement two lines up). The use of 
`cast` in the example supports this.
- the actual type is not that complicated (llvm::StringRef), so it e.g. won't 
cause the line to be split in two
- given the above, and the generally cautious approach to auto in llvm, its 
better to err on the side of spelling out the type

But like I said, it's not a big deal...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100146/new/

https://reviews.llvm.org/D100146

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100146: [lldb] [gdb-remote client] Refactor handling qSupported

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

In D100146#2684942 , @labath wrote:

> looks great, just fix the build errors :)

Yeah, I'm trying to see if I can reproduce them when building with Clang.




Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:375
+
+for (auto x : server_features) {
+  if (x == "qXfer:auxv:read+")

labath wrote:
> not a big deal, but this probably shouldn't be auto.
Could you explain a bit? I thought `auto` is convenient here since the actual 
type is visible three lines higher.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100146/new/

https://reviews.llvm.org/D100146

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100146: [lldb] [gdb-remote client] Refactor handling qSupported

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.
Herald added a subscriber: JDevlieghere.

looks great, just fix the build errors :)




Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:375
+
+for (auto x : server_features) {
+  if (x == "qXfer:auxv:read+")

not a big deal, but this probably shouldn't be auto.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100146/new/

https://reviews.llvm.org/D100146

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100146: [lldb] [gdb-remote client] Refactor handling qSupported

[Michał Górny via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3842de49f655: [lldb] [gdb-remote client] Refactor handling 
qSupported (authored by mgorny).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100146/new/

https://reviews.llvm.org/D100146

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h

Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -601,7 +601,8 @@
 
   // Given the list of compression types that the remote debug stub can support,
   // possibly enable compression if we find an encoding we can handle.
-  void MaybeEnableCompression(std::vector supported_compressions);
+  void MaybeEnableCompression(
+  llvm::ArrayRef supported_compressions);
 
   bool DecodeProcessInfoResponse(StringExtractorGDBRemote ,
  ProcessInstanceInfo _info);
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -345,6 +345,9 @@
   m_supports_qXfer_features_read = eLazyBoolNo;
   m_supports_qXfer_memory_map_read = eLazyBoolNo;
   m_supports_multiprocess = eLazyBoolNo;
+  m_supports_qEcho = eLazyBoolNo;
+  m_supports_QPassSignals = eLazyBoolNo;
+
   m_max_packet_size = UINT64_MAX; // It's supposed to always be there, but if
   // not, we assume no limit
 
@@ -362,97 +365,51 @@
   if (SendPacketAndWaitForResponse(packet.GetString(), response,
/*send_async=*/false) ==
   PacketResult::Success) {
-const char *response_cstr = response.GetStringRef().data();
-
 // Hang on to the qSupported packet, so that platforms can do custom
 // configuration of the transport before attaching/launching the process.
-m_qSupported_response = response_cstr;
-
-if (::strstr(response_cstr, "qXfer:auxv:read+"))
-  m_supports_qXfer_auxv_read = eLazyBoolYes;
-if (::strstr(response_cstr, "qXfer:libraries-svr4:read+"))
-  m_supports_qXfer_libraries_svr4_read = eLazyBoolYes;
-if (::strstr(response_cstr, "augmented-libraries-svr4-read")) {
-  m_supports_qXfer_libraries_svr4_read = eLazyBoolYes; // implied
-  m_supports_augmented_libraries_svr4_read = eLazyBoolYes;
-}
-if (::strstr(response_cstr, "qXfer:libraries:read+"))
-  m_supports_qXfer_libraries_read = eLazyBoolYes;
-if (::strstr(response_cstr, "qXfer:features:read+"))
-  m_supports_qXfer_features_read = eLazyBoolYes;
-if (::strstr(response_cstr, "qXfer:memory-map:read+"))
-  m_supports_qXfer_memory_map_read = eLazyBoolYes;
-
-// Look for a list of compressions in the features list e.g.
-// qXfer:features:read+;PacketSize=2;qEcho+;SupportedCompressions=zlib-
-// deflate,lzma
-const char *features_list = ::strstr(response_cstr, "qXfer:features:");
-if (features_list) {
-  const char *compressions =
-  ::strstr(features_list, "SupportedCompressions=");
-  if (compressions) {
-std::vector supported_compressions;
-compressions += sizeof("SupportedCompressions=") - 1;
-const char *end_of_compressions = strchr(compressions, ';');
-if (end_of_compressions == nullptr) {
-  end_of_compressions = strchr(compressions, '\0');
-}
-const char *current_compression = compressions;
-while (current_compression < end_of_compressions) {
-  const char *next_compression_name = strchr(current_compression, ',');
-  const char *end_of_this_word = next_compression_name;
-  if (next_compression_name == nullptr ||
-  end_of_compressions < next_compression_name) {
-end_of_this_word = end_of_compressions;
-  }
-
-  if (end_of_this_word) {
-if (end_of_this_word == current_compression) {
-  current_compression++;
-} else {
-  std::string this_compression(
-  current_compression, end_of_this_word - current_compression);
-  supported_compressions.push_back(this_compression);
-  current_compression = end_of_this_word + 1;
-}
-  } else {
-supported_compressions.push_back(current_compression);
-current_compression = end_of_compressions;
-  }
+m_qSupported_response = response.GetStringRef().str();
+

[Lldb-commits] [PATCH] D100146: [lldb] [gdb-remote client] Refactor handling qSupported

[Diana Picus via Phabricator via lldb-commits]

rovka accepted this revision.
rovka added a comment.
This revision is now accepted and ready to land.

LGTM


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100146/new/

https://reviews.llvm.org/D100146

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100146: [lldb] [gdb-remote client] Refactor handling qSupported

[Michał Górny via Phabricator via lldb-commits]

mgorny created this revision.
mgorny added reviewers: labath, emaste, krytarowski.
mgorny requested review of this revision.

Refactor the qSupported handler to split the reply into an array,
and identify features within the array rather than searching the string
for partial matches.  While at it, use StringRef.split() to process
the compression list instead of reinventing the wheel.

Switch the arguments to MaybeEnableCompression() to use an ArrayRef
of StringRefs to simplify parameter passing from GetRemoteQSupported().


https://reviews.llvm.org/D100146

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h

Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -601,7 +601,8 @@
 
   // Given the list of compression types that the remote debug stub can support,
   // possibly enable compression if we find an encoding we can handle.
-  void MaybeEnableCompression(std::vector supported_compressions);
+  void MaybeEnableCompression(
+  llvm::ArrayRef supported_compressions);
 
   bool DecodeProcessInfoResponse(StringExtractorGDBRemote ,
  ProcessInstanceInfo _info);
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -345,6 +345,9 @@
   m_supports_qXfer_features_read = eLazyBoolNo;
   m_supports_qXfer_memory_map_read = eLazyBoolNo;
   m_supports_multiprocess = eLazyBoolNo;
+  m_supports_qEcho = eLazyBoolNo;
+  m_supports_QPassSignals = eLazyBoolNo;
+
   m_max_packet_size = UINT64_MAX; // It's supposed to always be there, but if
   // not, we assume no limit
 
@@ -362,97 +365,51 @@
   if (SendPacketAndWaitForResponse(packet.GetString(), response,
/*send_async=*/false) ==
   PacketResult::Success) {
-const char *response_cstr = response.GetStringRef().data();
-
 // Hang on to the qSupported packet, so that platforms can do custom
 // configuration of the transport before attaching/launching the process.
-m_qSupported_response = response_cstr;
-
-if (::strstr(response_cstr, "qXfer:auxv:read+"))
-  m_supports_qXfer_auxv_read = eLazyBoolYes;
-if (::strstr(response_cstr, "qXfer:libraries-svr4:read+"))
-  m_supports_qXfer_libraries_svr4_read = eLazyBoolYes;
-if (::strstr(response_cstr, "augmented-libraries-svr4-read")) {
-  m_supports_qXfer_libraries_svr4_read = eLazyBoolYes; // implied
-  m_supports_augmented_libraries_svr4_read = eLazyBoolYes;
-}
-if (::strstr(response_cstr, "qXfer:libraries:read+"))
-  m_supports_qXfer_libraries_read = eLazyBoolYes;
-if (::strstr(response_cstr, "qXfer:features:read+"))
-  m_supports_qXfer_features_read = eLazyBoolYes;
-if (::strstr(response_cstr, "qXfer:memory-map:read+"))
-  m_supports_qXfer_memory_map_read = eLazyBoolYes;
-
-// Look for a list of compressions in the features list e.g.
-// qXfer:features:read+;PacketSize=2;qEcho+;SupportedCompressions=zlib-
-// deflate,lzma
-const char *features_list = ::strstr(response_cstr, "qXfer:features:");
-if (features_list) {
-  const char *compressions =
-  ::strstr(features_list, "SupportedCompressions=");
-  if (compressions) {
-std::vector supported_compressions;
-compressions += sizeof("SupportedCompressions=") - 1;
-const char *end_of_compressions = strchr(compressions, ';');
-if (end_of_compressions == nullptr) {
-  end_of_compressions = strchr(compressions, '\0');
-}
-const char *current_compression = compressions;
-while (current_compression < end_of_compressions) {
-  const char *next_compression_name = strchr(current_compression, ',');
-  const char *end_of_this_word = next_compression_name;
-  if (next_compression_name == nullptr ||
-  end_of_compressions < next_compression_name) {
-end_of_this_word = end_of_compressions;
-  }
-
-  if (end_of_this_word) {
-if (end_of_this_word == current_compression) {
-  current_compression++;
-} else {
-  std::string this_compression(
-  current_compression, end_of_this_word - current_compression);
-  supported_compressions.push_back(this_compression);
-  current_compression = end_of_this_word + 1;
-}
-  } else {
-