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.
Details
Diff Detail
Event Timeline
Could you upload the diffs with full context please? (i.e., git diff -U999999 or svn diff --diff-cmd=diff -x -U999999)
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 |
Please see inline comment
source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp | ||
---|---|---|
216–223 | This will collide with the MacOSX version. See more details in D8539#inline-74620. |
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 ↗ | (On Diff #24553) | 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... |
Just fix the return values of the CreateInstance functions to return ABISP() instead of NULL and this is good to go.
source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp | ||
---|---|---|
616 | 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. |
alpha order