Page MenuHomePhabricator

Fix LLDB elf core dump register access for ARM/AArch64
ClosedPublic

Authored by omjavaid on Apr 9 2020, 5:25 AM.

Details

Summary

This patch adds support to access AArch64 FP SIMD core dump registers and adds a test case to verify registers.

This patches fixes a bug where doing "register read --all" causes lldb to crash.

Diff Detail

Event Timeline

omjavaid created this revision.Apr 9 2020, 5:25 AM

Thanks for doing this. The patch is pretty straight-forward. I'd just like to get some clarification about the defensive read register checks.

lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.cpp
46

Other elf core register contexts don't have this check. Why is it necessary here? Is that the register read --all bug ? If it is then isn't the proper solution to ensure we don't call this with a null register info ?

lldb/test/API/functionalities/postmortem/elf-core/aarch64-neon.c
15–57

I think it would be simpler, more obvious, and generate a smaller core file if you just used the inline assembly necessary to populate the relevant registers with necessary values (similar to the test/Shell/Register/* tests).

omjavaid updated this revision to Diff 259182.Apr 22 2020, 12:30 AM

Updated diff after implementing core file using assembly code. Also removed the redundant register info is null condition.

LGTM?

labath accepted this revision.Apr 22 2020, 1:29 AM

Thanks. LGTM, assuming the checked-in core files are of a "reasonable" (~ several tens of KB) size.

lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
51–56

I guess all of these conditions should really be offset + reg_info->byte_size <= GetGPRSize()

This revision is now accepted and ready to land.Apr 22 2020, 1:29 AM
This revision was automatically updated to reflect the committed changes.
omjavaid marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 9:35 PM