Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
On 02/03/2020 19:54, Corinna Vinschen wrote: On Mar 2 18:03, Corinna Vinschen wrote: On Mar 1 15:38, Takashi Yano wrote: Hi Hans, [...] I pushed wpbuf_put as a simple inline function as a stop-gap measure so Cygwin builds on gcc 9.2.0. \o/ https://ci.appveyor.com/project/cygwin/cygwin/builds/31190427
Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
On Mar 2 18:03, Corinna Vinschen wrote:
> On Mar 1 15:38, Takashi Yano wrote:
> > Hi Hans,
> >
> > On Sat, 29 Feb 2020 19:10:02 +0100
> > Hans-Bernhard Bröker wrote:
> > > One more important note: the current implementation has a potential
> > > buffer overrun issue, because it writes first, and only then checks
> > > whether that may have overrun the buffer. And the check itself is off
> > > by one, too:
> > >
> > > >wpbuf[wpixput++] = x; \
> > > >if (wpixput > WPBUF_LEN) \
> > > > wpixput--; \
> > >
> > > That's why my latest code snippet does it differently:
> > >
> > > > if (ixput < WPBUF_LEN)
> > > >{
> > > > buf[ixput++] = x;
> > > >}
> >
> > Indeed. You are right. Thanks for pointing out that.
> > Another similar problem exists in console code of escape
> > sequence handling, so I will submit a patch for that.
> >
> > As for wpbuf, please continue to fix.
>
> Yeah, a patch in `git format-patch' format would be most welcome.
>
> For a first-time contribution (and then never again) we also need
> a 2-clause BSD license waiver per https://cygwin.com/contrib.html
I pushed wpbuf_put as a simple inline function as a stop-gap
measure so Cygwin builds on gcc 9.2.0.
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 Mar 1 15:38, Takashi Yano wrote:
> Hi Hans,
>
> On Sat, 29 Feb 2020 19:10:02 +0100
> Hans-Bernhard Bröker wrote:
> > One more important note: the current implementation has a potential
> > buffer overrun issue, because it writes first, and only then checks
> > whether that may have overrun the buffer. And the check itself is off
> > by one, too:
> >
> > >wpbuf[wpixput++] = x; \
> > >if (wpixput > WPBUF_LEN) \
> > > wpixput--; \
> >
> > That's why my latest code snippet does it differently:
> >
> > > if (ixput < WPBUF_LEN)
> > >{
> > > buf[ixput++] = x;
> > >}
>
> Indeed. You are right. Thanks for pointing out that.
> Another similar problem exists in console code of escape
> sequence handling, so I will submit a patch for that.
>
> As for wpbuf, please continue to fix.
Yeah, a patch in `git format-patch' format would be most welcome.
For a first-time contribution (and then never again) we also need
a 2-clause BSD license waiver per https://cygwin.com/contrib.html
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 Sun, 1 Mar 2020 14:56:31 +0100 Hans-Bernhard Bröker wrote: > Am 01.03.2020 um 07:33 schrieb Takashi Yano: > > > However, from the view point of performance, just inline > > static function is better. > > I don't see how that could be the case. Inline methods of a static C++ > object should not suffer any perfomance penalty compared to inline > functions operating on static variables. > > > Attached code measures the > > performance of access speed for wpbuf. > > I compiled it by g++ 7.4.0 with -O2 option. > > > > The result is as follows. > > > > Total1: 2.315627 second > > Total2: 1.588511 second > > Total3: 1.571572 second > > Strange. The result here (with GCC 9.2) is rather different: > > $ g++ -O2 -o tt wpbuf-bench.cc && ./tt > Total1: 0.753815 second > Total2: 0.757444 second > Total3: 1.217352 second > > And on inspection, all three bench*() functions do appear to have > exactly the same machine code, too. They may be inlined and mixed into > main() somewhat differently, though. That might explain the difference > more readily than any actual difference in speed between the three > implementations. I looked into the code generated by g++ 7.4.0 with -O2. The codes generated are different. With 32bit compiler, bench1(): L3: cmpl$255, %edx jg L2 movb$65, _wpbuf(%edx) movl$1, %ecx addl$1, %edx L2: subl$1, %eax [...] bench2(), bench3(): L22: cmpl$255, %edx jg L21 movb$65, _wpbuf2(%edx) addl$1, %edx L21: subl$1, %eax [...] With 64bit compiler, bench1(): .L3: cmpl$255, %edx jg .L2 movslq %edx, %rcx addl$1, %edx movb$65, (%r8,%rcx) movl$1, %ecx .L2: subl$1, %eax [...] bench2(), bench3(): .L15: cmpl$255, %edx jg .L14 movslq %edx, %rcx addl$1, %edx movb$65, (%r8,%rcx) .L14: subl$1, %eax [...] Obviously, code for bench2() and bench3() is shorter than bench1(). However, with g++ 9.2.0 with -O2, bench1(), bench2(), bench3(): L3: cmpl$255, %edx jg L2 movb$65, _wpbuf(%edx) addl$1, %edx L2: subl$1, %eax [...] all the codes are exactly the same, as you mentioned. So, if we assume g++ 9.2.0, please forget the previous remarks about speed. -- Takashi Yano
Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
Am 01.03.2020 um 07:33 schrieb Takashi Yano: However, from the view point of performance, just inline static function is better. I don't see how that could be the case. Inline methods of a static C++ object should not suffer any perfomance penalty compared to inline functions operating on static variables. Attached code measures the performance of access speed for wpbuf. I compiled it by g++ 7.4.0 with -O2 option. The result is as follows. Total1: 2.315627 second Total2: 1.588511 second Total3: 1.571572 second Strange. The result here (with GCC 9.2) is rather different: $ g++ -O2 -o tt wpbuf-bench.cc && ./tt Total1: 0.753815 second Total2: 0.757444 second Total3: 1.217352 second And on inspection, all three bench*() functions do appear to have exactly the same machine code, too. They may be inlined and mixed into main() somewhat differently, though. That might explain the difference more readily than any actual difference in speed between the three implementations.
Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
Hi Hans,
On Sat, 29 Feb 2020 19:10:02 +0100
Hans-Bernhard Bröker wrote:
> One more important note: the current implementation has a potential
> buffer overrun issue, because it writes first, and only then checks
> whether that may have overrun the buffer. And the check itself is off
> by one, too:
>
> >wpbuf[wpixput++] = x; \
> >if (wpixput > WPBUF_LEN) \
> > wpixput--; \
>
> That's why my latest code snippet does it differently:
>
> > if (ixput < WPBUF_LEN)
> >{
> > buf[ixput++] = x;
> >}
Indeed. You are right. Thanks for pointing out that.
Another similar problem exists in console code of escape
sequence handling, so I will submit a patch for that.
As for wpbuf, please continue to fix.
--
Takashi Yano
Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
On Fri, 28 Feb 2020 22:00:40 +0100
Hans-Bernhard Bröker wrote:
> // 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;
I agree your solution is more C++-like and smart.
However, from the view point of performance, just inline
static function is better. Attached code measures the
performance of access speed for wpbuf.
I compiled it by g++ 7.4.0 with -O2 option.
The result is as follows.
Total1: 2.315627 second
Total2: 1.588511 second
Total3: 1.571572 second
Class implementation is slow 40% than inline or macro.
So, IMHO, inline static function is the best.
--
Takashi Yano
#include
#include
#define WPBUF_LEN 256
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; };
} wpbuf;
unsigned char wpbuf2[WPBUF_LEN];
int ixput;
inline void wpbuf2_put(unsigned char x)
{
if (ixput < WPBUF_LEN) wpbuf2[ixput++] = x;
}
#define wpbuf3_put(x) \
{ \
if (ixput < WPBUF_LEN) wpbuf2[ixput++] = x; \
}
void bench1()
{
for (int i=0; i<1000; i++) {
wpbuf.empty();
for (int j=0; j
Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
One more important note: the current implementation has a potential
buffer overrun issue, because it writes first, and only then checks
whether that may have overrun the buffer. And the check itself is off
by one, too:
wpbuf[wpixput++] = x; \
if (wpixput > WPBUF_LEN) \
wpixput--; \
That's why my latest code snippet does it differently:
> if (ixput < WPBUF_LEN)
>{
> buf[ixput++] = x;
>}
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 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 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
Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
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
Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
On 26/02/2020 15:32, Takashi Yano wrote:
- Cygwin console with xterm compatible mode causes problem reported
in https://www.cygwin.com/ml/cygwin-patches/2020-q1/msg00212.html
if background/foreground colors are set to gray/black respectively
in Win10 1903/1909. This is caused by "CSI Ps L" (IL), "CSI Ps M"
(DL) and "ESC M" (RI) control sequences which are broken. This
patch adds a workaround for the issue.
---
winsup/cygwin/fhandler_console.cc | 156 +-
winsup/cygwin/wincap.cc | 10 ++
winsup/cygwin/wincap.h| 2 +
3 files changed, 166 insertions(+), 2 deletions(-)
diff --git a/winsup/cygwin/fhandler_console.cc
b/winsup/cygwin/fhandler_console.cc
index 328424a7d..c2198ea1e 100644
--- a/winsup/cygwin/fhandler_console.cc
+++ b/winsup/cygwin/fhandler_console.cc
@@ -57,6 +57,16 @@ bool NO_COPY fhandler_console::invisible_console;
Only one console can exist in a process, therefore, static is suitable. */
static struct fhandler_base::rabuf_t con_ra;
+/* Write pending buffer for ESC sequence handling
+ in xterm compatible mode */
+#define WPBUF_LEN 256
+static unsigned char wpbuf[WPBUF_LEN];
+static int wpixput;
+#define wpbuf_put(x) \
+ wpbuf[wpixput++] = x; \
+ if (wpixput > WPBUF_LEN) \
+wpixput--;
+
static void
beep ()
[...]
+ }
+ else
+ wpbuf_put (*src);
+ WriteConsoleA (get_output_handle (), wpbuf, wpixput, , 0);
+ con.state = normal;
+ wpixput = 0;
+ }
This generates a (useful!) warning with gcc 9.2.0:
../../../../winsup/cygwin/fhandler_console.cc: In member function 'virtual
ssize_t fhandler_console::write(const void*, size_t)':
../../../../winsup/cygwin/fhandler_console.cc:67:3: error: macro expands to
multiple statements [-Werror=multistatement-macros]
67 | wpbuf[wpixput++] = x; \
| ^
../../../../winsup/cygwin/fhandler_console.cc:67:3: note: in definition of
macro 'wpbuf_put'
67 | wpbuf[wpixput++] = x; \
| ^
../../../../winsup/cygwin/fhandler_console.cc:2993:8: note: some parts of macro
expansion are not guarded by this 'else' clause
2993 |else
|^~~~
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.
