| Message ID | 20191012235859.238387-3-morbo@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | realmode test fixes for clang | expand |
On Sat, Oct 12, 2019 at 4:59 PM Bill Wendling <morbo@google.com> wrote: > > It's fragile to try to retrieve the stack pointer by taking the address > of a variable on the stack. For instance, clang reserves more stack > space than gcc here, indicating that the variable may not be at the > start of the stack. Instead of relying upon this to work, retrieve the > "%rbp" value, which contains the value of "%rsp" before stack > allocation. > > Signed-off-by: Bill Wendling <morbo@google.com> > --- > x86/realmode.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/x86/realmode.c b/x86/realmode.c > index cf45fd6..7c89dd1 100644 > --- a/x86/realmode.c > +++ b/x86/realmode.c > @@ -518,11 +518,12 @@ extern void retf_imm(void); > > static void test_call(void) > { > - u32 esp[16]; > u32 addr; > > inregs = (struct regs){ 0 }; > - inregs.esp = (u32)esp; > + > + // At this point the original stack pointer is in %ebp. > + asm volatile ("mov %%ebp, %0" : "=rm"(inregs.esp)); I don't think we should assume the existence of frame pointers. Moreover, I think %esp is actually the value that should be saved here, regardless of how large the current frame is. > MK_INSN(call1, "mov $test_function, %eax \n\t" > "call *%eax\n\t"); > -- > 2.23.0.700.g56cf767bdb-goog >
On Wed, Oct 16, 2019 at 12:53 PM Jim Mattson <jmattson@google.com> wrote: > > On Sat, Oct 12, 2019 at 4:59 PM Bill Wendling <morbo@google.com> wrote: > > > > It's fragile to try to retrieve the stack pointer by taking the address > > of a variable on the stack. For instance, clang reserves more stack > > space than gcc here, indicating that the variable may not be at the > > start of the stack. Instead of relying upon this to work, retrieve the > > "%rbp" value, which contains the value of "%rsp" before stack > > allocation. > > > > Signed-off-by: Bill Wendling <morbo@google.com> > > --- > > x86/realmode.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/x86/realmode.c b/x86/realmode.c > > index cf45fd6..7c89dd1 100644 > > --- a/x86/realmode.c > > +++ b/x86/realmode.c > > @@ -518,11 +518,12 @@ extern void retf_imm(void); > > > > static void test_call(void) > > { > > - u32 esp[16]; > > u32 addr; > > > > inregs = (struct regs){ 0 }; > > - inregs.esp = (u32)esp; > > + > > + // At this point the original stack pointer is in %ebp. > > + asm volatile ("mov %%ebp, %0" : "=rm"(inregs.esp)); > > I don't think we should assume the existence of frame pointers. > Moreover, I think %esp is actually the value that should be saved > here, regardless of how large the current frame is. Never mind. After taking a closer look, esp[] is meant to provide stack space for the code under test, but inregs.esp should point to the top of this stack rather than the bottom. This is apparently a long-standing bug, similar to the one Avi fixed for test_long_jmp() in commit 4aa22949 ("realmode: fix esp in long jump test"). For consistency with test_long_jmp, I'd suggest changing the inregs.esp assignment to: inregs.esp = (u32)(esp+16); Note that you absolutely must preserve the esp[] array! > > MK_INSN(call1, "mov $test_function, %eax \n\t" > > "call *%eax\n\t"); > > -- > > 2.23.0.700.g56cf767bdb-goog > >
On 16/10/19 23:52, Jim Mattson wrote: > Never mind. After taking a closer look, esp[] is meant to provide > stack space for the code under test, but inregs.esp should point to > the top of this stack rather than the bottom. This is apparently a > long-standing bug, similar to the one Avi fixed for test_long_jmp() > in commit 4aa22949 ("realmode: fix esp in long jump test"). > > For consistency with test_long_jmp, I'd suggest changing the > inregs.esp assignment to: > inregs.esp = (u32)(esp+16); > > Note that you absolutely must preserve the esp[] array! > Agreed, thanks Jim for the review. Paolo
diff --git a/x86/realmode.c b/x86/realmode.c index cf45fd6..7c89dd1 100644 --- a/x86/realmode.c +++ b/x86/realmode.c @@ -518,11 +518,12 @@ extern void retf_imm(void); static void test_call(void) { - u32 esp[16]; u32 addr; inregs = (struct regs){ 0 }; - inregs.esp = (u32)esp; + + // At this point the original stack pointer is in %ebp. + asm volatile ("mov %%ebp, %0" : "=rm"(inregs.esp)); MK_INSN(call1, "mov $test_function, %eax \n\t" "call *%eax\n\t");
It's fragile to try to retrieve the stack pointer by taking the address of a variable on the stack. For instance, clang reserves more stack space than gcc here, indicating that the variable may not be at the start of the stack. Instead of relying upon this to work, retrieve the "%rbp" value, which contains the value of "%rsp" before stack allocation. Signed-off-by: Bill Wendling <morbo@google.com> --- x86/realmode.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)