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

[Jim Ingham via lldb-commits]

> On Mar 6, 2017, at 4:10 PM, Greg Clayton via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> clayborg requested changes to this revision.
> clayborg added a comment.
> This revision now requires changes to proceed.
> 
> 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.
> 
> 
> 
> ================
> Comment at: include/lldb/Target/Process.h:3148
> 
> +  virtual Error UpdateAutomaticSignalFiltering();
> +
> ----------------
> 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.

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.

Jim


> 
> 
> ================
> Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h:401
> +  Error UpdateAutomaticSignalFiltering() override;
> +
> private:
> ----------------
> Remove the override if we handle this in UpdateAutomaticSignalFiltering in specific to ProcessGDBRemote only?
> 
> 
> https://reviews.llvm.org/D30520
> 
> 
>