[PATCH] Cygwin: remove %esp from asm clobber list

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Cygwin: remove %esp from asm clobber list

Jon TURNEY
Mentioning the stack pointer in the clobber list is now a gcc warning.

We never wanted gcc to try to restore %esp after this (x86-specific)
asm, since the whole point of the inline asm here is to adjust %esp to
satisfy alignment, so remove %esp from the asm clobber list.

Of more concern is the alleged requirement that %esp must be unchanged
over an asm statement (which makes what this code is trying to do
impossible to write as a C function), although on x86 we are probably ok
in this particular instance.

../../../../winsup/cygwin/init.cc: In function 'void threadfunc_fe(void*)':
../../../../winsup/cygwin/init.cc:33:46: error: listing the stack pointer register '%esp' in a clobber list is deprecated [-Werror=deprecated]
../../../../winsup/cygwin/init.cc:33:46: note: the value of the stack pointer after an 'asm' statement must be the same as it was before the statement

Also, because we now using gcc's "basic" rather than "extended" asm
syntax we don't need to escape the '%' in '%esp' as '%%esp'.
---

Notes:
    N.B: This comes with a 'this should be ok, but I haven't actually
    tested that x86 Cygwin works after this' caveat.

 winsup/cygwin/crt0.c  | 2 +-
 winsup/cygwin/init.cc | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/crt0.c b/winsup/cygwin/crt0.c
index fee4b2e24..9fcebd8fa 100644
--- a/winsup/cygwin/crt0.c
+++ b/winsup/cygwin/crt0.c
@@ -27,7 +27,7 @@ mainCRTStartup ()
 #if __GNUC_PREREQ(6,0)
 #pragma GCC diagnostic pop
 #endif
-  asm volatile ("andl $-16,%%esp" ::: "%esp");
+  asm volatile ("andl $-16,%esp");
 #endif
 
   cygwin_crt0 (main);
diff --git a/winsup/cygwin/init.cc b/winsup/cygwin/init.cc
index 851a7ffed..7ae7d08fe 100644
--- a/winsup/cygwin/init.cc
+++ b/winsup/cygwin/init.cc
@@ -30,7 +30,7 @@ threadfunc_fe (VOID *arg)
 #if __GNUC_PREREQ(6,0)
 #pragma GCC diagnostic pop
 #endif
-  asm volatile ("andl $-16,%%esp" ::: "%esp");
+  asm volatile ("andl $-16,%esp");
 #endif
   _cygtls::call ((DWORD (*)  (void *, void *)) TlsGetValue (_my_oldfunc), arg);
 }
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: remove %esp from asm clobber list

Corinna Vinschen-2
On Feb 28 12:04, Jon Turney wrote:

> Mentioning the stack pointer in the clobber list is now a gcc warning.
>
> We never wanted gcc to try to restore %esp after this (x86-specific)
> asm, since the whole point of the inline asm here is to adjust %esp to
> satisfy alignment, so remove %esp from the asm clobber list.
>
> Of more concern is the alleged requirement that %esp must be unchanged
> over an asm statement (which makes what this code is trying to do
> impossible to write as a C function), although on x86 we are probably ok
> in this particular instance.
>
> ../../../../winsup/cygwin/init.cc: In function 'void threadfunc_fe(void*)':
> ../../../../winsup/cygwin/init.cc:33:46: error: listing the stack pointer register '%esp' in a clobber list is deprecated [-Werror=deprecated]
> ../../../../winsup/cygwin/init.cc:33:46: note: the value of the stack pointer after an 'asm' statement must be the same as it was before the statement
>
> Also, because we now using gcc's "basic" rather than "extended" asm
> syntax we don't need to escape the '%' in '%esp' as '%%esp'.
> ---
As discussed on IRC, let's try to fix that using the gcc funtion
attribute force_align_arg_pointer, as Mingw-w64 already did in 2018.


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: remove %esp from asm clobber list

Johannes Schindelin
In reply to this post by Jon TURNEY
Hi,

On Fri, 28 Feb 2020, Jon Turney wrote:

> Mentioning the stack pointer in the clobber list is now a gcc warning.
>
> We never wanted gcc to try to restore %esp after this (x86-specific)
> asm, since the whole point of the inline asm here is to adjust %esp to
> satisfy alignment, so remove %esp from the asm clobber list.
>
> Of more concern is the alleged requirement that %esp must be unchanged
> over an asm statement (which makes what this code is trying to do
> impossible to write as a C function), although on x86 we are probably ok
> in this particular instance.
>
> ../../../../winsup/cygwin/init.cc: In function 'void threadfunc_fe(void*)':
> ../../../../winsup/cygwin/init.cc:33:46: error: listing the stack pointer register '%esp' in a clobber list is deprecated [-Werror=deprecated]
> ../../../../winsup/cygwin/init.cc:33:46: note: the value of the stack pointer after an 'asm' statement must be the same as it was before the statement
>
> Also, because we now using gcc's "basic" rather than "extended" asm
> syntax we don't need to escape the '%' in '%esp' as '%%esp'.
> ---
>
> Notes:
>     N.B: This comes with a 'this should be ok, but I haven't actually
>     tested that x86 Cygwin works after this' caveat.

We run with this patch in Git for Windows for quite a while, and I have
yet to hear complaints:

https://github.com/git-for-windows/msys2-runtime/commit/dd5aa267f6f28aef2a241cc7a01bb71a434b403c

As far as I can tell, MSYS2 does the same.

So there is at least some confirmation to be had ;-)

Ciao,
Dscho

>
>  winsup/cygwin/crt0.c  | 2 +-
>  winsup/cygwin/init.cc | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/winsup/cygwin/crt0.c b/winsup/cygwin/crt0.c
> index fee4b2e24..9fcebd8fa 100644
> --- a/winsup/cygwin/crt0.c
> +++ b/winsup/cygwin/crt0.c
> @@ -27,7 +27,7 @@ mainCRTStartup ()
>  #if __GNUC_PREREQ(6,0)
>  #pragma GCC diagnostic pop
>  #endif
> -  asm volatile ("andl $-16,%%esp" ::: "%esp");
> +  asm volatile ("andl $-16,%esp");
>  #endif
>
>    cygwin_crt0 (main);
> diff --git a/winsup/cygwin/init.cc b/winsup/cygwin/init.cc
> index 851a7ffed..7ae7d08fe 100644
> --- a/winsup/cygwin/init.cc
> +++ b/winsup/cygwin/init.cc
> @@ -30,7 +30,7 @@ threadfunc_fe (VOID *arg)
>  #if __GNUC_PREREQ(6,0)
>  #pragma GCC diagnostic pop
>  #endif
> -  asm volatile ("andl $-16,%%esp" ::: "%esp");
> +  asm volatile ("andl $-16,%esp");
>  #endif
>    _cygtls::call ((DWORD (*)  (void *, void *)) TlsGetValue (_my_oldfunc), arg);
>  }
> --
> 2.21.0
>
>