[Lldb-commits] [PATCH] D147009: [lldb] Remove lldbExpression's dependency on ObjectFileJIT

[Alex Langford via Phabricator via lldb-commits]

bulbazord planned changes to this revision.
bulbazord added a comment.

In D147009#4227163 <https://reviews.llvm.org/D147009#4227163>, @labath wrote:

> I already regret getting involved in this, but here it goes...

Your effort and participation is greatly appreciated. I'm sorry if this is frustrating.

> Do you actually expect we will have multiple ObjectFile subclasses implementing the `ObjectFileCreateInstanceWithDelegate` interface? Because if not, then I think this is just a very roundabout way of being able to satisfy the syntactic requirements for calling ObjectFileJIT a plugin.

That's exactly what this is. If we had "ScriptedObjectFiles" or something similar to this (like with what @mib is doing with ScriptedProcess) then this abstraction might make more sense but I'm not sure if that will be a thing.

> However, semantically, I don't think ObjectFileJIT is really a plugin, because a fundamental property of a plug**in** is that one can plug it **out** (and still have the core functionality working). If I compiled LLDB without ObjectFileELF, then it would work just fine (sans the capability to open ELF files). If I plug ObjectFileJIT out, then **all** expression evaluation (a core functionality) becomes broken, regardless of which target I'm debugging (or anything else).
>
> I think that LLDB (falsely) equates the concept of a "plugin" with that of a "subclass". `ObjectFile` is clearly a class that was meant to be pluginized by subclassing. However, that doesn't mean that **every** subclass of `ObjectFile` needs to be a plugin.
>
> I find it very unlikely that someone is going to implement a different ObjectFile plugin that would take an ObjectFileDelegate and expose it as an ObjectFile instance. The idea just doesn't make sense. I wasn't around when this got created, but I'm pretty sure that's not what the author had in mind.
>
> I think that the situation here is reversed. ObjectFileJIT is **not** the plugin. ObjectFile**Delegate** is. And ObjectFileJIT is just an interface adaptor to present the delegate as an object file. To me, this puts it into the same category as e.g. ProcessTrace, which is a non-plugin subclass of a pluggable base, which implements a generic tracing capability on top of a trace **plugin**.
>
> Now, with all of this in mind, I'd like to propose a simpler solution to the plugin dependency problem: just drop the plugin pretense and make ObjectFileJIT a core class. We can move it to a core folder (next to the ObjectFile class -- like ProcessTrace), to satisfy the formal requirements. And, we probably don't need to do anything else...

I'm alright with this solution. We also don't need to rename "ObjectFileJITDelegate" to just "ObjectFileDelegate" so it'll be very clear what it is. I'm honestly not entirely sure why ObjectFileJIT is a plugin to begin with, but the argument you've laid out is very compelling for why it shouldn't be.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147009/new/

https://reviews.llvm.org/D147009