[Lldb-commits] [PATCH] D63181: [Target] Decouple ObjCLanguageRuntime from LanguageRuntime

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D63181#1539292 <https://reviews.llvm.org/D63181#1539292>, @xiaobai wrote:

> In D63181#1539291 <https://reviews.llvm.org/D63181#1539291>, @JDevlieghere wrote:
>
> > Have you considered making just `AddExceptionPrecondition` virtual? Wouldn't that solve the problem too, without the code duplication of making `CreateExceptionBreakpoint` virtual?
>
>
> I don't believe so. As I understand it, you might not have a process yet when you call `LanguageRuntime::CreateExceptionBreakpoint` from `Target`, so you won't have an instance of LanguageRuntime. You could create an instance of a LanguageRuntime object with a nullptr process just to invoke it as an instance method, but that seems like the wrong choice to me. All this combined has led me to believe that `AddExceptionPrecondition` needs to get a callback to a static function from PluginManager.


That's right.  The reason that this is necessary is that there were MacOS versions for a while where both the ObjC V1 and ObjC V2 runtimes were available on the same system.  And since you could have a C main executable that loaded ObjC code after it was launched, you couldn't a priori tell what ObjC runtime was going to be in force till you saw the runtime actually get loaded.  Since you could set an ObjC throw breakpoint, then run the process, we have to delay everything about choosing the actual exception breakpoint and its associated behaviors till after the process was launched.

That's also why setting exception breakpoints is a little contorted as well.

In the fullness of time we should get an lldb that could debug more than one version of swift in a single lldb, putting us back in this situation again.  So I think we should maintain this bit of complexity.

> 
> 
>> Also, I think it's totally reasonable to hoist `BreakpointPrecondition` out of `Breakpoint`. After having had to deal with the nuisance of not being able to forward declare nested classes, I try to avoid them in general.
> 
> Yeah, I was kinda iffy on this, but if you (and possibly others) think this is reasonable, I might go with this option. The only thing that would change is the callback would return a `BreakpointPreconditionSP` instead of `void`, and `AddExceptionPrecondition` would be the one to modify the breakpoint. I don't think the core idea would change much but the implementation might be slightly nicer. Let me know if you prefer that.


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

https://reviews.llvm.org/D63181