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

[Pavel Labath via Phabricator via lldb-commits]

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