Page MenuHomePhabricator

[LLDB] [DWARF] Strip out the thumb bit from addresses on ARM
Needs ReviewPublic

Authored by mstorsjo on Fri, Nov 29, 12:49 AM.

Details

Summary

Windows on ARM is always thumb, and contrary to ELF, there's no per-symbol flag to indicate arm vs thumb mode. Therefore, the linkers both Microsoft's link.exe and lld) sets the thumb bit in all relocations pointing to code sections.

In ELF, the thumb bit is only set for symbols marked as %function, that is, it isn't set in local labels used when constructing the dwarf debug info and line tables.

As the windows linkers sets the thumb bit in all code addresses, we'd have to cope with this and strip out the lowest bit from all code addresses if the architecture is ARM/Thumb.

There's already predecent for this in DWARFCallFrameInfo::GetFDEIndex, this extends the same concept to DWARF debug info and line tables. I'm not sure if I managed to get all spots covered here, but I tried to look for all cases of DW_AT_low_pc/DW_AT_high_pc and find the best place to handle it, where it would have to be handled in as few places as possible.

This is admittedly not very pretty, improvement suggestions are welcome.

Diff Detail

Event Timeline

mstorsjo created this revision.Fri, Nov 29, 12:49 AM
Herald added a project: Restricted Project. · View Herald Transcript

Yeah, this is going to be tricky... I don't really know what's the right way to do this, but here are the thoughts I have on this so far:

  • The new DWARFDebugInfoEntry member is going to substantially increase our memory footprint -- that's a non-starter
  • I don't like the inconsistency where all addresses are demangled in the DWARF code, except the line tables, which are in the "generic" code
  • 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.)
  • is this behavior reproducible with lld ? If so, I think it'd make the test more readable to run llvm-mc+lld as a part of the test
  • the address byte size would best be handled separately

Yeah, this is going to be tricky... I don't really know what's the right way to do this, but here are the thoughts I have on this so far:

  • The new DWARFDebugInfoEntry member is going to substantially increase our memory footprint -- that's a non-starter

Ok, I feared that class was one where the size is critical yeah...

  • I don't like the inconsistency where all addresses are demangled in the DWARF code, except the line tables, which are in the "generic" code

Yup. The reason for doing it like this was as I tried to find the most narrow interface where I could catch all of them.

  • 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.)
  • is this behavior reproducible with lld ? If so, I think it'd make the test more readable to run llvm-mc+lld as a part of the test

Yes it is. Ok, that's fine with me.

  • the address byte size would best be handled separately

On second thought, yes, this should be trivial to split out and make a similar test with inspecting line tables of an i386 binary.

mstorsjo updated this revision to Diff 231517.Fri, Nov 29, 4:12 AM
mstorsjo edited the summary of this revision. (Show Details)

Converted the test to .s form, using lld, split out the ObjectFile changes to D70848 (which this change now depends on for the tests).

Yeah, this is going to be tricky... I don't really know what's the right way to do this, but here are the thoughts I have on this so far:

  • The new DWARFDebugInfoEntry member is going to substantially increase our memory footprint -- that's a non-starter

Ok, I feared that class was one where the size is critical yeah...

  • I don't like the inconsistency where all addresses are demangled in the DWARF code, except the line tables, which are in the "generic" code

Yup. The reason for doing it like this was as I tried to find the most narrow interface where I could catch all of them.

Is there any corresponding place in generic code where one could do the same operation on the addresses that comes from pc_lo/pc_hi ranges from .debug_info and .debug_aranges (not familiar with this section and when it's generated etc), if that would end up touching fewer places? The existing predecent in DWARFCallFrameInfo::GetFDEIndex is dwarf specific, but in generic code outside of the dwarf symbolfile plugin.

labath added a subscriber: dblaikie.

Throwing some additional dwarf people in here...

Yeah, this is going to be tricky... I don't really know what's the right way to do this, but here are the thoughts I have on this so far:

  • The new DWARFDebugInfoEntry member is going to substantially increase our memory footprint -- that's a non-starter

Ok, I feared that class was one where the size is critical yeah...

  • I don't like the inconsistency where all addresses are demangled in the DWARF code, except the line tables, which are in the "generic" code

Yup. The reason for doing it like this was as I tried to find the most narrow interface where I could catch all of them.

Is there any corresponding place in generic code where one could do the same operation on the addresses that comes from pc_lo/pc_hi ranges from .debug_info and .debug_aranges (not familiar with this section and when it's generated etc), if that would end up touching fewer places? The existing predecent in DWARFCallFrameInfo::GetFDEIndex is dwarf specific, but in generic code outside of the dwarf symbolfile plugin.

debug_aranges is a sort of a lookup table for speeding up address->compile unit searches. llvm does not generate it by default since, and I think the reason is that you can usually get the same kind of data from the DW_AT_ranges attribute of the compile unit. I don't think you would be able to handle that in generic code, as debug_aranges does not make it's way into generic code.

Overall, I think this needs to be handled in DWARF code. That may even make sense, since PDB may not suffer from the same problem (does it?)

TBH, the m_addr_mask member issue is not that hard to resolve -- all DWARFDebugInfoEntry functions get a DWARFUnit argument (otherwise they wouldn't be able to do anything). Theoretically, we could store the mask there.

However, the question on my mind is what does that say about the llvm<->lldb dwarf parser convergence (one of our long term goals is to delete the lldb dwarf parser altogether, leaving just the high level lldb-specific stuff). Adding mask handling code low into the dwarf parser would go against that goal, as there is no equivalent llvm functionality.

One possible solution would be to try to add equivalent llvm functionality -- it sounds like something like this is needed there too, as all llvm tools (I don't know if there's anything besides llvm-symbolizer) will probably not work without it. (If they do, it would be interesting to figure out how they manage that.)

Another possibility might be to implement this in the higher levels of the dwarf code (the parts that are likely to stay in lldb forever)...

dblaikie added a subscriber: rnk.Mon, Dec 2, 7:41 AM

Throwing some additional dwarf people in here...

Yeah, this is going to be tricky... I don't really know what's the right way to do this, but here are the thoughts I have on this so far:

  • The new DWARFDebugInfoEntry member is going to substantially increase our memory footprint -- that's a non-starter

Ok, I feared that class was one where the size is critical yeah...

  • I don't like the inconsistency where all addresses are demangled in the DWARF code, except the line tables, which are in the "generic" code

Yup. The reason for doing it like this was as I tried to find the most narrow interface where I could catch all of them.

Is there any corresponding place in generic code where one could do the same operation on the addresses that comes from pc_lo/pc_hi ranges from .debug_info and .debug_aranges (not familiar with this section and when it's generated etc), if that would end up touching fewer places? The existing predecent in DWARFCallFrameInfo::GetFDEIndex is dwarf specific, but in generic code outside of the dwarf symbolfile plugin.

debug_aranges is a sort of a lookup table for speeding up address->compile unit searches. llvm does not generate it by default since, and I think the reason is that you can usually get the same kind of data from the DW_AT_ranges attribute of the compile unit. I don't think you would be able to handle that in generic code, as debug_aranges does not make it's way into generic code.

Overall, I think this needs to be handled in DWARF code. That may even make sense, since PDB may not suffer from the same problem (does it?)

TBH, the m_addr_mask member issue is not that hard to resolve -- all DWARFDebugInfoEntry functions get a DWARFUnit argument (otherwise they wouldn't be able to do anything). Theoretically, we could store the mask there.

However, the question on my mind is what does that say about the llvm<->lldb dwarf parser convergence (one of our long term goals is to delete the lldb dwarf parser altogether, leaving just the high level lldb-specific stuff). Adding mask handling code low into the dwarf parser would go against that goal, as there is no equivalent llvm functionality.

One possible solution would be to try to add equivalent llvm functionality -- it sounds like something like this is needed there too, as all llvm tools (I don't know if there's anything besides llvm-symbolizer) will probably not work without it. (If they do, it would be interesting to figure out how they manage that.)

Yeah, I don't know much about the ARM tagged pointer stuff, but if the tags don't appear in the return addresses in a stack trace & thus the addresses you'd pass to llvm-symbolizer then I think it'd make sense to implement it somewhere in LLVM's libDebugInfoDWARF (& yeah, don't know about PDB either, perhaps @rnk does). If there is no common code between debug_aranges and other address parsing -perhaps there should be? a filter of some kind that could be used for all addresses being read?

So some background on how address masks are handled in LLDB:

Typically the way we have tried to take care of the extra Thumb bit for ARM using:

lldb::addr_t Address::GetCallableLoadAddress(Target *target, bool is_indirect = false) const;
lldb::addr_t GetOpcodeLoadAddress(Target *target, AddressClass addr_class = AddressClass::eInvalid) const;

The first will add the extra bit to an address if needed. The latter will strip the bit if needed. This does require a target though and the target uses the "Architecture" class for ARM to do the work of using the mask. Not sure if we want to try to get an architecture class and use that here for stripping the bit instead of using an address mask?

Also, any lldb_private::Address can be asked for its address class:

AddressClass Address::GetAddressClass() const;

This will return "eCode" for ARM code and "eCodeAlternateISA" for Thumb code. This is resolved by the ObjectFile::GetAddressClass:

/// Get the address type given a file address in an object file.
///
/// Many binary file formats know what kinds This is primarily for ARM
/// binaries, though it can be applied to any executable file format that
/// supports different opcode types within the same binary. ARM binaries
/// support having both ARM and Thumb within the same executable container.
/// We need to be able to get \return
///     The size of an address in bytes for the currently selected
///     architecture (and object for archives). Returns zero if no
///     architecture or object has been selected.
virtual AddressClass GetAddressClass(lldb::addr_t file_addr);

So currently no code in LLDB tries to undo the address masks that might be in the object file or debug info, and we take care of it after the fact. Early in the ARM days there used to be extra symbols that were added to the symbol table with names like "$a" for ARM, "$t" for Thumb and "$d" for data. There would be multiple of these symbols in an ordered vector of symbols that would create a CPU map. This was early in the ARM days before the branch instruction would watch for bit zero. Later ARM architectures started using bit zero to indicate which mode to put the processor in instead of using an explicit "branch to arm" or "branch to thumb" instructions. When this new stuff came out bit zero started showing up in symbol tables. So the current code allows for either the old style (CPU map in symbol table with $a $t and $d symbols) and the new style (bit zero set and no CPU map).

lldb/include/lldb/Symbol/LineTable.h
335

Might be nice to let the line table parse itself first, and then in a post production step clean up all the addresses? Maybe

void LineTable::Finalize(Architecture *arch);

Then we let the architecture plug-in handle any stripping using:

lldb::addr_t Architecture::GetOpcodeLoadAddress(lldb::addr_t addr, AddressClass addr_class) const;

The ArchitectureArm plugin does this:

addr_t ArchitectureArm::GetOpcodeLoadAddress(addr_t opcode_addr,
                                             AddressClass addr_class) const {
  switch (addr_class) {
  case AddressClass::eData:
  case AddressClass::eDebug:
    return LLDB_INVALID_ADDRESS;
  default: break;
  }
  return opcode_addr & ~(1ull);
}
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAranges.cpp
19–20

Use Architecture plug-in instead of hard coded mask.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAranges.h
56

See comment in LineTable.h above.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
41–42

Use Architecture plug-in instead of hard coded mask.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
259–260

Use Architecture plug-in instead of hard coded mask.

275–276

ditto...

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
44–45 ↗(On Diff #231517)

remove

190 ↗(On Diff #231517)

Use Architecture plug-in instead of hard coded mask.

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
62

remove

158

remove

771

post produce with Architecture plug-in as mentioned in LineTable.h?

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
4006–4014

remove

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
320

Use Architecture plug-in instead of hard coded mask.

lldb/source/Symbol/LineTable.cpp
23–27

Use Architecture plug-in instead of hard coded mask or post produce using Architecture plug-in.

33–34

Use Architecture plug-in instead of hard coded mask or post produce using Architecture plug-in.

So the main goal here is to let the Architecture plug-in handle the address masking in all places and make sure there is no code like:

if (ArchSpec arch = m_objfile_sp->GetArchitecture()) {
  if (arch.GetTriple().getArch() == llvm::Triple::arm ||
      arch.GetTriple().getArch() == llvm::Triple::thumb)
    return ~1ull;
}

anywhere in the codebase. Otherwise when we add a new architecture that requires bit stripping/adding, we end up having to change many locations in the code (2 of which were added with this patch). So use the Architecture plug-in to do this and we just need to edit the Architecture plug-in code for each architecture.

amccarth resigned from this revision.Mon, Dec 2, 11:36 AM

I'm following along, but I don't think I have enough domain knowledge to qualify as a reviewer.

mstorsjo marked an inline comment as done.Mon, Dec 2, 12:48 PM

debug_aranges is a sort of a lookup table for speeding up address->compile unit searches. llvm does not generate it by default since, and I think the reason is that you can usually get the same kind of data from the DW_AT_ranges attribute of the compile unit.

Ok, thanks for the explanation!

On the topic of debug_aranges (but unrelated to this thread), I've been notified about a project, drmingw, a postmortem debugger for the mingw environment (I think), which stumbles on code built by llvm exactly due to the lack of debug_aranges: https://github.com/jrfonseca/drmingw/issues/42#issuecomment-526850588 They speculate that it almost even would be mandated by the dwarf spec.

Overall, I think this needs to be handled in DWARF code. That may even make sense, since PDB may not suffer from the same problem (does it?)

Not sure actually, I'd have to check. (FWIW, llvm doesn't support generating codeview debug info for PDB for ARM, only x86 and aarch64, but I could check with MSVC tools.)

lldb/include/lldb/Symbol/LineTable.h
335

Thanks for all the pointers and the explanations! I'll try to have a look into all of this in a few days.

I don't want to add noise to this, but there's a related change that I'd like to upstream soon -- ARMv8.3 pointer authentication bits in addresses. With the ARMv8.3 ISA, higher bits that aren't used for addressing can be used to sign & authenticate pointers that are saved in memory, where they may be overwritten via buffer over-runs et al, to verify that they have not been modified before they are used. I hope to put together a patchset for upstreaming the changes I made to lldb to support this soon.

This is similar to the 0th bit set for the lr on Aarch32 code, to indicate that the caller function is thumb; when lldb reads the lr value, it has to strip the 0th bit before it does a symbol lookup to find the caller. With PAC bits, lldb needs to do the same -- clear or set the top bits of the address before symbol lookup.

However, the PAC bits give us two complications that the thumb bit does not have: First, the number of bits used for ptrauth are variable - depending on how many page tables the operating system is using for processes, we need to strip more/fewer bits. So we need access to a Process object to determine this at runtime. Second, people may need to debug issues with their PAC bits, so we want to print the real unadulterated pointer value, and if it resolves to a symbol address, we print the stripped value and the symbol. For instance, where we might show the lr value like this for register read:

lr = 0x00000001bfe03b48  libdispatch.dylib`dispatch_mig_server + 272

once authentication bits have been added, we'll show:

lr = 0x4051f781bfe03b48 (0x00000001bfe03b48) libdispatch.dylib`dispatch_mig_server + 272

I'm not sure we should try to handle both the thumb bit and pac bits in the same mechanism, but it's close enough that I wanted to bring it up.

I currently have my method in the ABI (my method name is poorly chosen, but I'd go with StripAuthenticationBits right now if I were putting it in the ABI) because I have a weak pointer to the Process from there.

I also added a SBValue::GetValueAsAddress API similar to GetValueAsUnsigned, which runs the value through the strip method. At the API level, script writers may want to show the pointer value with PAC bits and/or the actual pointer value, depending on what they are doing.

I have calls to this ABI method scattered across lldb, although unlike the thumb bit, it won't show up in the DWARF at all.

I haven't thought about moving this StripAuthenticationBits method into Architecture - the callers would all need to provide a Process*, but that's how everyone gets an ABI already, so it might be straightforward to do that.

debug_aranges is a sort of a lookup table for speeding up address->compile unit searches. llvm does not generate it by default since, and I think the reason is that you can usually get the same kind of data from the DW_AT_ranges attribute of the compile unit.

Ok, thanks for the explanation!

On the topic of debug_aranges (but unrelated to this thread), I've been notified about a project, drmingw, a postmortem debugger for the mingw environment (I think), which stumbles on code built by llvm exactly due to the lack of debug_aranges: https://github.com/jrfonseca/drmingw/issues/42#issuecomment-526850588 They speculate that it almost even would be mandated by the dwarf spec.

Yeah, I've had some conversations with folks on a couple of different things that have been curious about Clang/LLVM's lack of debug_aranges - my general take on it (as one of the people involved in turning off debug_aranges by default in LLVM's output - it can still be enabled by -gdwarf-aranges) is that they add a significant amount of object size for little benefit compared to the ranges on CU DIEs themselves. Clients who read input without aranges for a given CU should look in the CU DIE to see if there are ranges there - it should add very little overhead to consumers & save some significant debug info size (especially for split-dwarf in object sizes (where the relocations still exist & take up a fair bit of space - especially with compression enabled, where relocations are not compressed)).

So some background on how address masks are handled in LLDB:

Typically the way we have tried to take care of the extra Thumb bit for ARM using:

lldb::addr_t Address::GetCallableLoadAddress(Target *target, bool is_indirect = false) const;
lldb::addr_t GetOpcodeLoadAddress(Target *target, AddressClass addr_class = AddressClass::eInvalid) const;

The first will add the extra bit to an address if needed. The latter will strip the bit if needed. This does require a target though and the target uses the "Architecture" class for ARM to do the work of using the mask. Not sure if we want to try to get an architecture class and use that here for stripping the bit instead of using an address mask?

So, where do I get hold of an Architecture* object instance from within the relevant contexts (within SymbolFileDWARF, where we have a reference to the object file)? Also within source/Symbol/DWARFCallFrameInfo.cpp where we have existing code that does the same.

labath added a comment.Tue, Dec 3, 6:47 AM

So some background on how address masks are handled in LLDB:

Typically the way we have tried to take care of the extra Thumb bit for ARM using:

lldb::addr_t Address::GetCallableLoadAddress(Target *target, bool is_indirect = false) const;
lldb::addr_t GetOpcodeLoadAddress(Target *target, AddressClass addr_class = AddressClass::eInvalid) const;

The first will add the extra bit to an address if needed. The latter will strip the bit if needed. This does require a target though and the target uses the "Architecture" class for ARM to do the work of using the mask. Not sure if we want to try to get an architecture class and use that here for stripping the bit instead of using an address mask?

So, where do I get hold of an Architecture* object instance from within the relevant contexts (within SymbolFileDWARF, where we have a reference to the object file)? Also within source/Symbol/DWARFCallFrameInfo.cpp where we have existing code that does the same.

Well... that's the tricky part (which I've tried to allude to in D70840#1763639. Currently the only thing holding an architecture object is the target class, but since one of the goals of the SymbolFile stuff is to be independent of any particular target, you cannot easily get hold of that. That is why I tried to steer this towards having this in the ArchSpec class. If we don't want that, we'll probably have to create one new Architecture instance local to each Module object...

So some background on how address masks are handled in LLDB:

Typically the way we have tried to take care of the extra Thumb bit for ARM using:

lldb::addr_t Address::GetCallableLoadAddress(Target *target, bool is_indirect = false) const;
lldb::addr_t GetOpcodeLoadAddress(Target *target, AddressClass addr_class = AddressClass::eInvalid) const;

The first will add the extra bit to an address if needed. The latter will strip the bit if needed. This does require a target though and the target uses the "Architecture" class for ARM to do the work of using the mask. Not sure if we want to try to get an architecture class and use that here for stripping the bit instead of using an address mask?

So, where do I get hold of an Architecture* object instance from within the relevant contexts (within SymbolFileDWARF, where we have a reference to the object file)? Also within source/Symbol/DWARFCallFrameInfo.cpp where we have existing code that does the same.

Well... that's the tricky part (which I've tried to allude to in D70840#1763639. Currently the only thing holding an architecture object is the target class, but since one of the goals of the SymbolFile stuff is to be independent of any particular target, you cannot easily get hold of that. That is why I tried to steer this towards having this in the ArchSpec class. If we don't want that, we'll probably have to create one new Architecture instance local to each Module object...

Ah, sorry for overlooking those parts of your comment. @clayborg - what do you think of @labath's suggestions here?

So some background on how address masks are handled in LLDB:

Typically the way we have tried to take care of the extra Thumb bit for ARM using:

lldb::addr_t Address::GetCallableLoadAddress(Target *target, bool is_indirect = false) const;
lldb::addr_t GetOpcodeLoadAddress(Target *target, AddressClass addr_class = AddressClass::eInvalid) const;

The first will add the extra bit to an address if needed. The latter will strip the bit if needed. This does require a target though and the target uses the "Architecture" class for ARM to do the work of using the mask. Not sure if we want to try to get an architecture class and use that here for stripping the bit instead of using an address mask?

So, where do I get hold of an Architecture* object instance from within the relevant contexts (within SymbolFileDWARF, where we have a reference to the object file)? Also within source/Symbol/DWARFCallFrameInfo.cpp where we have existing code that does the same.

Well... that's the tricky part (which I've tried to allude to in D70840#1763639. Currently the only thing holding an architecture object is the target class, but since one of the goals of the SymbolFile stuff is to be independent of any particular target, you cannot easily get hold of that. That is why I tried to steer this towards having this in the ArchSpec class. If we don't want that, we'll probably have to create one new Architecture instance local to each Module object...

Ah, sorry for overlooking those parts of your comment. @clayborg - what do you think of @labath's suggestions here?

Ping @clayborg - can you comment on @labath's suggestions?

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

  • 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/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...

mstorsjo updated this revision to Diff 233586.Thu, Dec 12, 5:37 AM
mstorsjo edited the summary of this revision. (Show Details)

Added ArchSpec::GetOpcodeLoadAddress() (didn't refactor Architecture to use it yet, so this is still duplicated code with the ArchitectureArm and ArchitectureMips plugins, but posting for feedback).

Changed the existing DWARFCallFrameInfo to use it for clearing the lowest bit, and added a Finalize pass to both LineTable and DWARFDebugAranges for fixing it up.

DWARFDebugInfoEntry still has the calls scattered around, as there's many different call paths, and once parsed, the data ends up in many different places.