Page MenuHomePhabricator

[Signals] Create a plugin directory just for signals
Needs ReviewPublic

Authored by xiaobai on Jun 14 2019, 3:00 PM.

Details

Summary

I plan on setting up a plugin architecture for UnixSignals since I
don't think it makes sense for UnixSignals to know about all the
platform-specific signal classes. The first step is to organize the directories
in a way that makes this easier to do.

Event Timeline

xiaobai created this revision.Jun 14 2019, 3:00 PM
xiaobai updated this revision to Diff 204862.Jun 14 2019, 3:16 PM

Fix up include in gtest

Although this is technically correct and pretty consistent with our other "plugins", I can't help but feel that it's incredibly wasteful. Each of the XXXSignals.cpp files is less than a 100 lines long (with the licence header and all bolierplate) and it's unlikely to ever grow beyond that. And essentially, all these files do is define a single enum. The reason they are this long is because the UnixSignals class is already over-engineered (e.g. I don't see why LinuxSignals needs to be a separate class, or why it needs to repeat the descriptions/default stop values for each of the signals). Making this a plugin would just add another chunk of boilerplate on top of that.

I don't know about others, but I'd rather us move in a direction which reduces the amount of boilerplate instead of adding more of it. In my ideal world, each of these signal definitions would just be a bunch of (number, name) pairs. This doesn't have/need to be a class or a plugin; a single constexpr variable would suffice for that. Then we'd just cross-reference this mapping with another one which gives the default stop values and descriptions for each of the signals, and that's it.

I know I am repeating myself, but each time I say this, it's because I find another reason for it: I think we should start a new library which I would roughly define as "utilities for describing and manipulating various low-level aspects of processes, but which is agnostic of any actual process class". The idea would be that we can shove here all classes which are shared between lldb-server liblldb. UnixSignals would be one candidate for it. AuxVector, MemoryRegionInfo are others. Plugins/Process/Utility (where most of the signal classes live) would be a pretty good place for it already, were it not for the "Plugins" part (it would be weird for non-plugin code to depend on something called a "plugin"). However, maybe we could just create a new top-level library called "ProcessUtil" (or whatever name we come up with) and move the relevant stuff there.

Anyway, TL;DR: I think this should be handled differently. However, if others are fine with this approach, then feel free to ignore me.

Although this is technically correct and pretty consistent with our other "plugins", I can't help but feel that it's incredibly wasteful. Each of the XXXSignals.cpp files is less than a 100 lines long (with the licence header and all bolierplate) and it's unlikely to ever grow beyond that. And essentially, all these files do is define a single enum. The reason they are this long is because the UnixSignals class is already over-engineered (e.g. I don't see why LinuxSignals needs to be a separate class, or why it needs to repeat the descriptions/default stop values for each of the signals). Making this a plugin would just add another chunk of boilerplate on top of that.

I don't know about others, but I'd rather us move in a direction which reduces the amount of boilerplate instead of adding more of it. In my ideal world, each of these signal definitions would just be a bunch of (number, name) pairs. This doesn't have/need to be a class or a plugin; a single constexpr variable would suffice for that. Then we'd just cross-reference this mapping with another one which gives the default stop values and descriptions for each of the signals, and that's it.

I know I am repeating myself, but each time I say this, it's because I find another reason for it: I think we should start a new library which I would roughly define as "utilities for describing and manipulating various low-level aspects of processes, but which is agnostic of any actual process class". The idea would be that we can shove here all classes which are shared between lldb-server liblldb. UnixSignals would be one candidate for it. AuxVector, MemoryRegionInfo are others. Plugins/Process/Utility (where most of the signal classes live) would be a pretty good place for it already, were it not for the "Plugins" part (it would be weird for non-plugin code to depend on something called a "plugin"). However, maybe we could just create a new top-level library called "ProcessUtil" (or whatever name we come up with) and move the relevant stuff there.

Anyway, TL;DR: I think this should be handled differently. However, if others are fine with this approach, then feel free to ignore me.

I really don't like my approach here either and agree that the Signals support is over-engineered as is. I also think it's quite unfortunate that UnixSignals inside of Target by default has the Darwin signals instead of them being broken out into a separate file like the rest of the platforms. I am okay with this approach in the short term, though.

I think that the "ProcessUtil" idea is a good one. I kind of feel like "Plugins/Process/Utility" is a dumping ground for loosely related classes for manipulating processes', but let's see what other people think about that.

jfb added a comment.Jun 17 2019, 9:57 AM

Can you describe what the goal of your plugin architecture is? Maybe you need an RFC by email before moving stuff around.

I want to understand what you're going for because as they are today the signals mostly work, and aren't really tested (because injecting these conditions isn't trivial). Anything you change is likely to break some subtle invariant which will only repro when your change is deployed at scale.

In D63363#1546464, @jfb wrote:

Can you describe what the goal of your plugin architecture is? Maybe you need an RFC by email before moving stuff around.

I want to understand what you're going for because as they are today the signals mostly work, and aren't really tested (because injecting these conditions isn't trivial). Anything you change is likely to break some subtle invariant which will only repro when your change is deployed at scale.

My goal is to remove non-plugin libraries dependencies on plugins. Today, Target depends on the ProcessUtility plugin because of UnixSignals. If Signals were their own plugin that could be accessed through the PluginManager interface, that dependency would go away. As Pavel said, this feels somewhat over engineered and contrived, but it is the simplest path forward. I am willing to drop this patch and go for a different solution if there is a nicer solution agreed upon by the community, so an RFC sounds like a nice idea. :)

jfb added a comment.Jun 17 2019, 10:14 AM
In D63363#1546464, @jfb wrote:

Can you describe what the goal of your plugin architecture is? Maybe you need an RFC by email before moving stuff around.

I want to understand what you're going for because as they are today the signals mostly work, and aren't really tested (because injecting these conditions isn't trivial). Anything you change is likely to break some subtle invariant which will only repro when your change is deployed at scale.

My goal is to remove non-plugin libraries dependencies on plugins. Today, Target depends on the ProcessUtility plugin because of UnixSignals. If Signals were their own plugin that could be accessed through the PluginManager interface, that dependency would go away. As Pavel said, this feels somewhat over engineered and contrived, but it is the simplest path forward. I am willing to drop this patch and go for a different solution if there is a nicer solution agreed upon by the community, so an RFC sounds like a nice idea. :)

Removing the dependency seems fine, but we have to be careful with how signals work: they're inherently global state, and you want to register / de-register them exactly where they're registered / de-registered right now to keep them working exactly the same. If you change this, we need to really think through why that's a good idea (it might be!).

In D63363#1546504, @jfb wrote:
In D63363#1546464, @jfb wrote:

Can you describe what the goal of your plugin architecture is? Maybe you need an RFC by email before moving stuff around.

I want to understand what you're going for because as they are today the signals mostly work, and aren't really tested (because injecting these conditions isn't trivial). Anything you change is likely to break some subtle invariant which will only repro when your change is deployed at scale.

My goal is to remove non-plugin libraries dependencies on plugins. Today, Target depends on the ProcessUtility plugin because of UnixSignals. If Signals were their own plugin that could be accessed through the PluginManager interface, that dependency would go away. As Pavel said, this feels somewhat over engineered and contrived, but it is the simplest path forward. I am willing to drop this patch and go for a different solution if there is a nicer solution agreed upon by the community, so an RFC sounds like a nice idea. :)

Removing the dependency seems fine, but we have to be careful with how signals work: they're inherently global state, and you want to register / de-register them exactly where they're registered / de-registered right now to keep them working exactly the same. If you change this, we need to really think through why that's a good idea (it might be!).

I think there's some misunderstading here. This code has nothing to do with signal handlers. All these classes do is describe the set of possible signals on a given target (e.g. if remote-debugging a linux mips machine from a darwin x86 host, we need to know what number does linux use to represent SIGSEGV on mips, etc.)...

jfb added a comment.Jun 17 2019, 12:23 PM
In D63363#1546504, @jfb wrote:
In D63363#1546464, @jfb wrote:

Can you describe what the goal of your plugin architecture is? Maybe you need an RFC by email before moving stuff around.

I want to understand what you're going for because as they are today the signals mostly work, and aren't really tested (because injecting these conditions isn't trivial). Anything you change is likely to break some subtle invariant which will only repro when your change is deployed at scale.

My goal is to remove non-plugin libraries dependencies on plugins. Today, Target depends on the ProcessUtility plugin because of UnixSignals. If Signals were their own plugin that could be accessed through the PluginManager interface, that dependency would go away. As Pavel said, this feels somewhat over engineered and contrived, but it is the simplest path forward. I am willing to drop this patch and go for a different solution if there is a nicer solution agreed upon by the community, so an RFC sounds like a nice idea. :)

Removing the dependency seems fine, but we have to be careful with how signals work: they're inherently global state, and you want to register / de-register them exactly where they're registered / de-registered right now to keep them working exactly the same. If you change this, we need to really think through why that's a good idea (it might be!).

I think there's some misunderstading here. This code has nothing to do with signal handlers. All these classes do is describe the set of possible signals on a given target (e.g. if remote-debugging a linux mips machine from a darwin x86 host, we need to know what number does linux use to represent SIGSEGV on mips, etc.)...

OK, if that's the intended change, and scope doesn't widen further, it would be good to say so in the review. The description leads me to believe that all things signal-related are in scope, and this refactoring is step #1 in that goal.

Although this is technically correct and pretty consistent with our other "plugins", I can't help but feel that it's incredibly wasteful. Each of the XXXSignals.cpp files is less than a 100 lines long (with the licence header and all bolierplate) and it's unlikely to ever grow beyond that. And essentially, all these files do is define a single enum. The reason they are this long is because the UnixSignals class is already over-engineered (e.g. I don't see why LinuxSignals needs to be a separate class, or why it needs to repeat the descriptions/default stop values for each of the signals). Making this a plugin would just add another chunk of boilerplate on top of that.

I don't know about others, but I'd rather us move in a direction which reduces the amount of boilerplate instead of adding more of it. In my ideal world, each of these signal definitions would just be a bunch of (number, name) pairs. This doesn't have/need to be a class or a plugin; a single constexpr variable would suffice for that. Then we'd just cross-reference this mapping with another one which gives the default stop values and descriptions for each of the signals, and that's it.

I know I am repeating myself, but each time I say this, it's because I find another reason for it: I think we should start a new library which I would roughly define as "utilities for describing and manipulating various low-level aspects of processes, but which is agnostic of any actual process class". The idea would be that we can shove here all classes which are shared between lldb-server liblldb. UnixSignals would be one candidate for it. AuxVector, MemoryRegionInfo are others. Plugins/Process/Utility (where most of the signal classes live) would be a pretty good place for it already, were it not for the "Plugins" part (it would be weird for non-plugin code to depend on something called a "plugin"). However, maybe we could just create a new top-level library called "ProcessUtil" (or whatever name we come up with) and move the relevant stuff there.

Anyway, TL;DR: I think this should be handled differently. However, if others are fine with this approach, then feel free to ignore me.

How would you bind a particular variant of UnixSignals to the process plugin that it goes along with in this scenario?

Although this is technically correct and pretty consistent with our other "plugins", I can't help but feel that it's incredibly wasteful. Each of the XXXSignals.cpp files is less than a 100 lines long (with the licence header and all bolierplate) and it's unlikely to ever grow beyond that. And essentially, all these files do is define a single enum. The reason they are this long is because the UnixSignals class is already over-engineered (e.g. I don't see why LinuxSignals needs to be a separate class, or why it needs to repeat the descriptions/default stop values for each of the signals). Making this a plugin would just add another chunk of boilerplate on top of that.

I don't know about others, but I'd rather us move in a direction which reduces the amount of boilerplate instead of adding more of it. In my ideal world, each of these signal definitions would just be a bunch of (number, name) pairs. This doesn't have/need to be a class or a plugin; a single constexpr variable would suffice for that. Then we'd just cross-reference this mapping with another one which gives the default stop values and descriptions for each of the signals, and that's it.

I know I am repeating myself, but each time I say this, it's because I find another reason for it: I think we should start a new library which I would roughly define as "utilities for describing and manipulating various low-level aspects of processes, but which is agnostic of any actual process class". The idea would be that we can shove here all classes which are shared between lldb-server liblldb. UnixSignals would be one candidate for it. AuxVector, MemoryRegionInfo are others. Plugins/Process/Utility (where most of the signal classes live) would be a pretty good place for it already, were it not for the "Plugins" part (it would be weird for non-plugin code to depend on something called a "plugin"). However, maybe we could just create a new top-level library called "ProcessUtil" (or whatever name we come up with) and move the relevant stuff there.

Anyway, TL;DR: I think this should be handled differently. However, if others are fine with this approach, then feel free to ignore me.

How would you bind a particular variant of UnixSignals to the process plugin that it goes along with in this scenario?

The same way as it is done now -- you pass in an ArchSpec, and get back a list of signals in return. The only difference would be that the implementation of "finding the right set of signals for a given ArchSpec" wouldn't be distributed over 5 subfolders, but it would happen in a single place. (I think that this code is so simple that we should shove all of it into a single file -- that would make it easier to share common stuff like signal descriptions between implementations)