Page MenuHomePhabricator

rL267291: Architecture change to thumb on parsing arm.attributes causes regression.

Authored by omjavaid on Apr 25 2016, 6:50 PM.



rL267291 introduces a lot of regression on arm-linux by fixing module architecture to thumb if it finds thumb32 tag set.

Tag_THUMB_ISA_use, (=9), uleb128
2 32-bit Thumb instructions were permitted (implies 16-bit instructions permitted)

Does not mean that there wont be any arm instruction in current module. Therefore we can not set the architecture to thumb without knowing the value of

Tag_ARM_ISA_use, (=8), uleb128
0 The user did not permit this entity to use ARM instructions
1 The user intended that this entity could use ARM instructions

For most cases Tag_ARM_ISA_use, (=8), uleb128 will be set to 1 that will force us to use arm as our architecture instead of thumb.

I am removing this code for now may be we can come up with a better solution to get it over with.

Diff Detail


Event Timeline

omjavaid updated this revision to Diff 54961.Apr 25 2016, 6:50 PM
omjavaid retitled this revision from to rL267291: Architecture change to thumb on parsing arm.attributes causes regression..
omjavaid updated this object.
omjavaid added reviewers: tberghammer, labath.
omjavaid added a subscriber: lldb-commits.
labath resigned from this revision.Apr 26 2016, 2:00 AM
labath removed a reviewer: labath.
labath added a subscriber: compnerd.

I'll let Tamas review this.

tberghammer accepted this revision.Apr 26 2016, 4:08 AM
tberghammer edited edge metadata.

The change looks good with the explanation but there is a few thing we should do for longer terms:

  • Read the value of both Tag_ARM_ISA_use and Tag_THUMB_ISA_use and set the architecture based on the 2 value (set it to thumb only is Tag_ARM_ISA_use=0 and Tag_THUMB_ISA_use=2)
  • Fix LLDB to work when the triple is "thumb-*-*-*" as currently we have several location where we don't handle this case even if the full executable contains thumb code only (e.g. Platform::GetSoftwareBreakpointTrapOpcode)
This revision is now accepted and ready to land.Apr 26 2016, 4:08 AM
This revision was automatically updated to reflect the committed changes.