[lldb] Account for registers being host endian when casting values #150011
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
Author
|
First commit is #148836, please review the second onward. |
Member
|
@llvm/pr-subscribers-lldb Author: David Spickett (DavidSpickett) ChangesFixes #135707 Follow up to #148836 Our Value/ValueObject system does not store the endian directly This breaks down when you have register values. These are always To fix this, check the context of the value. If it has a register I had an alternative where I passed an "is_register" flag into This solution isn't much more robust, but it works for all the test For testing, I had to add a minimal program file for each arch Full diff: https://github.com/llvm/llvm-project/pull/150011.diff 3 Files Affected:
diff --git a/lldb/source/Core/Value.cpp b/lldb/source/Core/Value.cpp
index c91b3f852f986..ff37c87f5dd6a 100644
--- a/lldb/source/Core/Value.cpp
+++ b/lldb/source/Core/Value.cpp
@@ -496,7 +496,10 @@ Status Value::GetValueAsData(ExecutionContext *exe_ctx, DataExtractor &data,
if (exe_ctx) {
Target *target = exe_ctx->GetTargetPtr();
if (target) {
- data.SetByteOrder(target->GetArchitecture().GetByteOrder());
+ // Registers are always stored in host endian.
+ data.SetByteOrder(m_context_type == ContextType::RegisterInfo
+ ? endian::InlHostByteOrder()
+ : target->GetArchitecture().GetByteOrder());
data.SetAddressByteSize(target->GetArchitecture().GetAddressByteSize());
break;
}
diff --git a/lldb/source/Expression/Materializer.cpp b/lldb/source/Expression/Materializer.cpp
index 17ea1596806d0..8fc3df1360824 100644
--- a/lldb/source/Expression/Materializer.cpp
+++ b/lldb/source/Expression/Materializer.cpp
@@ -1376,29 +1376,26 @@ class EntityRegister : public Materializer::Entity {
return;
}
- DataExtractor register_data;
-
- if (!reg_value.GetData(register_data)) {
- err = Status::FromErrorStringWithFormat(
- "couldn't get the data for register %s", m_register_info.name);
- return;
- }
-
- if (register_data.GetByteSize() != m_register_info.byte_size) {
+ if (reg_value.GetByteSize() != m_register_info.byte_size) {
err = Status::FromErrorStringWithFormat(
"data for register %s had size %llu but we expected %llu",
- m_register_info.name, (unsigned long long)register_data.GetByteSize(),
+ m_register_info.name, (unsigned long long)reg_value.GetByteSize(),
(unsigned long long)m_register_info.byte_size);
return;
}
- m_register_contents = std::make_shared<DataBufferHeap>(
- register_data.GetDataStart(), register_data.GetByteSize());
+ lldb_private::DataBufferHeap buf(reg_value.GetByteSize(), 0);
+ reg_value.GetAsMemoryData(m_register_info, buf.GetBytes(),
+ buf.GetByteSize(), map.GetByteOrder(), err);
+ if (!err.Success())
+ return;
+
+ m_register_contents = std::make_shared<DataBufferHeap>(buf);
Status write_error;
- map.WriteMemory(load_addr, register_data.GetDataStart(),
- register_data.GetByteSize(), write_error);
+ map.WriteMemory(load_addr, buf.GetBytes(), reg_value.GetByteSize(),
+ write_error);
if (!write_error.Success()) {
err = Status::FromErrorStringWithFormat(
diff --git a/lldb/test/API/commands/expression/TestRegisterExpressionEndian.py b/lldb/test/API/commands/expression/TestRegisterExpressionEndian.py
new file mode 100644
index 0000000000000..d6de8731385b6
--- /dev/null
+++ b/lldb/test/API/commands/expression/TestRegisterExpressionEndian.py
@@ -0,0 +1,128 @@
+""" Check that registers written to memory for expression evaluation are
+ written using the target's endian not the host's.
+"""
+
+from enum import Enum
+from textwrap import dedent
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test.gdbclientutils import *
+from lldbsuite.test.lldbgdbclient import GDBRemoteTestBase
+
+
+class Endian(Enum):
+ BIG = 0
+ LITTLE = 1
+
+
+class Responder(MockGDBServerResponder):
+ def __init__(self, doc, endian):
+ super().__init__()
+ self.target_xml = doc
+ self.endian = endian
+
+ def qXferRead(self, obj, annex, offset, length):
+ if annex == "target.xml":
+ return self.target_xml, False
+ return (None,)
+
+ def readRegister(self, regnum):
+ return "E01"
+
+ def readRegisters(self):
+ # 64 bit pc value.
+ data = ["00", "00", "00", "00", "00", "00", "12", "34"]
+ if self.endian == Endian.LITTLE:
+ data.reverse()
+ return "".join(data)
+
+
+class TestXMLRegisterFlags(GDBRemoteTestBase):
+ def do_endian_test(self, endian):
+ architecture, pc_reg_name, yaml_file, data, machine = {
+ Endian.BIG: ("s390x", "pswa", "s390x.yaml", "ELFDATA2MSB", "EM_S390"),
+ Endian.LITTLE: (
+ "aarch64",
+ "pc",
+ "aarch64.yaml",
+ "ELFDATA2LSB",
+ "EM_AARCH64",
+ ),
+ }[endian]
+
+ self.server.responder = Responder(
+ dedent(
+ f"""\
+ <?xml version="1.0"?>
+ <target version="1.0">
+ <architecture>{architecture}</architecture>
+ <feature>
+ <reg name="{pc_reg_name}" bitsize="64"/>
+ </feature>
+ </target>"""
+ ),
+ endian,
+ )
+
+ # We need to have a program file, so that we have a full type system,
+ # so that we can do the casts later.
+ obj_path = self.getBuildArtifact("main.o")
+ yaml_path = self.getBuildArtifact(yaml_file)
+ with open(yaml_path, "w") as f:
+ f.write(
+ dedent(
+ f"""\
+ --- !ELF
+ FileHeader:
+ Class: ELFCLASS64
+ Data: {data}
+ Type: ET_REL
+ Machine: {machine}
+ ...
+ """
+ )
+ )
+ self.yaml2obj(yaml_path, obj_path)
+ target = self.dbg.CreateTarget(obj_path)
+
+ process = self.connect(target)
+ lldbutil.expect_state_changes(
+ self, self.dbg.GetListener(), process, [lldb.eStateStopped]
+ )
+
+ # If expressions convert register values into target endian, the
+ # result of register read, expr and casts should be the same.
+ pc_value = "0x0000000000001234"
+ self.expect(
+ "register read pc",
+ substrs=[pc_value],
+ )
+ self.expect("expr --format hex -- $pc", substrs=[pc_value])
+
+ pc = (
+ process.thread[0]
+ .frame[0]
+ .GetRegisters()
+ .GetValueAtIndex(0)
+ .GetChildMemberWithName("pc")
+ )
+ ull = target.FindTypes("unsigned long long").GetTypeAtIndex(0)
+ pc_ull = pc.Cast(ull)
+
+ self.assertEqual(pc.GetValue(), pc_ull.GetValue())
+ self.assertEqual(pc.GetValueAsAddress(), pc_ull.GetValueAsAddress())
+ self.assertEqual(pc.GetValueAsSigned(), pc_ull.GetValueAsSigned())
+ self.assertEqual(pc.GetValueAsUnsigned(), pc_ull.GetValueAsUnsigned())
+
+ @skipIfXmlSupportMissing
+ @skipIfRemote
+ @skipIfLLVMTargetMissing("AArch64")
+ def test_little_endian_target(self):
+ self.do_endian_test(Endian.LITTLE)
+
+ @skipIfXmlSupportMissing
+ @skipIfRemote
+ @skipIfLLVMTargetMissing("SystemZ")
+ def test_big_endian_target(self):
+ self.do_endian_test(Endian.BIG)
|
This was referenced Jul 22, 2025
Fixes llvm#135707 Follow up to llvm#148836 which fixed some of this issue but not all of it. Our Value/ValueObject system does not store the endian directly in the values. Instead it assumes that the endian of the result of a cast can be assumed to be the target's endian, or the host but only as a fallback. It assumes the place it is copying from is also that endian. This breaks down when you have register values. These are always host endian and continue to be when cast. Casting them to big endian when on a little endian host breaks certain calls like GetValueAsUnsigned (oddly, others did still work, but I couldn't figure out why). To fix this, check the context of the value. If it has a register context, always treat it as host endian and make the result host endian. I had an alternative where I passed an "is_register" flag into all calls to this, but it felt like a layering violation and changed many more lines. This solution isn't much more robust, but it works for all the test cases I know of. Perhaps you can create a register value without a RegisterInfo backing it, but I don't know of a way myself. For testing, I had to add a minimal program file for each arch so that there is a type system to support the casting. This is generated from YAML since we only need the machine and endian to be set.
DavidSpickett
force-pushed
the
lldb-s390x-cast
branch
from
680949f to
18b61cf
Compare
DavidSpickett
changed the title
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/22549 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #135707
Follow up to #148836
which fixed some of this issue but not all of it.
Our Value/ValueObject system does not store the endian directly
in the values. Instead it assumes that the endian of the result
of a cast can be assumed to be the target's endian, or the host
but only as a fallback. It assumes the place it is copying from
is also that endian.
This breaks down when you have register values. These are always
host endian and continue to be when cast. Casting them to big
endian when on a little endian host breaks certain calls like
GetValueAsUnsigned.
To fix this, check the context of the value. If it has a register
context, always treat it as host endian and make the result host
endian.
I had an alternative where I passed an "is_register" flag into
all calls to this, but it felt like a layering violation and changed
many more lines.
This solution isn't much more robust, but it works for all the test
cases I know of. Perhaps you can create a register value without
a RegisterInfo backing it, but I don't know of a way myself.
For testing, I had to add a minimal program file for each arch
so that there is a type system to support the casting. This is
generated from YAML since we only need the machine and endian
to be set.