Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Takashi Yano]

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.

[Takashi Yano]

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.

[Hans-Bernhard Bröker]

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;
>}