bulbazord added a comment. In D146668#4223456 <https://reviews.llvm.org/D146668#4223456>, @labath wrote:
> I'm sorry, but I don't think this is a good change (which I guess means the > previous one is not good either, but this one drives the point). I really > don't want lldb-server to depend on a platform plugins . I have two reasons > for that: > > - lldb-server is always running on the "host" platform, so the abstraction is > largely useless > - the platform plugins end up (transitively) depending on the whole world -- > which includes tons of functionality that lldb-server does not need. With > these changes the size of the lldb-server binary has increased by over 50% in > my build (release+asserts on linux) -- from 40MB (which is already quite big > for what lldb-server does) to 62MB (which is just ludicrous). > > Of these, the second reason is the main one. If the platform plugin had a > simple self-contained API, then using it in lldb-server would not be a > problem. It might even be nice. However, we're very far from that state, and > I don't think it makes sense to try to achieve it. We use it as a repository > for os-specific logic in lldb, but here "lldb" really means the lldb client. > lldb-server doesn't need 90% of the Platform functionality, and most of the > other 10% (e.g. the file API) can be easily replaced by using the host > equivalents directly. Before this patch set, unix signals belonged in those > other 10 percent. > > I think we need to revert these patches and figure out a different way to > solve the issue at hand. The 50% increase is just too high. To be clear, I > totally understand and support what you're trying to achieve. Having no > outgoing plugin dependencies from the lldb core is a worthwhile endeavour, > and in fact could help shrink lldb-server size as well (because it wants to > depend on some -- and only some -- parts of lldb core). However, that won't > be the case if we solve that by reattaching all of the dependencies to > lldb-server. > > From the lldb client perspective, there's nothing wrong with having a > Platform::GetUnixSignals api. It fits in very well into the existing > functionality. However, since this is also used by lldb-server, we should > also have a non-platform way to access that data. One way to solve that would > be to have some sort of a "platform lite" plugin, which would contain the > signal API, and any other functionality that is useful for both lldb and > lldb-server -- and the real platform plugin could subsume that. > > However, I find that a bit overkill. The unix signals classes are very simple > (and very repetitive), so it seems to me like the plugin machinery would just > add a bunch of boilerplate (and institutionalize the repetition). I could be > convinced otherwise (particularly, if we find some other platform-like > functionality that could be placed into this plugin), but right now, I think > it would be better to just de-pluginize the entire UnixSignals machinery. For > example, we could just move it down to Utility so it can be accessed from > anywhere. Later, with some refactoring, I think we could even merge the > various ***Signals files into one, since all they should really differ is in > the values of signal constants. Like, we probably would not want to have a > different signal disposition settings for SIGINT on linux than say on > FreeBSD, so the fact that these are repeated in every implementation is just > a potential source of bugs. > > And we can still have Platform::GetUnixSignals to wrap this, and call it from > whereever it makes sense. I'm alright with reverting these patches if it balloons lldb-server's size by 50%. I really appreciate you trying to work with me to find a path forward here. I really do not want to revert without some idea of how we could solve this dependency/layering issue. I like the idea of a "platform lite" plugin, but I also agree that it is probably overkill right now. Let's keep that idea in our back pockets. Sometime today I will revert this patch (I'll re-commit the test in a follow-up as it is still useful) and its predecessor (D146263 <https://reviews.llvm.org/D146263>). I'll also do some investigation work and write something up on the LLVM discourse where we can have a discussion about the best way forward. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146668/new/ https://reviews.llvm.org/D146668 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
