This is an archive of the discontinued LLVM Phabricator instance.

AArch64 LLDB Support for aarch64-linux-gnu abi support (SysV)
ClosedPublic

Authored by omjavaid on Mar 23 2015, 4:59 AM.

Details

Summary

This patch adds support aarch64-linux-gnu (SysV) abi in lldb. Code is similar to what has been implemented for MacOS and I have gone through abi specification this code will work without any changes though I have not seen any improvements in lldb-server remote test results with this patch applied.

Diff Detail

Event Timeline

omjavaid updated this revision to Diff 22457.Mar 23 2015, 4:59 AM
omjavaid retitled this revision from to AArch64 LLDB Support for aarch64-linux-gnu abi support (SysV).
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).
emaste added a subscriber: emaste.Mar 23 2015, 6:14 AM

Could you upload the diffs with full context please? (i.e., git diff -U999999 or svn diff --diff-cmd=diff -x -U999999)

tberghammer edited edge metadata.Mar 23 2015, 6:59 AM

I added a few inline comments but overall it looks good. I think you haven't seen any improvement in the test results because this class isn't used by any of the tests (not sure who else use this class except ThreadElfCore).

P.S.: Next time please upload the diff with full context to make it easier to review.

source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
265–266

typo: 6 or 8 arguments are handled?

354–361

Please use reg_ctx->GetRegisterInfo() if possible (e.g.: based on dwarf numbers) as it don't have to iterate over all registers in the regiters contexs

455

Please use reg_ctx->GetRegisterInfo() if possible (same for other cases)

636

Please fix the comment to match with the one in line 666 (I don't know witch one is the correct)

668–669

Please add a comment about fall through or prevent it with a return

source/Plugins/ABI/SysV-arm64/ABISysV_arm64.h
28–49

Please remove virtual and add override markers where applicable (in the entry file)

The comment says 'aarch64-linux-gnu' ABI support -- this should be "AArch64 ABI" or "AArch64 SysV ABI", no?

You need to handle aarch64 in the other case in CreateRegisterContextForFrame as well.

source/Plugins/Process/elf-core/ThreadElfCore.cpp
129–131

please put in alpha order

omjavaid updated this revision to Diff 22543.Mar 24 2015, 1:22 AM
omjavaid edited edge metadata.

Adding diff with full context.

emaste added inline comments.Mar 24 2015, 5:58 AM
source/Plugins/ABI/CMakeLists.txt
3

alpha order

source/Plugins/Process/elf-core/ThreadElfCore.cpp
146

should be before x86_64

171

aarch64 case needed here

omjavaid updated this revision to Diff 24188.Apr 21 2015, 5:13 PM

Updated patch with suggested changes.

OK to commit?

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

Looks good

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

Please see inline comment

source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
217–224

This will collide with the MacOSX version. See more details in D8539#inline-74620.

This revision now requires changes to proceed.Apr 22 2015, 5:30 AM
omjavaid updated this revision to Diff 24553.Apr 28 2015, 8:45 AM
omjavaid edited edge metadata.

Patches updated after incorporating suggested changes.

Good to commit?

labath edited edge metadata.Apr 28 2015, 9:28 AM

Hi,

I don't know if you have decided with Tamas to handle this differently, but there are still small inconsistencies in the code and comments. I know this is mostly copy-pasted from the Apple version, but we should still fix them if we know how. Do you know what is correct in these cases?

Also, please fix the CreateInstance function as in D8539.

source/Plugins/ABI/MacOSX-arm64/ABIMacOSX_arm64.cpp
236

Same issues (NULL vs ABISP(), early returns) as in the other change.

source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
232

Ditto.

341

Arguments 1-8 perhaps?

617

This comment is still inconsistent with the one on line 647. Is x30 volatile or not?

650

Is the fallthrough here intentional? Please add a comment or a break/return...

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

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

This revision now requires changes to proceed.Apr 28 2015, 9:56 AM
omjavaid updated this revision to Diff 24588.Apr 28 2015, 5:38 PM
omjavaid edited edge metadata.

Updated patch with suggested corrections.

Good to commit?

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

Thanks. You can commit after fixing the remaining inconsistency below.

source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
615

x30 specified as volatile here

646

x30 specified as non-volatile here

omjavaid added inline comments.Apr 29 2015, 3:38 AM
source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
615

Although documentation says only x19-28 + sp are callee saved but I think we ll also have to treat x30 as non-volatile as each dwarf frame has its own value of lr and previous frames lr is stored on the stack.

I ll update the comment before commit.

clayborg accepted this revision.Apr 29 2015, 9:39 AM
clayborg edited edge metadata.
tberghammer accepted this revision.May 7 2015, 5:45 AM
tberghammer edited edge metadata.

Looks good (please fix the issue mentioned by Pavel before commit)

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