[Lldb-commits] [PATCH] D63363: [Signals] Create a plugin directory just for signals
Alex Langford via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Mon Jun 17 09:54:38 PDT 2019
xiaobai added a comment.
In D63363#1545427 <https://reviews.llvm.org/D63363#1545427>, @labath wrote:
> 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.
CHANGES SINCE LAST ACTION
More information about the lldb-commits