This is an archive of the discontinued LLVM Phabricator instance.

Fix THUMB mode discovery for LSB-set symbols
ClosedPublic

Authored by ADodds on May 27 2015, 9:31 AM.

Details

Summary

lldb connecting to lldb-server on ARM android was placing arm-breakpoints in a thumb function. This was fixed by marking a symbol as THUMB if its least significant bit is set while parsing symbols from the elf file.

Diff Detail

Repository
rL LLVM

Event Timeline

ADodds updated this revision to Diff 26606.May 27 2015, 9:31 AM
ADodds retitled this revision from to Fix THUMB mode discovery for LSB-set symbols.
ADodds updated this object.
ADodds edited the test plan for this revision. (Show Details)
ADodds added reviewers: clayborg, tberghammer.
ADodds set the repository for this revision to rL LLVM.
ADodds added a subscriber: Unknown Object (MLST).
clayborg edited edge metadata.May 27 2015, 9:58 AM

So are we missing the CPU map on android ELF files? The $m, $a and $t symbols should take care of this ISA mapping stuff unless they are not present.

tberghammer edited edge metadata.May 27 2015, 12:09 PM

So are we missing the CPU map on android ELF files? The $m, $a and $t symbols should take care of this ISA mapping stuff unless they are not present.

The ISA mapping symbols are optional and they aren't present in executables where the symtab section is striped (most system library on android).

tberghammer requested changes to this revision.May 27 2015, 12:12 PM
tberghammer edited edge metadata.

You have to add an entry to the m_address_class_map for arm symbols also because with the current change all address treated as thumb when you don't have mapping symbols.

P.S.: Next time please upload the diff with full context

This revision now requires changes to proceed.May 27 2015, 12:12 PM

You have to add an entry to the m_address_class_map for arm symbols also because with the current change all address treated as thumb when you don't have mapping symbols.

P.S.: Next time please upload the diff with full context

Perhaps I have misunderstood your concern, but the symbol is only marked as eAddressClassCodeAlternateISA if it is THUMB since that happense inside:

if (symbol_type == eSymbolTypeCode && symbol.st_value & 1)

Are you suggesting that I need to add an 'else' to this check and mark it as eAddressClassCode? I can see that would make sense.

You have to add an entry to the m_address_class_map for arm symbols also because with the current change all address treated as thumb when you don't have mapping symbols.

P.S.: Next time please upload the diff with full context

Perhaps I have misunderstood your concern, but the symbol is only marked as eAddressClassCodeAlternateISA if it is THUMB since that happense inside:

if (symbol_type == eSymbolTypeCode && symbol.st_value & 1)

Are you suggesting that I need to add an 'else' to this check and mark it as eAddressClassCode? I can see that would make sense.

Yes, you have to add an entry for arm symbols also because of the way m_address_class_map is used (see ObjectFileELF::GetAddressClass). Basically an entry means that all address have the specified type until the next entry (ordered by address). The reason behind this layout is that you can query the address class of any address without have to resolve it to a symbol it belongs to.

You have to add an entry to the m_address_class_map for arm symbols also because with the current change all address treated as thumb when you don't have mapping symbols.

P.S.: Next time please upload the diff with full context

Perhaps I have misunderstood your concern, but the symbol is only marked as eAddressClassCodeAlternateISA if it is THUMB since that happense inside:

if (symbol_type == eSymbolTypeCode && symbol.st_value & 1)

Are you suggesting that I need to add an 'else' to this check and mark it as eAddressClassCode? I can see that would make sense.

Yes, you have to add an entry for arm symbols also because of the way m_address_class_map is used (see ObjectFileELF::GetAddressClass). Basically an entry means that all address have the specified type until the next entry (ordered by address). The reason behind this layout is that you can query the address class of any address without have to resolve it to a symbol it belongs to.

Ah right! Yes that makes sense, I will make that change now.

ADodds updated this revision to Diff 26681.May 28 2015, 7:11 AM
ADodds edited edge metadata.
ADodds added a subscriber: domipheus.

This patch trys to address the issues raised by Tamas. Arm symbols will now also be added to the m_address_class_map too.

tberghammer accepted this revision.May 28 2015, 8:30 AM
tberghammer edited edge metadata.

LGTM

This revision is now accepted and ready to land.May 28 2015, 8:30 AM
This revision was automatically updated to reflect the committed changes.