Re: [PATCH] Cygwin: remove %esp from asm clobber list
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
>
>
Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
Am 28.02.2020 um 14:31 schrieb Corinna Vinschen:
[CC Hans]
Thanks. I wasn't subscribed to -patches so far. Will change that.
Hans,
as for making a patch for this issue, may I leave it to you
because you are already working on it?
My patch was meant only as a minimally-invasive stop-gap fix, because
the new GCC refused to compile the code as-is (it triggers a
-Werror...). As such, sorry for hurting Brian's eyes. ;-)
I agree that this really should be an inline function. I barely speak
C++, but even I have glimpsed that multi-statement macros are rightfully
frowned upon in C++ circles.
I believe a more C++-like implementation would have to encapsulate wpbuf
and wpixput into a helper class. This is what my _very_ rusty C++
allowed me to come up with:
// simple helper class to accumulate output in a buffer
// and send that to the console on request:
static class
{
private:
unsigned char buf[WPBUF_LEN];
int ixput;
public:
inline void put(unsigned char x)
{
if (ixput < WPBUF_LEN)
{
buf[ixput++] = x;
}
};
inline void empty() { ixput = 0; };
inline void sendOut(HANDLE , DWORD *wn) {
WriteConsoleA (handle, buf, ixput, wn, 0);
};
} wpbuf;
Using it works like this:
s/wpbuf_put/wpbuf.put/
s/wpixput = 0/wpbuf.empty()/
s/WriteConsoleA ( get_output_handle (), wpbuf, wpixput, \(*n\), 0
/wpbuf.sendOut( get_output_handle (), \1/g
Yes, sendOut() is ugly --- I didn't manage to find out how this class
could access get_output_handle() itself, so I had to let its callers
deal with that.
Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
On Feb 29 02:06, Takashi Yano wrote: > On Fri, 28 Feb 2020 15:49:05 +0100 > Corinna Vinschen wrote: > > Also, on second thought, given wpbuf is global inside this file, doesn't > > this require guarding against multi-threaded access? > > wpbuf_put() is used in write(), and almost whole of write() > code is guarded by output_mutex. So, I think it is already > thread-safe. Ah, right, thanks. Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
On Fri, 28 Feb 2020 15:49:05 +0100 Corinna Vinschen wrote: > Also, on second thought, given wpbuf is global inside this file, doesn't > this require guarding against multi-threaded access? wpbuf_put() is used in write(), and almost whole of write() code is guarded by output_mutex. So, I think it is already thread-safe. -- Takashi Yano
Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
On Feb 28 15:44, Corinna Vinschen wrote:
> On Feb 28 14:31, Corinna Vinschen wrote:
> > [CC Hans]
> >
> > On Feb 28 11:14, Takashi Yano wrote:
> > > On Thu, 27 Feb 2020 18:03:47 +
> > > Jon Turney wrote:
> > > > > +#define wpbuf_put(x) \
> > > > > + wpbuf[wpixput++] = x; \
> > > > > + if (wpixput > WPBUF_LEN) \
> > > > > +wpixput--;
> > > > > +
> > > >
> > > > So I think either the macro need it contents contained by a 'do { ... }
> > > > while(0)', or that instance of it needs to be surrounded by braces, to
> > > > do what you intend.
> > >
> > > Thanks for the advice. Fortunately, "if" statement does not
> > > cause a problem even if it is accidentally executed outside
> > > "else" block in this case.
> > >
> > > Hans,
> > > as for making a patch for this issue, may I leave it to you
> > > because you are already working on it?
> > >
> > > --
> > > Takashi Yano
>
> What about an inline function instead?
>
> diff --git a/winsup/cygwin/fhandler_console.cc
> b/winsup/cygwin/fhandler_console.cc
> index 64e12b8320a1..6c3e33818aca 100644
> --- a/winsup/cygwin/fhandler_console.cc
> +++ b/winsup/cygwin/fhandler_console.cc
> @@ -63,10 +63,14 @@ static struct fhandler_base::rabuf_t con_ra;
> static unsigned char wpbuf[WPBUF_LEN];
> static int wpixput;
> static unsigned char last_char;
> -#define wpbuf_put(x) \
> - wpbuf[wpixput++] = x; \
> - if (wpixput > WPBUF_LEN) \
> +
> +static inline void
> +wpbuf_put (unsigned char x)
> +{
> + wpbuf[wpixput++] = x;
> + if (wpixput > WPBUF_LEN)
> wpixput--;
> +}
Also, on second thought, given wpbuf is global inside this file, doesn't
this require guarding against multi-threaded access?
Corinna
--
Corinna Vinschen
Cygwin Maintainer
signature.asc
Description: PGP signature
Re: [PATCH] Cygwin: console: Adjust the detailed behaviour of ESC sequences.
On Feb 27 11:33, Takashi Yano wrote: > - This patch makes some detailed behaviour of ESC sequences such as > "CSI Ps L" (IL), "CSI Ps M" (DL) and "ESC M" (RI) in xterm mode > match with real xterm. > --- > winsup/cygwin/fhandler.h | 1 + > winsup/cygwin/fhandler_console.cc | 51 ++- > 2 files changed, 45 insertions(+), 7 deletions(-) Pushed. Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
On Feb 28 14:31, Corinna Vinschen wrote:
> [CC Hans]
>
> On Feb 28 11:14, Takashi Yano wrote:
> > On Thu, 27 Feb 2020 18:03:47 +
> > Jon Turney wrote:
> > > > +#define wpbuf_put(x) \
> > > > + wpbuf[wpixput++] = x; \
> > > > + if (wpixput > WPBUF_LEN) \
> > > > +wpixput--;
> > > > +
> > >
> > > So I think either the macro need it contents contained by a 'do { ... }
> > > while(0)', or that instance of it needs to be surrounded by braces, to
> > > do what you intend.
> >
> > Thanks for the advice. Fortunately, "if" statement does not
> > cause a problem even if it is accidentally executed outside
> > "else" block in this case.
> >
> > Hans,
> > as for making a patch for this issue, may I leave it to you
> > because you are already working on it?
> >
> > --
> > Takashi Yano
What about an inline function instead?
diff --git a/winsup/cygwin/fhandler_console.cc
b/winsup/cygwin/fhandler_console.cc
index 64e12b8320a1..6c3e33818aca 100644
--- a/winsup/cygwin/fhandler_console.cc
+++ b/winsup/cygwin/fhandler_console.cc
@@ -63,10 +63,14 @@ static struct fhandler_base::rabuf_t con_ra;
static unsigned char wpbuf[WPBUF_LEN];
static int wpixput;
static unsigned char last_char;
-#define wpbuf_put(x) \
- wpbuf[wpixput++] = x; \
- if (wpixput > WPBUF_LEN) \
+
+static inline void
+wpbuf_put (unsigned char x)
+{
+ wpbuf[wpixput++] = x;
+ if (wpixput > WPBUF_LEN)
wpixput--;
+}
static void
beep ()
Corinna
--
Corinna Vinschen
Cygwin Maintainer
signature.asc
Description: PGP signature
Re: [PATCH] Cygwin: remove %esp from asm clobber list
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 Description: PGP signature
Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
[CC Hans]
On Feb 28 11:14, Takashi Yano wrote:
> On Thu, 27 Feb 2020 18:03:47 +
> Jon Turney wrote:
> > > +#define wpbuf_put(x) \
> > > + wpbuf[wpixput++] = x; \
> > > + if (wpixput > WPBUF_LEN) \
> > > +wpixput--;
> > > +
> >
> > So I think either the macro need it contents contained by a 'do { ... }
> > while(0)', or that instance of it needs to be surrounded by braces, to
> > do what you intend.
>
> Thanks for the advice. Fortunately, "if" statement does not
> cause a problem even if it is accidentally executed outside
> "else" block in this case.
>
> Hans,
> as for making a patch for this issue, may I leave it to you
> because you are already working on it?
>
> --
> Takashi Yano
--
Corinna Vinschen
Cygwin Maintainer
signature.asc
Description: PGP signature
[PATCH] Cygwin: remove %esp from asm clobber list
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
