Change in coreboot[master]: src/arch/x86: Fix register accesses for x86_64 debugging

[Martin Schmidt (Code Review)]

Martin Schmidt has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45032 )

Change subject: src/arch/x86: Fix register accesses for x86_64 debugging
......................................................................

src/arch/x86: Fix register accesses for x86_64 debugging

Code that interacts with GDB was accessing the eip and eflags filed in
the eregs struct.
Because those registers are rip and rflags in x86_64, compilation would fail
with x86_64 + debugging enabled.

Fix compilation by accessing the fields with a macro
which expends to the corresponding x86_64 or x86 registers.

Signed-off-by: Martin Schmidt 
Change-Id: I07183fe84d083e79ccfda19df82e87c374aed84a
---
M src/arch/x86/exception.c
M src/arch/x86/include/arch/registers.h
2 files changed, 12 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/45032/1

diff --git a/src/arch/x86/exception.c b/src/arch/x86/exception.c
index f10c7bf..07e2314 100644
--- a/src/arch/x86/exception.c
+++ b/src/arch/x86/exception.c
@@ -380,9 +380,9 @@
 #if CONFIG(GDB_STUB)
    int signo;
    memcpy(gdb_stub_registers, info, 8*sizeof(uint32_t));
-	gdb_stub_registers[PC] = info->eip;
+	gdb_stub_registers[PC] = info->X86_ARCH_IP_REGISTER;
    gdb_stub_registers[CS] = info->cs;
-	gdb_stub_registers[PS] = info->eflags;
+	gdb_stub_registers[PS] = info->X86_ARCH_FLAGS_REGISTER;
    signo = GDB_UNKNOWN;
    if (info->vector < ARRAY_SIZE(exception_to_signal))
    	signo = exception_to_signal[info->vector];
@@ -416,9 +416,9 @@
    		copy_from_hex(&gdb_stub_registers, in_buffer + 1,
    			sizeof(gdb_stub_registers));
    		memcpy(info, gdb_stub_registers, 8*sizeof(uint32_t));
-			info->eip    = gdb_stub_registers[PC];
+			info->X86_ARCH_IP_REGISTER = gdb_stub_registers[PC];
    		info->cs     = gdb_stub_registers[CS];
-			info->eflags = gdb_stub_registers[PS];
+			info->X86_ARCH_FLAGS_REGISTER = gdb_stub_registers[PS];
    		memcpy(out_buffer, "OK", 3);
    		break;
    	case 'm':
@@ -452,13 +452,13 @@
    		 */
    		ptr = &in_buffer[1];
    		if (parse_ulong(&ptr, &addr))
-				info->eip = addr;
+				info->X86_ARCH_IP_REGISTER = addr;

/* Clear the trace bit */
-			info->eflags &= ~(1 << 8);
+			info->X86_ARCH_FLAGS_REGISTER &= ~(1 << 8);
    		/* Set the trace bit if we are single stepping */
    		if (in_buffer[0] == 's')
-				info->eflags |= (1 << 8);
+				info->X86_ARCH_FLAGS_REGISTER |= (1 << 8);
    		return;
    	case 'D':
    		memcpy(out_buffer, "OK", 3);
diff --git a/src/arch/x86/include/arch/registers.h b/src/arch/x86/include/arch/registers.h
index 5f8f9be..ec3b71a 100644
--- a/src/arch/x86/include/arch/registers.h
+++ b/src/arch/x86/include/arch/registers.h
@@ -67,6 +67,8 @@
    QUAD_DOWNTO16(sp);
    uint64_t ss;
 };
+#define X86_ARCH_IP_REGISTER rip
+#define X86_ARCH_FLAGS_REGISTER rflags
 #else
 struct eregs {
    LONG_DOWNTO8(a);
@@ -83,7 +85,10 @@
    uint32_t cs;
    uint32_t eflags;
 };
+#define X86_ARCH_IP_REGISTER eip
+#define X86_ARCH_FLAGS_REGISTER eflags
 #endif
+
 #endif // !ASSEMBLER

#if CONFIG(COMPILER_LLVM_CLANG)

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45032 )

Change subject: src/arch/x86: Fix register accesses for x86_64 debugging
......................................................................

Patch Set 1:

(7 comments)

https://review.coreboot.org/c/coreboot/+/45032/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45032/1//COMMIT_MSG@9 
PS1, Line 9: filed
filed, or filled?

https://review.coreboot.org/c/coreboot/+/45032/1//COMMIT_MSG@11 
PS1, Line 11: compilation would fail
            : with x86_64 + debugging enabled.
If some combination of settings results in a build failure when it shouldn't, it means it's not being build-tested. Please add a config file under configs/ to make sure it doesn't break again.

CB:43977 added an example config to build-test several options. Watch out for the filename, CB:43611 had to rename a file because Jenkins ignored it.

https://review.coreboot.org/c/coreboot/+/45032/1//COMMIT_MSG@9 
PS1, Line 9: Code that interacts with GDB was accessing the eip and eflags filed in
           : the eregs struct.
           : Because those registers are rip and rflags in x86_64, compilation would fail
           : with x86_64 + debugging enabled.
Please wrap lines at 72 characters.

https://review.coreboot.org/c/coreboot/+/45032/1//COMMIT_MSG@14 
PS1, Line 14: compilation
Does it work as intended, though?

https://review.coreboot.org/c/coreboot/+/45032/1//COMMIT_MSG@14 
PS1, Line 14: accessing the fields with a macro
IMHO, the macros are a bit of an eyesore. How about using inline functions that take in a `struct eregs` and return the register value?

https://review.coreboot.org/c/coreboot/+/45032/1//COMMIT_MSG@15 
PS1, Line 15: expends
expands

https://review.coreboot.org/c/coreboot/+/45032/1/src/arch/x86/exception.c 
File src/arch/x86/exception.c:

https://review.coreboot.org/c/coreboot/+/45032/1/src/arch/x86/exception.c@38... 
PS1, Line 382: uint32_t
There's lots of assumptions about register sizes. Since `gdb_stub_registers` is an array of `uint32_t`, the following writes will truncate for x86_64.

[Martin Schmidt (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/45032

to look at the new patch set (#2).

Change subject: src/arch/x86: Fix register accesses for x86_64 debugging
......................................................................

src/arch/x86: Fix register accesses for x86_64 debugging

Code that interacts with GDB was accessing the eip and eflags fields in
the eregs struct.
Because those registers are rip and rflags in x86_64, compilation would
fail with x86_64 + debugging enabled.

Fix compilation by accessing the fields with a macro
which expands to the corresponding x86_64 or x86 registers.

Signed-off-by: Martin Schmidt 
Change-Id: I07183fe84d083e79ccfda19df82e87c374aed84a
---
M src/arch/x86/exception.c
M src/arch/x86/include/arch/registers.h
2 files changed, 12 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/45032/2

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45032 )

Change subject: src/arch/x86: Fix register accesses for x86_64 debugging
......................................................................

Patch Set 2:

(2 comments)

To avoid future breakages, it’d be great, if you added a build configuration in a separate commit, cf. `configs/` in the repository.

https://review.coreboot.org/c/coreboot/+/45032/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45032/2//COMMIT_MSG@11 
PS2, Line 11: Because those registers are rip and rflags in x86_64, compilation would
Please add a blank line between paragraphs.

https://review.coreboot.org/c/coreboot/+/45032/2//COMMIT_MSG@16 
PS2, Line 16: 
Please add the tag:
> TEST=

and maybe say, what you tested.

[S] Change in coreboot[master]: src/arch/x86: Fix register accesses for x86_64 debugging

[Martin L Roth (Code Review)]

Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/45032?usp=email )

Change subject: src/arch/x86: Fix register accesses for x86_64 debugging
......................................................................

Abandoned

This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.