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

Eugene Zemtsov via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Mar 2 18:47:10 PST 2017


eugene added inline comments.


================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:3766-3776
+  llvm::SmallVector<int32_t, 40> signals_to_ignore;
+  int32_t signo = m_unix_signals_sp->GetFirstSignalNumber();
+  while (signo != LLDB_INVALID_SIGNAL_NUMBER) {
+    bool should_ignore = !m_unix_signals_sp->GetShouldStop(signo) &&
+                         !m_unix_signals_sp->GetShouldNotify(signo) &&
+                         !m_unix_signals_sp->GetShouldSuppress(signo);
+
----------------
jingham wrote:
> 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.
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.


================
Comment at: source/Target/Process.cpp:1629-1630
+
+void Process::DidLaunch() { UpdateAutomaticSignalFiltering(); }
+
 Error Process::ResumeSynchronous(Stream *stream) {
----------------
jingham wrote:
> 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.
I added a comment to UnixSignals::m_version


https://reviews.llvm.org/D30520





More information about the lldb-commits mailing list