This is an archive of the discontinued LLVM Phabricator instance.

LLDB ARM: Add support for arm-linux-gnu abi
ClosedPublic

Authored by omjavaid on Mar 23 2015, 5:26 AM.

Details

Summary

This patch is first in a series of upcoming patches that add support for arm-linux-lldb.

This patch will be usable once once support for Register Context will be added later this week.

Diff Detail

Event Timeline

omjavaid updated this revision to Diff 22458.Mar 23 2015, 5:26 AM
omjavaid retitled this revision from to LLDB ARM: Add support for arm-linux-gnu abi .
omjavaid updated this object.
omjavaid edited the test plan for this revision. (Show Details)
omjavaid added reviewers: rengolin, vharron, tberghammer.
omjavaid added a subscriber: Unknown Object (MLST).
tberghammer edited edge metadata.Mar 23 2015, 7:08 AM

Looks good with a few minor suggestions

source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
225

Please use GetRegisterInfo if possible (at other places also)

611

I think we want to use r11 based on the comment. Please check it.

674–675

I think a default label is missing here (or something else?)

source/Plugins/ABI/SysV-arm/ABISysV_arm.h
26–41

Please add override attributes where applicable (for the entry file)

emaste added a subscriber: emaste.Mar 23 2015, 7:10 AM
omjavaid updated this revision to Diff 22545.Mar 24 2015, 1:49 AM
omjavaid edited edge metadata.

Adding full context diff.

emaste added inline comments.Mar 24 2015, 5:55 AM
lib/Makefile
42

Likely ought to be in alpha order, before _ppc
(even if _hexagon is in the wrong spot)

source/Initialization/InitializeLLDB.cpp
23 ↗(On Diff #22545)

this path looks incorrect

source/Plugins/ABI/CMakeLists.txt
3

alpha order

source/Plugins/Process/elf-core/ThreadElfCore.cpp
20 ↗(On Diff #22545)

Also //#include "Plugins/Process/Utility/RegisterContextFreeBSD_arm.h"

130 ↗(On Diff #22545)

please put in alpha order

153 ↗(On Diff #22545)

you need a case here for arm

omjavaid updated this revision to Diff 24175.Apr 21 2015, 2:58 PM

This patch update fixes all issues highlighted in review process.

Still need to figure out how to unwind dwarf frames in thumb mode. I have marked it as a todo for later.

tberghammer accepted this revision.Apr 22 2015, 3:35 AM
tberghammer edited edge metadata.

Looks good (please include the review link in the commit message)

This revision is now accepted and ready to land.Apr 22 2015, 3:35 AM
tberghammer requested changes to this revision.Apr 22 2015, 5:28 AM
tberghammer edited edge metadata.

I applied your change locally and started to use them against an android-arm device and found 2 (major) issues.

source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
186–195

This create instance will collide with the one in ABIMacOSX_arm and the plugin system will select one of them randomly (based on the plugin initialization order). I suggest to disable this plugin on OSX if OS is in [Darwin, IOS, MacOSX] and change the check in the OSX plugin to enable that one only in the listed platforms.

651

Is SP preserved or not? In the comment you say it is but then state that it is volatile what means it isn't preserved.

This revision now requires changes to proceed.Apr 22 2015, 5:28 AM
clayborg requested changes to this revision.Apr 22 2015, 10:15 AM
clayborg edited edge metadata.

Update the ABIMacOSX_arm::CreateInstance() and ABISysV_arm::CreateInstance() to check the triple's vendor so that both this plug-in and ABIMacOSX_arm can co-exist.

source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
183–195

This should be changed to also check the vendor:

arch.GetTriple().getVendor() != llvm::Triple::Apple

And the ABIMacOSX_arm::CreateInstance() should be changed to check:

arch.GetTriple().getVendor() == llvm::Triple::Apple
omjavaid updated this revision to Diff 24551.Apr 28 2015, 8:25 AM
omjavaid edited edge metadata.

Patch updated with suggested changes.

Good to commit?

labath accepted this revision.Apr 28 2015, 8:59 AM
labath edited edge metadata.

Tamas is on holiday, so I'll try to handle this for him.

Looks good after fixing the small style issue. However, please give a chance for Greg to respond.

source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
201

Please use return ABISP() for consistency with the case above.

Also, consider using early exits to simplify this function. E.g.

if (vendor_type != llvm::Triple::Apple)
  return ABISIP()
...
source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
202

Same as above...

clayborg requested changes to this revision.Apr 28 2015, 9:54 AM
clayborg edited edge metadata.

Just fix the return values of the CreateInstance to return ABISP() instead of NULL and this will be good to go.

This revision now requires changes to proceed.Apr 28 2015, 9:54 AM
omjavaid updated this revision to Diff 24582.Apr 28 2015, 4:24 PM
omjavaid edited edge metadata.

Patch updated with suggested corrections.

Good to commit?

clayborg accepted this revision.Apr 28 2015, 4:51 PM
clayborg edited edge metadata.

Looks good.

labath accepted this revision.Apr 29 2015, 1:24 AM
labath edited edge metadata.

thanks for fixing this, lgtm

tberghammer accepted this revision.May 7 2015, 5:45 AM
tberghammer edited edge metadata.
This revision is now accepted and ready to land.May 7 2015, 5:45 AM
rengolin closed this revision.May 8 2015, 5:13 AM