| Message ID | 20250325180005.275552-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/elf: Remove ASM_CALL_CONSTRAINT from elf_core_save_regs() | expand |
On 25.03.2025 19:00, Andrew Cooper wrote: > I was mistaken about when ASM_CALL_CONSTRAINT is applicable. It is not > applicable for plain pushes/pops, so remove it from the flags logic. > > Clarify the description of ASM_CALL_CONSTRAINT to be explicit about unwinding > using framepointers. > > Fixes: 0754534b8a38 ("x86/elf: Improve code generation in elf_core_save_regs()") > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/arch/x86/include/asm/asm_defns.h | 5 +++-- > xen/arch/x86/include/asm/x86_64/elf.h | 2 +- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/include/asm/asm_defns.h b/xen/arch/x86/include/asm/asm_defns.h > index 92b4116a1564..689d1dcbf754 100644 > --- a/xen/arch/x86/include/asm/asm_defns.h > +++ b/xen/arch/x86/include/asm/asm_defns.h > @@ -28,8 +28,9 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, " > > /* > * This output constraint should be used for any inline asm which has a "call" > - * instruction. Otherwise the asm may be inserted before the frame pointer > - * gets set up by the containing function. > + * instruction, which forces the stack frame to be set up prior to the asm > + * block. This matters when unwinding using framepointers, where the asm's > + * function can get skipped over. Does "forces the stack frame to be set up" really mean the stack frame, or the frame pointer (if one is in use)? In the latter case I can see how the asm() being moved ahead of that point could cause problems. In the former case I apparently still don't understand (yet) what the issue is that ASM_CALL_CONSTRAINT ultimately is to help with. Jan
On 26/03/2025 9:00 am, Jan Beulich wrote: > On 25.03.2025 19:00, Andrew Cooper wrote: >> I was mistaken about when ASM_CALL_CONSTRAINT is applicable. It is not >> applicable for plain pushes/pops, so remove it from the flags logic. >> >> Clarify the description of ASM_CALL_CONSTRAINT to be explicit about unwinding >> using framepointers. >> >> Fixes: 0754534b8a38 ("x86/elf: Improve code generation in elf_core_save_regs()") >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> --- >> xen/arch/x86/include/asm/asm_defns.h | 5 +++-- >> xen/arch/x86/include/asm/x86_64/elf.h | 2 +- >> 2 files changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/x86/include/asm/asm_defns.h b/xen/arch/x86/include/asm/asm_defns.h >> index 92b4116a1564..689d1dcbf754 100644 >> --- a/xen/arch/x86/include/asm/asm_defns.h >> +++ b/xen/arch/x86/include/asm/asm_defns.h >> @@ -28,8 +28,9 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, " >> >> /* >> * This output constraint should be used for any inline asm which has a "call" >> - * instruction. Otherwise the asm may be inserted before the frame pointer >> - * gets set up by the containing function. >> + * instruction, which forces the stack frame to be set up prior to the asm >> + * block. This matters when unwinding using framepointers, where the asm's >> + * function can get skipped over. > Does "forces the stack frame to be set up" really mean the stack frame, or the > frame pointer (if one is in use)? What do you consider to be the difference, given how frame pointers work in our ABI? It is the frame pointer which needs setting up, which at a minimum involves spilling registers to the stack and getting %rsp into it's eventual position. > In the latter case I can see how the asm() > being moved ahead of that point could cause problems. In the former case I > apparently still don't understand (yet) what the issue is that > ASM_CALL_CONSTRAINT ultimately is to help with. The specific bug is from a sequence of functions a, b and c, where b uses an asm() to call c. a() pushes old %rbp sets up new %rbp, b() fails to do so early enough, and c() pushes a()'s frame pointer, not b()'s. Then unwinding via frame pointer skips b() in the backtrace. I don't know what the precise effects of the constraint are. The compiler maintainers can't agree either, and say it's fragile and can't be relied upon, but it seems to be the only way to have the desired effect on emitted code. ~Andrew
On 26.03.2025 10:17, Andrew Cooper wrote: > On 26/03/2025 9:00 am, Jan Beulich wrote: >> On 25.03.2025 19:00, Andrew Cooper wrote: >>> I was mistaken about when ASM_CALL_CONSTRAINT is applicable. It is not >>> applicable for plain pushes/pops, so remove it from the flags logic. >>> >>> Clarify the description of ASM_CALL_CONSTRAINT to be explicit about unwinding >>> using framepointers. >>> >>> Fixes: 0754534b8a38 ("x86/elf: Improve code generation in elf_core_save_regs()") >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Jan Beulich <JBeulich@suse.com> >>> CC: Roger Pau Monné <roger.pau@citrix.com> >>> --- >>> xen/arch/x86/include/asm/asm_defns.h | 5 +++-- >>> xen/arch/x86/include/asm/x86_64/elf.h | 2 +- >>> 2 files changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/xen/arch/x86/include/asm/asm_defns.h b/xen/arch/x86/include/asm/asm_defns.h >>> index 92b4116a1564..689d1dcbf754 100644 >>> --- a/xen/arch/x86/include/asm/asm_defns.h >>> +++ b/xen/arch/x86/include/asm/asm_defns.h >>> @@ -28,8 +28,9 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, " >>> >>> /* >>> * This output constraint should be used for any inline asm which has a "call" >>> - * instruction. Otherwise the asm may be inserted before the frame pointer >>> - * gets set up by the containing function. >>> + * instruction, which forces the stack frame to be set up prior to the asm >>> + * block. This matters when unwinding using framepointers, where the asm's >>> + * function can get skipped over. >> Does "forces the stack frame to be set up" really mean the stack frame, or the >> frame pointer (if one is in use)? > > What do you consider to be the difference, given how frame pointers work > in our ABI? My point is that frame pointers are an optional part. Sufficiently high optimization levels omit them by default, iirc. And depending on CONFIG_FRAME_POINTER we may explicitly pass -fno-omit-frame-pointer. Even in that case there is a stack frame that the compiler is setting up. Yet in that case the effect of ASM_CALL_CONSTRAINT is not relevant. Hence also why the construct expands to nothing in that case. The comment, however, is placed outside if the #ifdef, and hence applies to both forms (according to the way I read such, at least). > It is the frame pointer which needs setting up, which at a minimum > involves spilling registers to the stack and getting %rsp into it's > eventual position. Right, and all I'm effectively asking for is s/stack frame/frame pointer/ in the new comment text. Then: Reviewed-by: Jan Beulich <jbeulich@suse.com> Alternatively part or all of the comment could be moved inside the #ifdef. Jan
diff --git a/xen/arch/x86/include/asm/asm_defns.h b/xen/arch/x86/include/asm/asm_defns.h index 92b4116a1564..689d1dcbf754 100644 --- a/xen/arch/x86/include/asm/asm_defns.h +++ b/xen/arch/x86/include/asm/asm_defns.h @@ -28,8 +28,9 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, " /* * This output constraint should be used for any inline asm which has a "call" - * instruction. Otherwise the asm may be inserted before the frame pointer - * gets set up by the containing function. + * instruction, which forces the stack frame to be set up prior to the asm + * block. This matters when unwinding using framepointers, where the asm's + * function can get skipped over. */ #ifdef CONFIG_FRAME_POINTER register unsigned long current_stack_pointer asm("rsp"); diff --git a/xen/arch/x86/include/asm/x86_64/elf.h b/xen/arch/x86/include/asm/x86_64/elf.h index f33be46ddec9..e7bec7327aa2 100644 --- a/xen/arch/x86/include/asm/x86_64/elf.h +++ b/xen/arch/x86/include/asm/x86_64/elf.h @@ -56,7 +56,7 @@ static inline void elf_core_save_regs(ELF_Gregset *core_regs, /* orig_rax not filled in for now */ asm ( "lea (%%rip), %0" : "=r" (core_regs->rip) ); asm ( "mov %%cs, %0" : "=m" (core_regs->cs) ); - asm ( "pushfq; popq %0" : "=m" (core_regs->rflags) ASM_CALL_CONSTRAINT ); + asm ( "pushfq; popq %0" : "=m" (core_regs->rflags) ); asm ( "movq %%rsp, %0" : "=m" (core_regs->rsp) ); asm ( "mov %%ss, %0" : "=m" (core_regs->ss) ); rdmsrl(MSR_FS_BASE, core_regs->thread_fs);
I was mistaken about when ASM_CALL_CONSTRAINT is applicable. It is not applicable for plain pushes/pops, so remove it from the flags logic. Clarify the description of ASM_CALL_CONSTRAINT to be explicit about unwinding using framepointers. Fixes: 0754534b8a38 ("x86/elf: Improve code generation in elf_core_save_regs()") Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/include/asm/asm_defns.h | 5 +++-- xen/arch/x86/include/asm/x86_64/elf.h | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) base-commit: 28fa31d6bb7835be530c2855dd6cf4e77438ae12