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

Jim Ingham via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Tue Mar 21 11:19:03 PDT 2017


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





More information about the lldb-commits mailing list