Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lldb][AArch64] Correct type of 32 bit GPR RegisterValues when using core files #70054

Merged
merged 1 commit into from Oct 25, 2023

Conversation

DavidSpickett
Copy link
Collaborator

As ReadRegister always read into a uint64_t, when it called operator= with uint64_t it was setting the RegisterValue's type to eTypeUInt64 regardless of its size.

This mostly works because most registers are 64 bit, and very few bits of code rely on the type being correct. However, cpsr, fpsr and fpcr are in fact 32 bit, and my upcoming register fields code relies on this type being correct.

Which is how I found this bug and unfortunately is the only way to test it. As RegisterValue::Type never makes it out via the API anywhere. So this change will be tested once I start adding register field information.

…core files

As ReadRegister always read into a uint64_t, when it called operator= with uint64_t
it was setting the RegisterValue's type to eTypeUInt64 regardless of its size.

This mostly works because most registers are 64 bit, and very few bits of code
rely on the type being correct. However, cpsr, fpsr and fpcr are in fact 32 bit,
and my upcoming register fields code relies on this type being correct.

Which is how I found this bug and unfortunately is the only way to test it.
As RegisterValue::Type never makes it out via the API anywhere. So this
change will be tested once I start adding register field information.
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 24, 2023

@llvm/pr-subscribers-lldb

Author: David Spickett (DavidSpickett)

Changes

As ReadRegister always read into a uint64_t, when it called operator= with uint64_t it was setting the RegisterValue's type to eTypeUInt64 regardless of its size.

This mostly works because most registers are 64 bit, and very few bits of code rely on the type being correct. However, cpsr, fpsr and fpcr are in fact 32 bit, and my upcoming register fields code relies on this type being correct.

Which is how I found this bug and unfortunately is the only way to test it. As RegisterValue::Type never makes it out via the API anywhere. So this change will be tested once I start adding register field information.


Full diff: https://github.com/llvm/llvm-project/pull/70054.diff

1 Files Affected:

  • (modified) lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp (+3-5)
diff --git a/lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp b/lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
index 6f6c6d073939a61..f3e763b0475ceb1 100644
--- a/lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
+++ b/lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
@@ -184,11 +184,9 @@ bool RegisterContextCorePOSIX_arm64::ReadRegister(const RegisterInfo *reg_info,
 
   offset = reg_info->byte_offset;
   if (offset + reg_info->byte_size <= GetGPRSize()) {
-    uint64_t v = m_gpr_data.GetMaxU64(&offset, reg_info->byte_size);
-    if (offset == reg_info->byte_offset + reg_info->byte_size) {
-      value = v;
-      return true;
-    }
+    value.SetFromMemoryData(*reg_info, m_gpr_data.GetDataStart() + offset,
+                            reg_info->byte_size, lldb::eByteOrderLittle, error);
+    return error.Success();
   }
 
   const uint32_t reg = reg_info->kinds[lldb::eRegisterKindLLDB];

Copy link
Member

@bulbazord bulbazord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this bug only really manifests with later changes, I think it's fine to fix it without a test for now. Please make sure there is a test that exercises this behavior in your follow-up.

@DavidSpickett
Copy link
Collaborator Author

Please make sure there is a test that exercises this behavior in your follow-up.

This fixes a crash when trying to use cpsr field info with a core file, so this will be covered in those tests later.

@DavidSpickett DavidSpickett merged commit a770098 into llvm:main Oct 25, 2023
4 checks passed
@DavidSpickett DavidSpickett deleted the lldb-cpsr-core branch October 25, 2023 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants