[Lldb-commits] [PATCH] D30520: Make LLDB skip server-client roundtrip for signals that don't require any actions

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Jim's comment about putting the function in the parent class makes sense -- apart from remote stubs I guess you could also envision other ways a process class could speed up signal processing in case it know we don't care about them).

Jim: do you have a idea about how to write a test for this? These performance-improvement kind of changes don't fit really well into the SB API test framework, as this change should not have any observable behavior, and we already have tests that make sure that "process handle" does the right thing (i.e., we don't stop for ignored signals). The best thing I can come up with (without going on a major refactoring effort) is enabling logging and then grepping it for some string.

Actually, I can think of a potential improvement. Move the code currently in `ProcessGDBRemote::SendSignalsToIgnoreIfNeeded` to `GDBRemoteCommunicationClient`. The latter is unit-testable so it would give as more coverage, and I think an API like `Error GDBRemoteCommunicationClient::IgnoreSignalsIfSupported(const UnixSignals &signals)` would still make sense. It won't solve the problem of testing the whole thing end-to-end, but it would certainly increase the test coverage of this change.

Let me know what you think.



================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:1067
                     "using %s %s",
-                    __FUNCTION__, process_arch.GetArchitectureName()
-                                      ? process_arch.GetArchitectureName()
----------------
It looks like you reformatted the whole file instead of just your changes. Random diff makes review harder. If you set it up correctly `git clang-format` should only format the code around your changes, so I recommend using that.


================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:3795
+    LLDB_LOG(log, "Error: {0}", error);
+  }
+  return error;
----------------
No braces.


================
Comment at: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:332
+
+  HandlePacket(server, "QPassSignals:2;3;5;7;B;D;11", "OK");
+  EXPECT_TRUE(result.get().Success());
----------------
The documentation for QPassSignals <https://sourceware.org/gdb/onlinedocs/gdb/General-Query-Packets.html> states that "Signals are numbered identically to continue packets and stop replies". While it does not say that directly, I would interpret that as "you should always use two digits for the signal number" because that's how stop-reply packets work. It's fine to keep the receiving code lax, but let's be stricter  in what we generate.


https://reviews.llvm.org/D30520