Skip to content

X86_64: silence some truncation warnings #76

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

X86_64: silence some truncation warnings #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions Headers/DebugServer2/Architecture/X86_64/CPUState.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,13 +238,14 @@ struct CPUState64 {
gp.r14 = regs[14];
gp.r15 = regs[15];
gp.rip = regs[16];
gp.eflags = regs[17];
gp.cs = regs[18];
gp.ss = regs[19];
gp.ds = regs[20];
gp.es = regs[21];
gp.fs = regs[22];
gp.gs = regs[23];
// Truncate the value to 32-bit as these are 32-bit registers.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I'm a bit confused about the point of CPUState64 being a union. Why not simply
memcpy(gp.regs, regs, sizeof(gp.regs));
and avoid the need to remember which of these need to be truncated, etc?

gp.eflags = static_cast<uint32_t>(regs[17]);
gp.cs = static_cast<uint32_t>(regs[18]);
gp.ss = static_cast<uint32_t>(regs[19]);
gp.ds = static_cast<uint32_t>(regs[20]);
gp.es = static_cast<uint32_t>(regs[21]);
gp.fs = static_cast<uint32_t>(regs[22]);
gp.gs = static_cast<uint32_t>(regs[23]);
}

public:
Expand Down Expand Up @@ -594,7 +595,7 @@ struct CPUState {
}
inline void setPC(uint64_t pc) {
if (is32)
state32.setPC(pc);
state32.setPC(static_cast<uint32_t>(pc));
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in slack, IMO better to have a helper like

// When debugging a 32-bit inferior on a 64-bit platform registers etc frequently need truncation to 32-bits with an assumption that the high bits are zero. This helper does the cast and asserts no information is lost in the process.
static inline truncate64To32Losslessly(uint64_t data) {
  DS2ASSERT(data >> 32 == 0 && "64-bit value being truncated to 32-bits has non-zero high word");
  return static_cast<uint32_t>(data);
}

else
state64.setPC(pc);
}
Expand All @@ -604,7 +605,7 @@ struct CPUState {
}
inline void setSP(uint64_t sp) {
if (is32)
state32.setSP(sp);
state32.setSP(static_cast<uint32_t>(sp));
else
state64.setSP(sp);
}
Expand Down
2 changes: 1 addition & 1 deletion Headers/DebugServer2/Core/HardwareBreakpointManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ class HardwareBreakpointManager : public BreakpointManager {
protected:
virtual ErrorCode disableDebugCtrlReg(uint64_t &ctrlReg, int idx);
virtual ErrorCode enableDebugCtrlReg(uint64_t &ctrlReg, int idx, Mode mode,
int size);
size_t size);
#endif

public:
Expand Down
12 changes: 7 additions & 5 deletions Sources/Core/X86/HardwareBreakpointManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ ErrorCode HardwareBreakpointManager::disableLocation(int idx,

ErrorCode HardwareBreakpointManager::enableDebugCtrlReg(uint64_t &ctrlReg,
int idx, Mode mode,
int size) {
size_t size) {
int enableIdx = idx * 2;
#if !defined(OS_WIN32)
enableIdx += 1;
Expand Down Expand Up @@ -193,7 +193,7 @@ int HardwareBreakpointManager::hit(Target::Thread *thread, Site &site) {
if (debugRegs[kStatusRegIdx] & (1ull << i)) {
DS2ASSERT(_locations[i] != 0);
site = _sites.find(_locations[i])->second;
regIdx = i;
regIdx = static_cast<int>(i);
Copy link
Contributor

Choose a reason for hiding this comment

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

simpler to s/size_t/int/ at l.192?

break;
}
}
Expand Down Expand Up @@ -277,11 +277,13 @@ ErrorCode HardwareBreakpointManager::writeDebugRegisters(
state.dr.dr[i] = (i == 4 || i == 5) ? 0 : regs[i];
}
#elif defined(ARCH_X86_64)
for (int i = 0; i < kNumDebugRegisters; ++i) {
for (size_t i = 0; i < kNumDebugRegisters; ++i) {
if (state.is32) {
state.state32.dr.dr[i] = (i == 4 || i == 5) ? 0 : regs[i];
state.state32.dr.dr[i] =
(i == 4 || i == 5) ? 0 : static_cast<uint32_t>(regs[i]);
} else {
state.state64.dr.dr[i] = (i == 4 || i == 5) ? 0 : regs[i];
state.state64.dr.dr[i] =
(i == 4 || i == 5) ? 0 : static_cast<uint32_t>(regs[i]);
}
}
#else
Expand Down