DavidSpickett created this revision.
Herald added subscribers: ctetreau, kristof.beyls.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
While working in support for SME's ZA register, I found a circumstance
where restoring ZA after SVE, when the current SVE mode is non-streaming,
will kick the process back into FPSIMD mode. Meaning the SVE values that
you just wrote are now cut off at 128 bit.
The fix for that, I think, is to write ZA then SVE. Problem with that
is, the current ReadAll/WriteAll makes a lot of assumptions about the
saved data length.
This patch changes the format so there is a "kind" written before
each data block. This tells WriteAllRegisterValues what it's looking at
without brittle checks on length, or assumptions about ordering.
If we want to change the order of restoration, all we now have to
do is change the order of saving.
This exposes a bug where the TLS registers are not restored.
This will be fixed by https://reviews.llvm.org/D156512 in some form,
depending on what lands first.
Existing SVE tests certainly check restoration and when I got this
wrong, many, many tests failed. So I think we have enough coverage
already, and more will be coming with future ZA changes.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D156687
Files:
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
===================================================================
--- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
+++ lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
@@ -499,6 +499,30 @@
return Status("Failed to write register value");
}
+enum SavedRegistersKind : uint32_t {
+ GPR,
+ SVE, // Used for SVE and SSVE.
+ FPR, // When there is no SVE, or SVE in FPSIMD mode.
+ MTE,
+ TLS,
+};
+
+static uint8_t *AddSavedRegistersKind(uint8_t *dst, SavedRegistersKind kind) {
+ *(reinterpret_cast<uint32_t *>(dst)) = kind;
+ return dst + sizeof(uint32_t);
+}
+
+static uint8_t *AddSavedRegistersData(uint8_t *dst, void *src, size_t size) {
+ ::memcpy(dst, src, size);
+ return dst + size;
+}
+
+static uint8_t *AddSavedRegisters(uint8_t *dst, enum SavedRegistersKind kind,
+ void *src, size_t size) {
+ dst = AddSavedRegistersKind(dst, kind);
+ return AddSavedRegistersData(dst, src, size);
+}
+
Status NativeRegisterContextLinux_arm64::ReadAllRegisterValues(
lldb::WritableDataBufferSP &data_sp) {
// AArch64 register data must contain GPRs and either FPR or SVE registers.
@@ -512,33 +536,33 @@
// corresponds to register sets enabled by current register context.
Status error;
- uint32_t reg_data_byte_size = GetGPRBufferSize();
+ uint32_t reg_data_byte_size = sizeof(SavedRegistersKind) + GetGPRBufferSize();
error = ReadGPR();
if (error.Fail())
return error;
// If SVE is enabled we need not copy FPR separately.
if (GetRegisterInfo().IsSVEEnabled() || GetRegisterInfo().IsSSVEEnabled()) {
- reg_data_byte_size += GetSVEBufferSize();
- // Also store the current SVE mode.
- reg_data_byte_size += sizeof(uint32_t);
+ // Store mode and register data.
+ reg_data_byte_size +=
+ sizeof(SavedRegistersKind) + sizeof(uint32_t) + GetSVEBufferSize();
error = ReadAllSVE();
} else {
- reg_data_byte_size += GetFPRSize();
+ reg_data_byte_size += sizeof(SavedRegistersKind) + GetFPRSize();
error = ReadFPR();
}
if (error.Fail())
return error;
if (GetRegisterInfo().IsMTEEnabled()) {
- reg_data_byte_size += GetMTEControlSize();
+ reg_data_byte_size += sizeof(SavedRegistersKind) + GetMTEControlSize();
error = ReadMTEControl();
if (error.Fail())
return error;
}
// tpidr is always present but tpidr2 depends on SME.
- reg_data_byte_size += GetTLSBufferSize();
+ reg_data_byte_size += sizeof(SavedRegistersKind) + GetTLSBufferSize();
error = ReadTLS();
if (error.Fail())
return error;
@@ -546,23 +570,26 @@
data_sp.reset(new DataBufferHeap(reg_data_byte_size, 0));
uint8_t *dst = data_sp->GetBytes();
- ::memcpy(dst, GetGPRBuffer(), GetGPRBufferSize());
- dst += GetGPRBufferSize();
+ dst = AddSavedRegisters(dst, SavedRegistersKind::GPR, GetGPRBuffer(),
+ GetGPRBufferSize());
if (GetRegisterInfo().IsSVEEnabled() || GetRegisterInfo().IsSSVEEnabled()) {
- *dst = static_cast<uint8_t>(m_sve_state);
- dst += sizeof(m_sve_state);
- ::memcpy(dst, GetSVEBuffer(), GetSVEBufferSize());
- dst += GetSVEBufferSize();
+ dst = AddSavedRegistersKind(dst, SavedRegistersKind::SVE);
+ *(reinterpret_cast<uint32_t *>(dst)) = static_cast<uint32_t>(m_sve_state);
+ dst += sizeof(uint32_t);
+ dst = AddSavedRegistersData(dst, GetSVEBuffer(), GetSVEBufferSize());
} else {
- ::memcpy(dst, GetFPRBuffer(), GetFPRSize());
- dst += GetFPRSize();
+ dst = AddSavedRegisters(dst, SavedRegistersKind::FPR, GetFPRBuffer(),
+ GetFPRSize());
}
- if (GetRegisterInfo().IsMTEEnabled())
- ::memcpy(dst, GetMTEControl(), GetMTEControlSize());
+ if (GetRegisterInfo().IsMTEEnabled()) {
+ dst = AddSavedRegisters(dst, SavedRegistersKind::MTE, GetMTEControl(),
+ GetMTEControlSize());
+ }
- ::memcpy(dst, GetTLSBuffer(), GetTLSBufferSize());
+ dst = AddSavedRegisters(dst, SavedRegistersKind::TLS, GetTLSBuffer(),
+ GetTLSBufferSize());
return error;
}
@@ -597,7 +624,8 @@
return error;
}
- uint64_t reg_data_min_size = GetGPRBufferSize() + GetFPRSize();
+ uint64_t reg_data_min_size =
+ GetGPRBufferSize() + GetFPRSize() + 2 * (sizeof(SavedRegistersKind));
if (data_sp->GetByteSize() < reg_data_min_size) {
error.SetErrorStringWithFormat(
"NativeRegisterContextLinux_arm64::%s data_sp contained insufficient "
@@ -606,71 +634,76 @@
return error;
}
- // Register data starts with GPRs
- ::memcpy(GetGPRBuffer(), src, GetGPRBufferSize());
- m_gpr_is_valid = true;
-
- error = WriteGPR();
- if (error.Fail())
- return error;
-
- src += GetGPRBufferSize();
+ const uint8_t *end = src + data_sp->GetByteSize();
+ while (src < end) {
+ const SavedRegistersKind kind =
+ *reinterpret_cast<const SavedRegistersKind *>(src);
+ src += sizeof(SavedRegistersKind);
- // Verify if register data may contain SVE register values.
- bool contains_sve_reg_data =
- (data_sp->GetByteSize() > (reg_data_min_size + GetSVEHeaderSize()));
+ switch (kind) {
+ case SavedRegistersKind::GPR:
+ ::memcpy(GetGPRBuffer(), src, GetGPRBufferSize());
+ m_gpr_is_valid = true;
- if (contains_sve_reg_data) {
- // Restore to the correct mode, streaming or not.
- m_sve_state = static_cast<SVEState>(*src);
- src += sizeof(m_sve_state);
+ error = WriteGPR();
+ if (error.Fail())
+ return error;
- // We have SVE register data first write SVE header.
- ::memcpy(GetSVEHeader(), src, GetSVEHeaderSize());
- if (!sve::vl_valid(m_sve_header.vl)) {
- m_sve_header_is_valid = false;
- error.SetErrorStringWithFormat("NativeRegisterContextLinux_arm64::%s "
- "Invalid SVE header in data_sp",
- __FUNCTION__);
- return error;
- }
- m_sve_header_is_valid = true;
- error = WriteSVEHeader();
- if (error.Fail())
- return error;
+ src += GetGPRBufferSize();
+
+ break;
+ case SavedRegistersKind::SVE:
+ // Restore to the correct mode, streaming or not.
+ m_sve_state = static_cast<SVEState>(*src);
+ src += sizeof(m_sve_state);
+
+ // First write SVE header.
+ ::memcpy(GetSVEHeader(), src, GetSVEHeaderSize());
+ if (!sve::vl_valid(m_sve_header.vl)) {
+ m_sve_header_is_valid = false;
+ error.SetErrorStringWithFormat("NativeRegisterContextLinux_arm64::%s "
+ "Invalid SVE header in data_sp",
+ __FUNCTION__);
+ return error;
+ }
+ m_sve_header_is_valid = true;
+ error = WriteSVEHeader();
+ if (error.Fail())
+ return error;
- // SVE header has been written configure SVE vector length if needed.
- ConfigureRegisterContext();
+ // SVE header has been written configure SVE vector length if needed.
+ ConfigureRegisterContext();
- // Make sure data_sp contains sufficient data to write all SVE registers.
- reg_data_min_size = GetGPRBufferSize() + GetSVEBufferSize();
- if (data_sp->GetByteSize() < reg_data_min_size) {
- error.SetErrorStringWithFormat(
- "NativeRegisterContextLinux_arm64::%s data_sp contained insufficient "
- "register data bytes, expected %" PRIu64 ", actual %" PRIu64,
- __FUNCTION__, reg_data_min_size, data_sp->GetByteSize());
- return error;
- }
+ ::memcpy(GetSVEBuffer(), src, GetSVEBufferSize());
+ m_sve_buffer_is_valid = true;
+ error = WriteAllSVE();
+ if (error.Fail())
+ return error;
+ src += GetSVEBufferSize();
- ::memcpy(GetSVEBuffer(), src, GetSVEBufferSize());
- m_sve_buffer_is_valid = true;
- error = WriteAllSVE();
- src += GetSVEBufferSize();
- } else {
- ::memcpy(GetFPRBuffer(), src, GetFPRSize());
- m_fpu_is_valid = true;
- error = WriteFPR();
- src += GetFPRSize();
- }
+ break;
+ case SavedRegistersKind::FPR:
+ m_fpu_is_valid = true;
+ error = WriteFPR();
+ if (error.Fail())
+ return error;
+ src += GetFPRSize();
- if (error.Fail())
- return error;
+ break;
+ case SavedRegistersKind::MTE:
+ ::memcpy(GetMTEControl(), src, GetMTEControlSize());
+ m_mte_ctrl_is_valid = true;
+ error = WriteMTEControl();
+ if (error.Fail())
+ return error;
+ src += GetMTEControlSize();
- if (GetRegisterInfo().IsMTEEnabled() &&
- data_sp->GetByteSize() > reg_data_min_size) {
- ::memcpy(GetMTEControl(), src, GetMTEControlSize());
- m_mte_ctrl_is_valid = true;
- error = WriteMTEControl();
+ break;
+ case SavedRegistersKind::TLS:
+ // FIXME: Will be fixed by a follow up patch.
+ src += GetTLSBufferSize();
+ break;
+ }
}
return error;
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
