[x86,libgcc] PR target/83917 Correct debug for -mcall-ms2sysv-xlogues stubs

Message ID 20180119233310.19670-1-daniel.santos@pobox.com
State New
Headers show
Series
  • [x86,libgcc] PR target/83917 Correct debug for -mcall-ms2sysv-xlogues stubs
Related show

Commit Message

Daniel Santos Jan. 19, 2018, 11:33 p.m.
When stepping through tail-call restore stubs the debugger has to assume
that rsp - 8 is the CFA, although it is not.  This is because I did not
explicitly add any .cfi directives.  This patch adds them to the
tail-call restore stubs, but this is new territory for me, so I would
appreciate feedback.

I've reg-tested on x86_64, but I still need to test on Solaris and
Darwin.  OK to commit after those tests?

Thanks,
Daniel

Signed-off-by: Daniel Santos <daniel.santos@pobox.com>

---
 libgcc/config/i386/resms64fx.h | 19 +++++++++++++++++++
 libgcc/config/i386/resms64x.h  | 22 ++++++++++++++++++++++
 2 files changed, 41 insertions(+)

-- 
2.15.0

Comments

Jakub Jelinek Jan. 19, 2018, 11:35 p.m. | #1
On Fri, Jan 19, 2018 at 05:33:10PM -0600, Daniel Santos wrote:
> When stepping through tail-call restore stubs the debugger has to assume

> that rsp - 8 is the CFA, although it is not.  This is because I did not

> explicitly add any .cfi directives.  This patch adds them to the

> tail-call restore stubs, but this is new territory for me, so I would

> appreciate feedback.

> 

> I've reg-tested on x86_64, but I still need to test on Solaris and

> Darwin.  OK to commit after those tests?


I think you can't assume that the assembler supports .cfi_* directives.
While e.g. libgcc/config/i386/morestack.S uses them unconditionally,
it is guarded with:
        if test "$libgcc_cv_cfi" = "yes"; then
                tmake_file="${tmake_file} t-stack i386/t-stack-i386"
        fi
in config.host.  E.g. cygwin.S has:
#ifdef HAVE_GAS_CFI_SECTIONS_DIRECTIVE
        .cfi_sections   .debug_frame
# define cfi_startproc()                .cfi_startproc
# define cfi_endproc()                  .cfi_endproc
# define cfi_adjust_cfa_offset(X)       .cfi_adjust_cfa_offset X
# define cfi_def_cfa_register(X)        .cfi_def_cfa_register X
# define cfi_register(D,S)              .cfi_register D, S
# ifdef __x86_64__
#  define cfi_push(X)           .cfi_adjust_cfa_offset 8; .cfi_rel_offset X, 0
#  define cfi_pop(X)            .cfi_adjust_cfa_offset -8; .cfi_restore X
# else
#  define cfi_push(X)           .cfi_adjust_cfa_offset 4; .cfi_rel_offset X, 0
#  define cfi_pop(X)            .cfi_adjust_cfa_offset -4; .cfi_restore X
# endif
#else
# define cfi_startproc()
# define cfi_endproc()
# define cfi_adjust_cfa_offset(X)
# define cfi_def_cfa_register(X)
# define cfi_register(D,S)
# define cfi_push(X)
# define cfi_pop(X)
#endif /* HAVE_GAS_CFI_SECTIONS_DIRECTIVE */
perhaps you need something similar or commonize that (though, without
.cfi_sections, you want the default).

	Jakub
Daniel Santos Jan. 21, 2018, 12:01 a.m. | #2
On 01/19/2018 05:35 PM, Jakub Jelinek wrote:
> On Fri, Jan 19, 2018 at 05:33:10PM -0600, Daniel Santos wrote:

>> When stepping through tail-call restore stubs the debugger has to assume

>> that rsp - 8 is the CFA, although it is not.  This is because I did not

>> explicitly add any .cfi directives.  This patch adds them to the

>> tail-call restore stubs, but this is new territory for me, so I would

>> appreciate feedback.

>>

>> I've reg-tested on x86_64, but I still need to test on Solaris and

>> Darwin.  OK to commit after those tests?

> I think you can't assume that the assembler supports .cfi_* directives.

> While e.g. libgcc/config/i386/morestack.S uses them unconditionally,

> it is guarded with:

>         if test "$libgcc_cv_cfi" = "yes"; then

>                 tmake_file="${tmake_file} t-stack i386/t-stack-i386"

>         fi


Ah hah! That explains a lot.  Yeah, I wasn't thinking all assemblers
would support it but I saw them in the Solaris assembler manual and
figured that they were maybe more widely supported than I had thought.

> in config.host.  E.g. cygwin.S has:

> #ifdef HAVE_GAS_CFI_SECTIONS_DIRECTIVE

>         .cfi_sections   .debug_frame

> # define cfi_startproc()                .cfi_startproc

> # define cfi_endproc()                  .cfi_endproc

> # define cfi_adjust_cfa_offset(X)       .cfi_adjust_cfa_offset X

> # define cfi_def_cfa_register(X)        .cfi_def_cfa_register X

> # define cfi_register(D,S)              .cfi_register D, S

> # ifdef __x86_64__

> #  define cfi_push(X)           .cfi_adjust_cfa_offset 8; .cfi_rel_offset X, 0

> #  define cfi_pop(X)            .cfi_adjust_cfa_offset -8; .cfi_restore X

> # else

> #  define cfi_push(X)           .cfi_adjust_cfa_offset 4; .cfi_rel_offset X, 0

> #  define cfi_pop(X)            .cfi_adjust_cfa_offset -4; .cfi_restore X

> # endif

> #else

> # define cfi_startproc()

> # define cfi_endproc()

> # define cfi_adjust_cfa_offset(X)

> # define cfi_def_cfa_register(X)

> # define cfi_register(D,S)

> # define cfi_push(X)

> # define cfi_pop(X)

> #endif /* HAVE_GAS_CFI_SECTIONS_DIRECTIVE */

> perhaps you need something similar or commonize that (though, without

> .cfi_sections, you want the default).

>

> 	Jakub


Thanks.  I like the idea of commonizing the macros for consistency.

As far as adding tests, I guess I would need to dig into
lib/gcc-gdb-test.exp to figure out how to do that.

Daniel
Jakub Jelinek Jan. 21, 2018, 7:24 a.m. | #3
On Sat, Jan 20, 2018 at 06:01:16PM -0600, Daniel Santos wrote:
> As far as adding tests, I guess I would need to dig into

> lib/gcc-gdb-test.exp to figure out how to do that.


The gdb-test infrastructure allows essentially
b linenumber
p something
p somethingother

Not sure if it would work if instead of using a number of linenumber
you'd use the symbol for these snippets or symbol+number,
plus you are interested not in printing values of variables there, but
about sensible backtraces. 

Probably gdb's testsuite is better suited for that.

	Jakub

Patch

diff --git a/libgcc/config/i386/resms64fx.h b/libgcc/config/i386/resms64fx.h
index c5f63d879fe..7dc8c7d89ed 100644
--- a/libgcc/config/i386/resms64fx.h
+++ b/libgcc/config/i386/resms64fx.h
@@ -34,21 +34,40 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 
 	.text
 MS2SYSV_STUB_BEGIN(resms64fx_17)
+.cfi_startproc
+.cfi_def_cfa %rbp, 16
 	mov	-0x68(%rsi),%r15
+.cfi_endproc
 MS2SYSV_STUB_BEGIN(resms64fx_16)
+.cfi_startproc
+.cfi_def_cfa %rbp, 16
 	mov	-0x60(%rsi),%r14
+.cfi_endproc
 MS2SYSV_STUB_BEGIN(resms64fx_15)
+.cfi_startproc
+.cfi_def_cfa %rbp, 16
 	mov	-0x58(%rsi),%r13
+.cfi_endproc
 MS2SYSV_STUB_BEGIN(resms64fx_14)
+.cfi_startproc
+.cfi_def_cfa %rbp, 16
 	mov	-0x50(%rsi),%r12
+.cfi_endproc
 MS2SYSV_STUB_BEGIN(resms64fx_13)
+.cfi_startproc
+.cfi_def_cfa %rbp, 16
 	mov	-0x48(%rsi),%rbx
+.cfi_endproc
 MS2SYSV_STUB_BEGIN(resms64fx_12)
+.cfi_startproc
+.cfi_def_cfa %rbp, 16
 	mov	-0x40(%rsi),%rdi
 	SSE_RESTORE
 	mov	-0x38(%rsi),%rsi
 	leaveq
+.cfi_def_cfa %rsp, 8
 	ret
+.cfi_endproc
 MS2SYSV_STUB_END(resms64fx_12)
 MS2SYSV_STUB_END(resms64fx_13)
 MS2SYSV_STUB_END(resms64fx_14)
diff --git a/libgcc/config/i386/resms64x.h b/libgcc/config/i386/resms64x.h
index 1b44938ae7c..753be1f4c52 100644
--- a/libgcc/config/i386/resms64x.h
+++ b/libgcc/config/i386/resms64x.h
@@ -33,23 +33,45 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 
 	.text
 MS2SYSV_STUB_BEGIN(resms64x_18)
+.cfi_startproc
+.cfi_def_cfa %r10, 8
 	mov	-0x70(%rsi),%r15
+.cfi_endproc
 MS2SYSV_STUB_BEGIN(resms64x_17)
+.cfi_startproc
+.cfi_def_cfa %r10, 8
 	mov	-0x68(%rsi),%r14
+.cfi_endproc
 MS2SYSV_STUB_BEGIN(resms64x_16)
+.cfi_startproc
+.cfi_def_cfa %r10, 8
 	mov	-0x60(%rsi),%r13
+.cfi_endproc
 MS2SYSV_STUB_BEGIN(resms64x_15)
+.cfi_startproc
+.cfi_def_cfa %r10, 8
 	mov	-0x58(%rsi),%r12
+.cfi_endproc
 MS2SYSV_STUB_BEGIN(resms64x_14)
+.cfi_startproc
+.cfi_def_cfa %r10, 8
 	mov	-0x50(%rsi),%rbp
+.cfi_endproc
 MS2SYSV_STUB_BEGIN(resms64x_13)
+.cfi_startproc
+.cfi_def_cfa %r10, 8
 	mov	-0x48(%rsi),%rbx
+.cfi_endproc
 MS2SYSV_STUB_BEGIN(resms64x_12)
+.cfi_startproc
+.cfi_def_cfa %r10, 8
 	mov	-0x40(%rsi),%rdi
 	SSE_RESTORE
 	mov	-0x38(%rsi),%rsi
 	mov	%r10,%rsp
+.cfi_def_cfa_register %rsp
 	ret
+.cfi_endproc
 MS2SYSV_STUB_END(resms64x_12)
 MS2SYSV_STUB_END(resms64x_13)
 MS2SYSV_STUB_END(resms64x_14)