[Lldb-commits] [PATCH] D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

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.

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? 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?


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

https://reviews.llvm.org/D70840