[Lldb-commits] [PATCH] D146668: [lldb-server] Use Platform plugin corresponding to the host

Mon Mar 27 10:44:55 PDT 2023

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