labath added a comment. In D70840#1779077 <https://reviews.llvm.org/D70840#1779077>, @mstorsjo wrote:
> In D70840#1763639 <https://reviews.llvm.org/D70840#1763639>, @labath wrote: > > > - I think we ought to have some kind of a utility function to hold the > > logic for the `&~1` stuff. there is something like that in > > Architecture::GetOpcodeLoadAddress. The Architecture class is mostly > > independent of a target, so we may be able create one instance for each > > module or symbol file, but that feels quite heavy. I might even go for > > putting something like that into the ArchSpec class. The raison d'être of > > the Architecture class was to evict some heavy code out of ArchSpec -- this > > was ArchitectureArm::OverrideStopInfo. That one is definitely too heavy, so > > still don't thing it belongs in ArchSpec, but the `&~1` thingy seems like > > it could find a place there.) > > > I'm trying to dig into this now even if @clayborg hasn't commented on your > suggestions. > > This was moved out from the Target class in D48623 > <https://reviews.llvm.org/D48623>/rG7aa9e7bc5739f468143b7b97060a9fbbfb94c6d2 > <https://reviews.llvm.org/rG7aa9e7bc5739f468143b7b97060a9fbbfb94c6d2>. In > addition to GetOpcodeLoadAddress and GetCallableLoadAddress, there's also > GetBreakableLoadAddress in ArchitectureMips, which is a rather big piece of > code as well, so I guess that can't be motivated to be moved, but as you > write, the `&~1` bit in itself might be ok. Yes `GetBreakableLoadAddress` is definitely too big (and a huge hack too)... > If we add a GetOpcodeLoadAddress in ArchSpec, which does `&~1` on mips and > arm - should we try to call this from the Architecture plugins (we don't have > an ArchSpec stored there right now), or just see it as tolerable and > justifiable duplication? I think it's fine to have an ArchSpec member in the "Architecture" plugin, and then implement the forwarding in the base class. As an alternative, we could look at the existing callers of this function, and see if they have an ArchSpec around so they could call this directly. > The existing GetOpcodeLoadAddress takes an AddressClass as well, should we > keep that, or just make a plain GetOpcodeLoadAddress that assumes that the > provided address is a code address? I /think/ keeping the address class argument would be less confusing, but I am open to other ideas too... CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70840/new/ https://reviews.llvm.org/D70840 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
