ObjCLanguageRuntime was being pulled into LanguageRuntime because of
Breakpoint Preconditions. If we move BreakpointPrecondition out of Breakpoint,
we can extend the LanguageRuntime plugin interface so that LanguageRuntimes
can give us a BreakpointPrecondition for exceptions.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Have you considered making just AddExceptionPrecondition virtual? Wouldn't that solve the problem too, without the code duplication of making CreateExceptionBreakpoint virtual? 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.
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.
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.
Moving BreakpointPrecondition to a forward-declarable place sounds good to me.
source/Target/ObjCLanguageRuntime.cpp | ||
---|---|---|
379 ↗ | (On Diff #204212) | Shouldn't you be checking the language here or something. Given that you're iterating over all plugins, won't this set the precondition unconditionally (:P) on all exception breakpoints? |
384 ↗ | (On Diff #204212) | new doesn't fail |
Alright, so I think I'll go ahead and change this patch to make that happen. Thanks for your input! :)
source/Target/ObjCLanguageRuntime.cpp | ||
---|---|---|
379 ↗ | (On Diff #204212) | It will! For some reason I added a language paramter but forgot to use it. Thanks for pointing that out. |
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.