Re: [PATCH] Correct debug for -mcall-ms2sysv-xlogues stubs (PR target/83917, take 2)

[Daniel Santos <daniel dot santos at pobox dot com>]

Sorry for the dropping the ball on this and thank you Jakub for stepping in!

I've had a patch set sort-of rotting in my local repo, but I like yours
better.  I think I had gotten hung up on trying to figure out how to
write a test for this, and like you I just tested mine manually in gdb. 
I do have one correction though.


On 02/22/2018 08:56 AM, Jakub Jelinek wrote:
> Hi!
>
> On Sat, Jan 20, 2018 at 06:01:16PM -0600, Daniel Santos wrote:
>> Thanks.  I like the idea of commonizing the macros for consistency.
> Didn't see a progress on this P3 for a while, so I've written this
> version of the patch; no tests though, what I've been using in testing was:
> /* { dg-do compile { target lp64 } } */
> /* { dg-options "-mno-avx -msse2 -mcall-ms2sysv-xlogues -O2" } */
>
> void __attribute__((sysv_abi, noipa))
> foo (void)
> {
> }
>
> static void __attribute__((sysv_abi)) (*volatile foop) () = foo;
>
> void __attribute__((ms_abi, noipa))
> bar (void)
> {
>   foop ();
> }
>
> int
> main ()
> {
>   bar ();
>   return 0;
> }
>
> with/without -fno-omit-frame-pointer, disas bar; b on the tail
> call in there, stepi; bt (which before the patch failed, now works),
> also up; p $rbp to see if %rbp has been properly declared to be saved.
> There is no need to cfi_startproc/cfi_endproc for every single entrypoint in
> there, it is enough if the whole range is covered.  On the other side
> we need the cfi_offset for the frame pointer case, otherwise up; p/x $rbp
> doesn't work properly.
>
> Ok for trunk if it passes bootstrap/regtest on x86_64-linux and i686-linux?
>
> 2018-02-22  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR debug/83917
> 	* config/i386/i386-asm.h (PACKAGE_VERSION, PACKAGE_NAME,
> 	PACKAGE_STRING, PACKAGE_TARNAME, PACKAGE_URL): Undefine between
> 	inclusion of auto-target.h and auto-host.h.
> 	(USE_GAS_CFI_DIRECTIVES): Define if not defined already based on
> 	__GCC_HAVE_DWARF2_CFI_ASM.
> 	(cfi_startproc, cfi_endproc, cfi_adjust_cfa_offset,
> 	cfi_def_cfa_register, cfi_def_cfa, cfi_register, cfi_offset, cfi_push,
> 	cfi_pop): Define.
> 	* config/i386/cygwin.S: Don't include auto-host.h here, just
> 	define USE_GAS_CFI_DIRECTIVES to 1 or 0 and include i386-asm.h.
> 	(cfi_startproc, cfi_endproc, cfi_adjust_cfa_offset,
> 	cfi_def_cfa_register, cfi_register, cfi_push, cfi_pop): Remove.
> 	* config/i386/resms64fx.h: Add cfi_* directives.
> 	* config/i386/resms64x.h: Likewise.
>
> --- libgcc/config/i386/i386-asm.h.jj	2018-01-03 10:42:56.317763517 +0100
> +++ libgcc/config/i386/i386-asm.h	2018-02-22 15:33:43.812922298 +0100
> @@ -27,8 +27,47 @@ see the files COPYING3 and COPYING.RUNTI
>  #define I386_ASM_H
>  
>  #include "auto-target.h"
> +#undef PACKAGE_VERSION
> +#undef PACKAGE_NAME
> +#undef PACKAGE_STRING
> +#undef PACKAGE_TARNAME
> +#undef PACKAGE_URL

This is a beautiful, temporary(?) fix to an ugly problem!

>  #include "auto-host.h"
>  
> +#ifndef USE_GAS_CFI_DIRECTIVES
> +# ifdef __GCC_HAVE_DWARF2_CFI_ASM
> +#  define USE_GAS_CFI_DIRECTIVES 1
> +# else
> +#  define USE_GAS_CFI_DIRECTIVES 0
> +# endif
> +#endif
> +#if USE_GAS_CFI_DIRECTIVES
> +# 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_def_cfa(R,O)		.cfi_def_cfa R, O
> +# define cfi_register(D,S)		.cfi_register D, S
> +# define cfi_offset(R,O)		.cfi_offset R, O
> +# 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_def_cfa(R,O)
> +# define cfi_register(D,S)
> +# define cfi_offset(R,O)
> +# define cfi_push(X)
> +# define cfi_pop(X)
> +#endif
> +
>  #define PASTE2(a, b) PASTE2a(a, b)
>  #define PASTE2a(a, b) a ## b
>  
> --- libgcc/config/i386/cygwin.S.jj	2018-01-03 10:42:56.309763515 +0100
> +++ libgcc/config/i386/cygwin.S	2018-02-22 15:30:34.597925496 +0100
> @@ -23,31 +23,13 @@
>   * <http://www.gnu.org/licenses/>.
>   */
>  
> -#include "auto-host.h"

The following include should be here.

+#include "i386-asm.h"


> -
>  #ifdef HAVE_GAS_CFI_SECTIONS_DIRECTIVE
> +# define USE_GAS_CFI_DIRECTIVES 1
>  	.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 */
> +# define USE_GAS_CFI_DIRECTIVES 0
> +#endif
> +#include "i386-asm.h"

...instead of where it is above. ^^

>  
>  #ifdef L_chkstk
>  /* Function prologue calls __chkstk to probe the stack when allocating more
> --- libgcc/config/i386/resms64fx.h.jj	2018-01-03 10:42:56.246763504 +0100
> +++ libgcc/config/i386/resms64fx.h	2018-02-22 15:14:53.391623798 +0100
> @@ -33,6 +33,9 @@ see the files COPYING3 and COPYING.RUNTI
>   * from the function.  */
>  
>  	.text
> +	cfi_startproc()
> +	cfi_offset(%rbp, -16)
> +	cfi_def_cfa(%rbp, 16)
>  MS2SYSV_STUB_BEGIN(resms64fx_17)
>  	mov	-0x68(%rsi),%r15
>  MS2SYSV_STUB_BEGIN(resms64fx_16)
> @@ -48,7 +51,9 @@ MS2SYSV_STUB_BEGIN(resms64fx_12)
>  	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)
> --- libgcc/config/i386/resms64x.h.jj	2018-01-03 10:42:56.308763515 +0100
> +++ libgcc/config/i386/resms64x.h	2018-02-22 15:02:00.400106064 +0100
> @@ -32,6 +32,8 @@ see the files COPYING3 and COPYING.RUNTI
>   * function.  */
>  
>  	.text
> +	cfi_startproc()
> +	cfi_def_cfa(%r10, 8)
>  MS2SYSV_STUB_BEGIN(resms64x_18)
>  	mov	-0x70(%rsi),%r15
>  MS2SYSV_STUB_BEGIN(resms64x_17)
> @@ -49,7 +51,9 @@ MS2SYSV_STUB_BEGIN(resms64x_12)
>  	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)
>
>
> 	Jakub
>

Thanks Jakub!
Daniel