Make LLDB skip server-client roundtrip for signals that don't require any actions

[eugene]

Summary

If QPassSignals packaet is supported by lldb-server, lldb-client will utilize it and ask the server to ignore signals that don't require stops or notifications.
Such signals will be immediately re-injected into inferior to continue normal execution.

[jingham]

The ability to send a set of signals for the remote stub to ignore isn't inherently specific to the GDB Remote protocol. Some other remote protocol could very well implement the same functionality. SendSignalsToIgnoreIfNeeded seems like it should be in Process, and called in the Resume before DoResume gets called and in DidLaunch.

[jingham]

Of course, the default implementation would do nothing, and you would override it with your GDB remote protocol specific version.

[jingham]

Also, it would be good to add a test that ensures that calling "process handle" actually passes the right thing to your SendSignalsToIgnore.

[labath]

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.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

1067 ↗

(On Diff #90265)

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.

3795 ↗

(On Diff #90265)

No braces.

unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

332 ↗

(On Diff #90265)

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.

[jingham]

Note that ProcessGDBRemote (or maybe the client) keeps a history array of packets sent. Greg added that long ago so that when you enabled the packet log you would get the packet history up to the time you enabled it, and not just the new packets. That has often come in quite handy. And having a SB API like:

bool SBProcess::HasRemoteTrafficLog();
SBStringList SBProcess::DumpRemoteTraffic(<some kind of checkpoint>);

doesn't seem like a bad API. I could imagine some really horribly geeky debugger UI that would want to have a packet traffic window. Then using the checkpoint, you could get the strings from event to event and look for the packets you expected. That seems a lot of work for testing this bit of code, however.

Short of that, Pavel's last suggestion seems right for testing that the contents of the Process's m_unix_signals_sp is correctly copied into the ignore signals packet you are going to send. The higher level tests already ensure that we are correctly filling m_unix_signals_sp from commands or SB API's, but this last step is uncovered. So if you move the API to the communication client, and pass in the signals, you can then test that last step in isolation. And it seems perfectly reasonable for the Communication client to be doing the actual work of copying the signals over.

Another bit that isn't tested is that if you change the signals in the process, you will trigger resending the packet. You could test more of this path if:

(a) you had the UnixSignals class be the one that returns the array of signals to ignore rather than it being freestanding logic in the ProcessGDBRemote class; the bit of logic that checks pass, stop & notify seems more appropriate in the signals class anyway.

(b) change the m_last_signals_version logic to a "HasChanged" and "SetHasChanged(bool changed)" API vended by UnixSignals. SetHasChanged would be called passing false in the Process::SendSignalsToIgnoreIfNeeded, that seems pretty clearly right. And you probably have to call SetHasChanged(true) to force things for the launch case, but that should be straightforward.

Then you could test most of the chain by making your own UnixSignals and passing that to the GDBRemoteCommunicationClient. If you change it and pass it in again, you could ensure that the packet is sent, and if you don't change it that it is not. That leaves out that Process handles the SetHasChanged properly but it gets pretty close.

[eugene]

Addressing code review commends, and moving a signal filtering method to the base Process class.

[jingham]

I added a few comments. It doesn't look like you addressed Pavel's idea for testing more of the code path you're introducing. Do you plan to do that?

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

3766–3776 ↗

(On Diff #90407)

This code should go into the UnixSignals class. Any other Process plugin that wanted to implement expedited signal handling would also have to do this computation, and we shouldn't duplicate the code.

source/Target/Process.cpp

1629–1630 ↗

(On Diff #90407)

For this to work, you are relying on the Constructor of the signals class to set up the object by calling AddSignal on all the signals. Otherwise initially m_version in the signals class and m_last_signal_version will start out at 0 and you won't prime the filtering here.

You should probably add a comment to that effect somewhere in UnixSignals so we don't forget this dependency.

[eugene]

Added comment to UnixSignals::m_version.
Still thinking about the best way to test it all.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

3766–3776 ↗

(On Diff #90407)

I think it would be a mistake to over-engineer it and try to anticipate needs of any possible implementation at this point.
Maybe another implementation would require a list of signals that need to be stopped instead of ignored.

Whenever we have an alternative implementation that needs to do such thins we can always go back and generalize this code, as for now I think we need to keep things simple.

I like the concise interface that UnixSignals provides and I don't want to pollute it with things can be easily done in the code that uses it.

source/Target/Process.cpp

1629–1630 ↗

(On Diff #90407)

I added a comment to UnixSignals::m_version

[labath]

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

3766–3776 ↗

(On Diff #90407)

I agree with Eugene. I think there are advantages to the UnixSignals class being "dumb", and having the process plugin decide what to do with the data there. If anything, what this code needs is a better signal iteration API, so you could write this as something like:

for (auto signal: signals) {
  if (!signal.stop && !signal.notify && !signal.supress)
    my_list.push_back(signal.number);
}

[jingham]

That still seems wrong to me. The UnixSignals class manages the data about signal state including how to respond to them, so it should be the class that comprehends that data. The mental exercise of "if I had to do this in another place, would I have to duplicate an algorithm wholly specific to this class" is not over-engineering, it's how you decide where responsibility lies. And in this case it's clear that if another process class wanted to do this job, it would have to duplicate this code, which means it doesn't belong in Process.

[eugene]

Moved signal filtering to UnixSignals and added unit tests for UnixSignals class.

[labath]

Looks great, just a couple of style nits in the tests.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

82 ↗

(On Diff #90563)

Is this include still necessary?

unittests/Signals/UnixSignalsTest.cpp

43 ↗

(On Diff #90563)

This (and the function) should probably have an EXPECT_.. prefix instead, as it does not abort the evaluation of the function it is in (like other ASSERT_*** macros).

64 ↗

(On Diff #90563)

We generally put the "expected" value first, and the observed second. The ASSERT_ macro does that, but here you seem to have inverted it.

[jingham]

That looks good. This is a good addition for processes that are really chatty with some signals. Thanks for working on this.

[clayborg]

Pretty close. My only objection is we have many "lldb_private::Process::Will" and "lldb_private::Process::Did" prefixed functions and none of them are required to call the superclass version. I would prefer that this doesn't change. See my inlined comments,

source/Target/Process.cpp

1624–1627 ↗

(On Diff #90563)

I would prefer to not require people to call the base class functions for any "Process::Will*" or "Process::Did*" there are many of these and the ideas are that these are things that can be overridden. These aren't called in that many places, so it will be easy to just call this function before calling it, or adding a Process::PrivateWillResume that calls UpdateAutomaticSignalFiltering() followed by Process::WillResume().

1629 ↗

(On Diff #90563)

Ditto.

unittests/Signals/UnixSignalsTest.cpp

64 ↗

(On Diff #90563)

Is this "expected" value being first documented somewhere? If not it should be. I have seen this comment a few times and it would be great if the headerdoc already says this.

[clayborg]

My main objection is that if we have 10 "lldb_private::Process::Will*" functions and only some require you to call the superclass, then it is confusing. It is also hard to enforce. We probably have other process subclasses that override these functions and they all would be broken. It also makes it harder when merging code to other branches that might have an extra process subclass. The merge would go fine, but any process subclasses that exist only in other branches would now be out of date and doing the wrong thing by not calling the superclass.

[eugene]

Addressing review comments on SignalTests and getting rid of dependency on DidLaunch and WillResume

[eugene]

Addressing review comments.

unittests/Signals/UnixSignalsTest.cpp

43 ↗

(On Diff #90563)

This function calls ASSERT_EQ so it does abort evaluation.

[clayborg]

Very close. Can we try to get UpdateAutomaticSignalFiltering out of lldb_private::Process as my inline comments suggest? It would be cleaner and I am not sure we actually need Process::UpdateAutomaticSignalFiltering() for all processes.

include/lldb/Target/Process.h

3148 ↗

(On Diff #90755)

Can we remove this and only have it in ProcessGDBRemote? Then we just call it when we need to in ProcessGDBRemote? This seems like something that ProcessGDBRemote should take care of without having Process knowing about it. Seems like a adding a call to ProcessGDBRemote::UpdateAutomaticSignalFiltering() can be done in ProcessGDBRemote::WillResume() and ProcessGDBRemote::DidLaunch() might do the trick? It seems like each process plug-in will have different needs regarding signals.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

401 ↗

(On Diff #90755)

Remove the override if we handle this in UpdateAutomaticSignalFiltering in specific to ProcessGDBRemote only?

[eugene]

include/lldb/Target/Process.h

3148 ↗

(On Diff #90755)

I 100% agree with you here. UpdateAutomaticSignalFiltering() is very much implementation specific and it shouldn't be in the lldb_private::Process. And it was how I originally implemented it in older revision.

But Jim felt differently and I trusted his judgement given that he has vastly more experience working with LLDB codebase.

[clayborg]

Jim Ingham said (in email):

I disagree. The different processes are at this point more about transport than about the platform details. That indicates to me that it's more likely that different process implementations will have different ways of implementing signal filtering, but the input of which signals will be filtered will be the same. Plus for each process the place where you would implement turning on and changing the filtering are the same. So it makes more sense to have the part that says "go do this filtering" all be in common code, and the particular plugins implement how to do this.

That is fine. Seems like the UnixSignals class should be coordinating this instead of doing it manually in the process? Seems like the UnixSignals class should have guts here and call through to the Process::UpdateAutomaticSignalFiltering() only if needed?

[eugene]

UnixSignals is a nice self contained class that already does 99% of the work (see UnixSignals::GetFilteredSignals). I don't think we should have it call anybody.
Only process knows when it is the right time to send actual QPassSignals packet, there is not need to somehow push this knowledge into UnixSignals.

Let's just decide if UpdateAutomaticSignalFiltering() is specific enough to live in GDBProcess or it's generic enough to live in the base process. I'm fine either way.

[clayborg]

Ok, sounds like everyone thought through the solution. Lets start with this and we can iterate if needed.

[labath]

unittests/Signals/UnixSignalsTest.cpp

43 ↗

(On Diff #90563)

Unfortunately it does not. If you look up what ASSERT_EQ does, you'll see that it basically amounts to EXPECT_EQ(a,b); return; https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#assertion-placement, https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#using-assertions-in-sub-routines. So it does abort, but only the top most frame. Since your topmost frame is the AssertEqArrays function, it will just do an early return from that, and the caller will happily continue. If you wanted to be fancy you could define the macro to something like CompareArrays(...); if(HasFailure()) return;. Another solution would be to write a comparison function that returns an AssertionResult https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#using-a-function-that-returns-an-assertionresult and then use that in EXPECT_TRUE/ASSERT_TRUE as you see fit (but that's probably overkill).

[eugene]

Rename ASSERT_EQ_ARRAYS to EXPECT_EQ_ARRAYS

[labath]

Thank you.

lldb/trunk/include/lldb/Target/Process.h

 
   typedef bool(PreResumeActionCallback)(void *);
 
   void AddPreResumeAction(PreResumeActionCallback callback, void *baton);
 
   bool RunPreResumeActions();
 
   void ClearPreResumeActions();
-  
   void ClearPreResumeAction(PreResumeActionCallback callback, void *baton);
 
   ProcessRunLock &GetRunLock();
 
   virtual Error SendEventData(const char *data) {
     Error return_error("Sending an event is not supported for this process.");
     return return_error;
   }
protected:
   bool ProcessIOHandlerIsActive();
 
   bool ProcessIOHandlerExists() const {
     return static_cast<bool>(m_process_input_reader);
   }
 
   Error StopForDestroyOrDetach(lldb::EventSP &exit_event_sp);
 
+  virtual Error UpdateAutomaticSignalFiltering();
+
   bool StateChangedIsExternallyHijacked();
 
   void LoadOperatingSystemPlugin(bool flush);
 
 private:
   //------------------------------------------------------------------
   /// This is the part of the event handling that for a process event.
   /// It decides what to do with the event and returns true if the

lldb/trunk/include/lldb/Target/UnixSignals.h

 //===-- UnixSignals.h -------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef lldb_UnixSignals_h_
 #define lldb_UnixSignals_h_
 
 // C Includes
 // C++ Includes
 #include <map>
 #include <string>
+#include <vector>
 
 // Other libraries and framework includes
 // Project includes
 #include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-private.h"
+#include "llvm/ADT/Optional.h"
 
 namespace lldb_private {
 
 class UnixSignals {
 public:
   static lldb::UnixSignalsSP Create(const ArchSpec &arch);
 
   //------------------------------------------------------------------
public:
   // UnixSignal object.
 
   void AddSignal(int signo, const char *name, bool default_suppress,
                  bool default_stop, bool default_notify,
                  const char *description, const char *alias = nullptr);
 
   void RemoveSignal(int signo);
 
+  // Returns a current version of the data stored in this class.
+  // Version gets incremented each time Set... method is called.
+  uint64_t GetVersion() const;
+
+  // Returns a vector of signals that meet criteria provided in arguments.
+  // Each should_[suppress|stop|notify] flag can be
+  // None  - no filtering by this flag
+  // true  - only signals that have it set to true are returned
+  // false - only signals that have it set to true are returned
+  std::vector<int32_t> GetFilteredSignals(llvm::Optional<bool> should_suppress,
+                                          llvm::Optional<bool> should_stop,
+                                          llvm::Optional<bool> should_notify);
+
 protected:
   //------------------------------------------------------------------
   // Classes that inherit from UnixSignals can see and modify these
   //------------------------------------------------------------------
 
   struct Signal {
     ConstString m_name;
     ConstString m_alias;
     std::string m_description;
     bool m_suppress : 1, m_stop : 1, m_notify : 1;
 
     Signal(const char *name, bool default_suppress, bool default_stop,
            bool default_notify, const char *description, const char *alias);
 
     ~Signal() {}
   };
 
   virtual void Reset();
 
   typedef std::map<int32_t, Signal> collection;
 
   collection m_signals;
 
+  // This version gets incremented every time something is changing in
+  // this class, including when we call AddSignal from the constructor.
+  // So after the object is constructed m_version is going to be > 0
+  // if it has at least one signal registered in it.
+  uint64_t m_version = 0;
+
   // GDBRemote signals need to be copyable.
   UnixSignals(const UnixSignals &rhs);
 
   const UnixSignals &operator=(const UnixSignals &rhs) = delete;
 };
 
 } // Namespace lldb
 #endif // lldb_UnixSignals_h_

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h

public:
   bool GetQXferLibrariesReadSupported();
 
   bool GetQXferLibrariesSVR4ReadSupported();
 
   uint64_t GetRemoteMaxPacketSize();
 
   bool GetEchoSupported();
 
+  bool GetQPassSignalsSupported();
+
   bool GetAugmentedLibrariesSVR4ReadSupported();
 
   bool GetQXferFeaturesReadSupported();
 
   LazyBool SupportsAllocDeallocMemory() // const
   {
     // Uncomment this to have lldb pretend the debug server doesn't respond to
     // alloc/dealloc memory packets.
GetModulesInfo(llvm::ArrayRef<FileSpec> module_file_specs,
                  const llvm::Triple &triple);
 
   bool ReadExtFeature(const lldb_private::ConstString object,
                       const lldb_private::ConstString annex, std::string &out,
                       lldb_private::Error &err);
 
   void ServeSymbolLookups(lldb_private::Process *process);
 
+  // Sends QPassSignals packet to the server with given signals to ignore.
+  Error SendSignalsToIgnore(llvm::ArrayRef<int32_t> signals);
+
   //------------------------------------------------------------------
   /// Return the feature set supported by the gdb-remote server.
   ///
   /// This method returns the remote side's response to the qSupported
   /// packet.  The response is the complete string payload returned
   /// to the client.
   ///
   /// @return
protected:
   LazyBool m_supports_qXfer_auxv_read;
   LazyBool m_supports_qXfer_libraries_read;
   LazyBool m_supports_qXfer_libraries_svr4_read;
   LazyBool m_supports_qXfer_features_read;
   LazyBool m_supports_augmented_libraries_svr4_read;
   LazyBool m_supports_jThreadExtendedInfo;
   LazyBool m_supports_jLoadedDynamicLibrariesInfos;
   LazyBool m_supports_jGetSharedCacheInfo;
+  LazyBool m_supports_QPassSignals;
 
   bool m_supports_qProcessInfoPID : 1, m_supports_qfProcessInfo : 1,
       m_supports_qUserName : 1, m_supports_qGroupName : 1,
       m_supports_qThreadStopInfo : 1, m_supports_z0 : 1, m_supports_z1 : 1,
       m_supports_z2 : 1, m_supports_z3 : 1, m_supports_z4 : 1,
       m_supports_QEnvironment : 1, m_supports_QEnvironmentHexEncoded : 1,
       m_supports_qSymbol : 1, m_qSymbol_requests_done : 1,
       m_supports_qModuleInfo : 1, m_supports_jThreadsInfo : 1,

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

: GDBRemoteClientBase("gdb-remote.client", "gdb-remote.client.rx_packet"),
       m_supports_qXfer_auxv_read(eLazyBoolCalculate),
       m_supports_qXfer_libraries_read(eLazyBoolCalculate),
       m_supports_qXfer_libraries_svr4_read(eLazyBoolCalculate),
       m_supports_qXfer_features_read(eLazyBoolCalculate),
       m_supports_augmented_libraries_svr4_read(eLazyBoolCalculate),
       m_supports_jThreadExtendedInfo(eLazyBoolCalculate),
       m_supports_jLoadedDynamicLibrariesInfos(eLazyBoolCalculate),
       m_supports_jGetSharedCacheInfo(eLazyBoolCalculate),
+      m_supports_QPassSignals(eLazyBoolCalculate),
       m_supports_qProcessInfoPID(true), m_supports_qfProcessInfo(true),
       m_supports_qUserName(true), m_supports_qGroupName(true),
       m_supports_qThreadStopInfo(true), m_supports_z0(true),
       m_supports_z1(true), m_supports_z2(true), m_supports_z3(true),
       m_supports_z4(true), m_supports_QEnvironment(true),
       m_supports_QEnvironmentHexEncoded(true), m_supports_qSymbol(true),
       m_qSymbol_requests_done(false), m_supports_qModuleInfo(true),
       m_supports_jThreadsInfo(true), m_supports_jModulesInfo(true),

 
 bool GDBRemoteCommunicationClient::GetEchoSupported() {
   if (m_supports_qEcho == eLazyBoolCalculate) {
     GetRemoteQSupported();
   }
   return m_supports_qEcho == eLazyBoolYes;
 }
 
+bool GDBRemoteCommunicationClient::GetQPassSignalsSupported() {
+  if (m_supports_QPassSignals == eLazyBoolCalculate) {
+    GetRemoteQSupported();
+  }
+  return m_supports_QPassSignals == eLazyBoolYes;
+}
+
 bool GDBRemoteCommunicationClient::GetAugmentedLibrariesSVR4ReadSupported() {
   if (m_supports_augmented_libraries_svr4_read == eLazyBoolCalculate) {
     GetRemoteQSupported();
   }
   return m_supports_augmented_libraries_svr4_read == eLazyBoolYes;
 }
 
 bool GDBRemoteCommunicationClient::GetQXferLibrariesSVR4ReadSupported() {
if (features_list) {
       }
     }
 
     if (::strstr(response_cstr, "qEcho"))
       m_supports_qEcho = eLazyBoolYes;
     else
       m_supports_qEcho = eLazyBoolNo;
 
+    if (::strstr(response_cstr, "QPassSignals+"))
+      m_supports_QPassSignals = eLazyBoolYes;
+    else
+      m_supports_QPassSignals = eLazyBoolNo;
+
     const char *packet_size_str = ::strstr(response_cstr, "PacketSize=");
     if (packet_size_str) {
       StringExtractorGDBRemote packet_response(packet_size_str +
                                                strlen("PacketSize="));
       m_max_packet_size =
           packet_response.GetHexMaxU64(/*little_endian=*/false, UINT64_MAX);
       if (m_max_packet_size == 0) {
         m_max_packet_size = UINT64_MAX; // Must have been a garbled response
if (!m_supported_async_json_packets_is_valid) {
     }
   }
 
   return m_supported_async_json_packets_sp
              ? m_supported_async_json_packets_sp->GetAsArray()
              : nullptr;
 }
 
+Error GDBRemoteCommunicationClient::SendSignalsToIgnore(
+    llvm::ArrayRef<int32_t> signals) {
+  // Format packet:
+  // QPassSignals:<hex_sig1>;<hex_sig2>...;<hex_sigN>
+  auto range = llvm::make_range(signals.begin(), signals.end());
+  std::string packet = formatv("QPassSignals:{0:$[;]@(x-2)}", range).str();
+
+  StringExtractorGDBRemote response;
+  auto send_status = SendPacketAndWaitForResponse(packet, response, false);
+
+  if (send_status != GDBRemoteCommunication::PacketResult::Success)
+    return Error("Sending QPassSignals packet failed");
+
+  if (response.IsOKResponse()) {
+    return Error();
+  } else {
+    return Error("Unknown error happened during sending QPassSignals packet.");
+  }
+}
+
 Error GDBRemoteCommunicationClient::ConfigureRemoteStructuredData(
     const ConstString &type_name, const StructuredData::ObjectSP &config_sp) {
   Error error;
 
   if (type_name.GetLength() == 0) {
     error.SetErrorString("invalid type_name argument");
     return error;
   }

lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

protected:
   // Query remote GDBServer for a detailed loaded library list
   Error GetLoadedModuleList(LoadedModuleInfoList &);
 
   lldb::ModuleSP LoadModuleAtAddress(const FileSpec &file,
                                      lldb::addr_t link_map,
                                      lldb::addr_t base_addr,
                                      bool value_is_offset);
 
+  Error UpdateAutomaticSignalFiltering() override;
+
 private:
   //------------------------------------------------------------------
   // For ProcessGDBRemote only
   //------------------------------------------------------------------
   std::string m_partial_profile_data;
   std::map<uint64_t, uint32_t> m_thread_id_to_used_usec_map;
+  uint64_t m_last_signals_version = 0;
 
   static bool NewThreadNotifyBreakpointHit(void *baton,
                                            StoppointCallbackContext *context,
                                            lldb::user_id_t break_id,
                                            lldb::user_id_t break_loc_id);
 
   //------------------------------------------------------------------
   // ContinueDelegate interface

lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

 #include "Plugins/Process/Utility/StopInfoMachException.h"
 #include "ProcessGDBRemote.h"
 #include "ProcessGDBRemoteLog.h"
 #include "ThreadGDBRemote.h"
 #include "Utility/StringExtractorGDBRemote.h"
 #include "lldb/Host/Host.h"
 
 #include "llvm/ADT/StringSwitch.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/Threading.h"
+#include "llvm/Support/raw_ostream.h"
 
 #define DEBUGSERVER_BASENAME "debugserver"
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::process_gdb_remote;
 
 namespace lldb {
 // Provide a function that can easily dump the packet history if we know a
bool ProcessGDBRemote::NewThreadNotifyBreakpointHit(
   // thread when it starts to
   // run so I can stop it if that's what I want to do.
   Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_STEP));
   if (log)
     log->Printf("Hit New Thread Notification breakpoint.");
   return false;
 }
 
+Error ProcessGDBRemote::UpdateAutomaticSignalFiltering() {
+  Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
+  LLDB_LOG(log, "Check if need to update ignored signals");
+
+  // QPassSignals package is not supported by the server,
+  // there is no way we can ignore any signals on server side.
+  if (!m_gdb_comm.GetQPassSignalsSupported())
+    return Error();
+
+  // No signals, nothing to send.
+  if (m_unix_signals_sp == nullptr)
+    return Error();
+
+  // Signals' version hasn't changed, no need to send anything.
+  uint64_t new_signals_version = m_unix_signals_sp->GetVersion();
+  if (new_signals_version == m_last_signals_version) {
+    LLDB_LOG(log, "Signals' version hasn't changed. version={0}",
+             m_last_signals_version);
+    return Error();
+  }
+
+  auto signals_to_ignore =
+      m_unix_signals_sp->GetFilteredSignals(false, false, false);
+  Error error = m_gdb_comm.SendSignalsToIgnore(signals_to_ignore);
+
+  LLDB_LOG(log,
+           "Signals' version changed. old version={0}, new version={1}, "
+           "signals ignored={2}, update result={3}",
+           m_last_signals_version, new_signals_version,
+           signals_to_ignore.size(), error);
+
+  if (error.Success())
+    m_last_signals_version = new_signals_version;
+
+  return error;
+}
+
 bool ProcessGDBRemote::StartNoticingNewThreads() {
   Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_STEP));
   if (m_thread_create_bp_sp) {
     if (log && log->GetVerbose())
       log->Printf("Enabled noticing new thread breakpoint.");
     m_thread_create_bp_sp->SetEnabled(true);
   } else {
     PlatformSP platform_sp(GetTarget().GetPlatform());

lldb/trunk/source/Target/Process.cpp

if (exe_module->GetFileSpec().Exists()) {
 
             SystemRuntime *system_runtime = GetSystemRuntime();
             if (system_runtime)
               system_runtime->DidLaunch();
 
             if (!m_os_ap)
                 LoadOperatingSystemPlugin(false);
 
+            // We successfully launched the process and stopped,
+            // now it the right time to set up signal filters before resuming.
+            UpdateAutomaticSignalFiltering();
+
             // Note, the stop event was consumed above, but not handled. This
             // was done
             // to give DidLaunch a chance to run. The target is either stopped
             // or crashed.
             // Directly set the state.  This is done to prevent a stop message
             // with a bunch
             // of spurious output on thread status, as well as not pop a
             // ProcessIOHandler.
Error Process::PrivateResume() {
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS |
                                                   LIBLLDB_LOG_STEP));
   if (log)
     log->Printf("Process::PrivateResume() m_stop_id = %u, public state: %s "
                 "private state: %s",
                 m_mod_id.GetStopID(), StateAsCString(m_public_state.GetValue()),
                 StateAsCString(m_private_state.GetValue()));
 
+  // If signals handing status changed we might want to update
+  // our signal filters before resuming.
+  UpdateAutomaticSignalFiltering();
+
   Error error(WillResume());
   // Tell the process it is about to resume before the thread list
   if (error.Success()) {
     // Now let the thread list know we are about to resume so it
     // can let all of our threads know that they are about to be
     // resumed. Threads will each be called with
     // Thread::WillResume(StateType) where StateType contains the state
     // that they are supposed to have when the process is resumed
if (find_it == m_structured_data_plugin_map.end()) {
     // We don't have a mapping for this structured data type.
     return false;
   }
 
   // Route the structured data to the plugin.
   find_it->second->HandleArrivalOfStructuredData(*this, type_name, object_sp);
   return true;
 }
+
+Error Process::UpdateAutomaticSignalFiltering() {
+  // Default implementation does nothign.
+  // No automatic signal filtering to speak of.
+  return Error();
+}

lldb/trunk/source/Target/UnixSignals.cpp

 }
 
 void UnixSignals::AddSignal(int signo, const char *name, bool default_suppress,
                             bool default_stop, bool default_notify,
                             const char *description, const char *alias) {
   Signal new_signal(name, default_suppress, default_stop, default_notify,
                     description, alias);
   m_signals.insert(std::make_pair(signo, new_signal));
+  ++m_version;
 }
 
 void UnixSignals::RemoveSignal(int signo) {
   collection::iterator pos = m_signals.find(signo);
   if (pos != m_signals.end())
     m_signals.erase(pos);
+  ++m_version;
 }
 
 const char *UnixSignals::GetSignalAsCString(int signo) const {
   collection::const_iterator pos = m_signals.find(signo);
   if (pos == m_signals.end())
     return nullptr;
   else
     return pos->second.m_name.GetCString();
if (pos != m_signals.end())
     return pos->second.m_suppress;
   return false;
 }
 
 bool UnixSignals::SetShouldSuppress(int signo, bool value) {
   collection::iterator pos = m_signals.find(signo);
   if (pos != m_signals.end()) {
     pos->second.m_suppress = value;
+    ++m_version;
     return true;
   }
   return false;
 }
 
 bool UnixSignals::SetShouldSuppress(const char *signal_name, bool value) {
   const int32_t signo = GetSignalNumberFromName(signal_name);
   if (signo != LLDB_INVALID_SIGNAL_NUMBER)
     return SetShouldSuppress(signo, value);
   return false;
 }
 
 bool UnixSignals::GetShouldStop(int signo) const {
   collection::const_iterator pos = m_signals.find(signo);
   if (pos != m_signals.end())
     return pos->second.m_stop;
   return false;
 }
 
 bool UnixSignals::SetShouldStop(int signo, bool value) {
   collection::iterator pos = m_signals.find(signo);
   if (pos != m_signals.end()) {
     pos->second.m_stop = value;
+    ++m_version;
     return true;
   }
   return false;
 }
 
 bool UnixSignals::SetShouldStop(const char *signal_name, bool value) {
   const int32_t signo = GetSignalNumberFromName(signal_name);
   if (signo != LLDB_INVALID_SIGNAL_NUMBER)
     return SetShouldStop(signo, value);
   return false;
 }
 
 bool UnixSignals::GetShouldNotify(int signo) const {
   collection::const_iterator pos = m_signals.find(signo);
   if (pos != m_signals.end())
     return pos->second.m_notify;
   return false;
 }
 
 bool UnixSignals::SetShouldNotify(int signo, bool value) {
   collection::iterator pos = m_signals.find(signo);
   if (pos != m_signals.end()) {
     pos->second.m_notify = value;
+    ++m_version;
     return true;
   }
   return false;
 }
 
 bool UnixSignals::SetShouldNotify(const char *signal_name, bool value) {
   const int32_t signo = GetSignalNumberFromName(signal_name);
   if (signo != LLDB_INVALID_SIGNAL_NUMBER)
     return SetShouldNotify(signo, value);
   return false;
 }
 
 int32_t UnixSignals::GetNumSignals() const { return m_signals.size(); }
 
 int32_t UnixSignals::GetSignalAtIndex(int32_t index) const {
   if (index < 0 || m_signals.size() <= static_cast<size_t>(index))
     return LLDB_INVALID_SIGNAL_NUMBER;
   auto it = m_signals.begin();
   std::advance(it, index);
   return it->first;
 }
+
+uint64_t UnixSignals::GetVersion() const { return m_version; }
+
+std::vector<int32_t>
+UnixSignals::GetFilteredSignals(llvm::Optional<bool> should_suppress,
+                                llvm::Optional<bool> should_stop,
+                                llvm::Optional<bool> should_notify) {
+  std::vector<int32_t> result;
+  for (int32_t signo = GetFirstSignalNumber();
+       signo != LLDB_INVALID_SIGNAL_NUMBER;
+       signo = GetNextSignalNumber(signo)) {
+
+    bool signal_suppress = false;
+    bool signal_stop = false;
+    bool signal_notify = false;
+    GetSignalInfo(signo, signal_suppress, signal_stop, signal_notify);
+
+    // If any of filtering conditions are not met,
+    // we move on to the next signal.
+    if (should_suppress.hasValue() &&
+        signal_suppress != should_suppress.getValue())
+      continue;
+
+    if (should_stop.hasValue() && signal_stop != should_stop.getValue())
+      continue;
+
+    if (should_notify.hasValue() && signal_notify != should_notify.getValue())
+      continue;
+
+    result.push_back(signo);
+  }
+
+  return result;
+}

lldb/trunk/unittests/CMakeLists.txt

 add_subdirectory(Expression)
 add_subdirectory(Host)
 add_subdirectory(Interpreter)
 add_subdirectory(Language)
 add_subdirectory(ObjectFile)
 add_subdirectory(Platform)
 add_subdirectory(Process)
 add_subdirectory(ScriptInterpreter)
+add_subdirectory(Signals)
 add_subdirectory(Symbol)
 add_subdirectory(SymbolFile)
 add_subdirectory(Target)
 add_subdirectory(UnwindAssembly)
 add_subdirectory(Utility)

lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

TEST_F(GDBRemoteCommunicationClientTest, TestPacketSpeedJSON) {
   dict_sp = object_sp->GetAsDictionary();
   ASSERT_TRUE(bool(dict_sp));
 
   int num_packets;
   ASSERT_TRUE(dict_sp->GetValueForKeyAsInteger("num_packets", num_packets))
       << ss.GetString();
   ASSERT_EQ(10, num_packets);
 }
+
+TEST_F(GDBRemoteCommunicationClientTest, SendSignalsToIgnore) {
+  TestClient client;
+  MockServer server;
+  Connect(client, server);
+  if (HasFailure())
+    return;
+
+  const lldb::tid_t tid = 0x47;
+  const uint32_t reg_num = 4;
+  std::future<Error> result = std::async(std::launch::async, [&] {
+    return client.SendSignalsToIgnore({2, 3, 5, 7, 0xB, 0xD, 0x11});
+  });
+
+  HandlePacket(server, "QPassSignals:02;03;05;07;0b;0d;11", "OK");
+  EXPECT_TRUE(result.get().Success());
+
+  result = std::async(std::launch::async, [&] {
+    return client.SendSignalsToIgnore(std::vector<int32_t>());
+  });
+
+  HandlePacket(server, "QPassSignals:", "OK");
+  EXPECT_TRUE(result.get().Success());
+}

lldb/trunk/unittests/Signals/CMakeLists.txt

+add_lldb_unittest(SignalsTests
+  UnixSignalsTest.cpp
+
+  LINK_LIBS
+    lldbTarget
+  )

lldb/trunk/unittests/Signals/UnixSignalsTest.cpp

+//===-- UnixSignalsTest.cpp -------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+#include <string>
+
+#include "gtest/gtest.h"
+
+#include "lldb/Target/UnixSignals.h"
+#include "llvm/Support/FormatVariadic.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using llvm::None;
+
+class TestSignals : public UnixSignals {
+public:
+  TestSignals() {
+    m_signals.clear();
+    AddSignal(2, "SIG2", false, true, true, "DESC2");
+    AddSignal(4, "SIG4", true, false, true, "DESC4");
+    AddSignal(8, "SIG8", true, true, true, "DESC8");
+    AddSignal(16, "SIG16", true, false, false, "DESC16");
+  }
+};
+
+void ExpectEqArrays(llvm::ArrayRef<int32_t> expected,
+                    llvm::ArrayRef<int32_t> observed, const char *file,
+                    int line) {
+  std::string location = llvm::formatv("{0}:{1}", file, line);
+  ASSERT_EQ(expected.size(), observed.size()) << location;
+
+  for (size_t i = 0; i < observed.size(); ++i) {
+    ASSERT_EQ(expected[i], observed[i])
+        << "array index: " << i << "location:" << location;
+  }
+}
+
+#define EXPECT_EQ_ARRAYS(expected, observed)                                   \
+  ExpectEqArrays((expected), (observed), __FILE__, __LINE__);
+
+TEST(UnixSignalsTest, Iteration) {
+  TestSignals signals;
+
+  EXPECT_EQ(4, signals.GetNumSignals());
+  EXPECT_EQ(2, signals.GetFirstSignalNumber());
+  EXPECT_EQ(4, signals.GetNextSignalNumber(2));
+  EXPECT_EQ(8, signals.GetNextSignalNumber(4));
+  EXPECT_EQ(16, signals.GetNextSignalNumber(8));
+  EXPECT_EQ(LLDB_INVALID_SIGNAL_NUMBER, signals.GetNextSignalNumber(16));
+}
+
+TEST(UnixSignalsTest, GetInfo) {
+  TestSignals signals;
+
+  bool should_suppress = false, should_stop = false, should_notify = false;
+  int32_t signo = 4;
+  std::string name =
+      signals.GetSignalInfo(signo, should_suppress, should_stop, should_notify);
+  EXPECT_EQ("SIG4", name);
+  EXPECT_EQ(true, should_suppress);
+  EXPECT_EQ(false, should_stop);
+  EXPECT_EQ(true, should_notify);
+
+  EXPECT_EQ(true, signals.GetShouldSuppress(signo));
+  EXPECT_EQ(false, signals.GetShouldStop(signo));
+  EXPECT_EQ(true, signals.GetShouldNotify(signo));
+  EXPECT_EQ(name, signals.GetSignalAsCString(signo));
+}
+
+TEST(UnixSignalsTest, VersionChange) {
+  TestSignals signals;
+
+  int32_t signo = 8;
+  uint64_t ver = signals.GetVersion();
+  EXPECT_GT(ver, 0ull);
+  EXPECT_EQ(true, signals.GetShouldSuppress(signo));
+  EXPECT_EQ(true, signals.GetShouldStop(signo));
+  EXPECT_EQ(true, signals.GetShouldNotify(signo));
+
+  EXPECT_EQ(signals.GetVersion(), ver);
+
+  signals.SetShouldSuppress(signo, false);
+  EXPECT_LT(ver, signals.GetVersion());
+  ver = signals.GetVersion();
+
+  signals.SetShouldStop(signo, true);
+  EXPECT_LT(ver, signals.GetVersion());
+  ver = signals.GetVersion();
+
+  signals.SetShouldNotify(signo, false);
+  EXPECT_LT(ver, signals.GetVersion());
+  ver = signals.GetVersion();
+
+  EXPECT_EQ(false, signals.GetShouldSuppress(signo));
+  EXPECT_EQ(true, signals.GetShouldStop(signo));
+  EXPECT_EQ(false, signals.GetShouldNotify(signo));
+
+  EXPECT_EQ(ver, signals.GetVersion());
+}
+
+TEST(UnixSignalsTest, GetFilteredSignals) {
+  TestSignals signals;
+
+  auto all_signals = signals.GetFilteredSignals(None, None, None);
+  std::vector<int32_t> expected = {2, 4, 8, 16};
+  EXPECT_EQ_ARRAYS(expected, all_signals);
+
+  auto supressed = signals.GetFilteredSignals(true, None, None);
+  expected = {4, 8, 16};
+  EXPECT_EQ_ARRAYS(expected, supressed);
+
+  auto not_supressed = signals.GetFilteredSignals(false, None, None);
+  expected = {2};
+  EXPECT_EQ_ARRAYS(expected, not_supressed);
+
+  auto stopped = signals.GetFilteredSignals(None, true, None);
+  expected = {2, 8};
+  EXPECT_EQ_ARRAYS(expected, stopped);
+
+  auto not_stopped = signals.GetFilteredSignals(None, false, None);
+  expected = {4, 16};
+  EXPECT_EQ_ARRAYS(expected, not_stopped);
+
+  auto notified = signals.GetFilteredSignals(None, None, true);
+  expected = {2, 4, 8};
+  EXPECT_EQ_ARRAYS(expected, notified);
+
+  auto not_notified = signals.GetFilteredSignals(None, None, false);
+  expected = {16};
+  EXPECT_EQ_ARRAYS(expected, not_notified);
+
+  auto signal4 = signals.GetFilteredSignals(true, false, true);
+  expected = {4};
+  EXPECT_EQ_ARRAYS(expected, signal4);
+}