[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

None of this isn't modeling the situation particularly clearly, since we have "architecture specific modifications to general behaviors" that aren't coming in through plugins so that it would be easy to modify the behavior for new architectures. Instead, we're requiring new architectures to go in and edit supposedly generic code.  We put off that abstraction because it was unclear what we would need it to do, and because of that for the most part we just put these architecture specific behaviors inline in various places.  Then this one callback had nowhere to go but ArchSpec which is the only logical place for architecture specific code to accumulate.   It doesn't seem like we should gate cleaning up ArchSpec on that larger issue, however.

Rather than introducing another file, it seems simpler to make this part of StopInfo, since it is logically modifying the StopInfo for a thread.  It isn't right there, but it's any worse, and StopInfo is closer to the machine (though so far mostly closer to the Platform) than Process should be.  And StopInfo's already have to know about pretty much all the details of Process/Thread/etc...  so you wouldn't be adding knowledge to them that isn't appropriate.

I'd also suggest removing the "callback" name from it since it really isn't a general purpose callback, it is absolutely determined by the ArchSpec.  For instance if you were to use anything but the ARM one for ARM debugging you would break debugging.  It's not optional...  Instead I'd call it something like InvokeStopInfoArchOverride.  That will also make it clear that this is a target for gathering up into some "ArchSpec specific behaviors" if/when we get around to that.


https://reviews.llvm.org/D31172