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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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

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.

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...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147009