Re: [PATCH] mingw: make stderr unbuffered again

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 15, 2017, 12:32 UTC
Message-ID: <alpine.DEB.2.20.1702151312330.3496@virtualbox>
In-Reply-To: <ef8549ea-7222-fdd0-739d-855ad428e39c@kdbg.org>

Hi Hannes,

On Tue, 14 Feb 2017, Johannes Sixt wrote:

Show 17 quoted lines

> Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:
> > On Mon, 13 Feb 2017, Junio C Hamano wrote:
> > > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> > > > When removing the hack for isatty(), we actually removed more than
> > > > just an isatty() hack: we removed the hack where internal data
> > > > structures of the MSVC runtime are modified in order to redirect
> > > > stdout/stderr.
> > > >
> > > > Instead of using that hack (that does not work with newer versions
> > > > of the runtime, anyway), we replaced it by reopening the
> > > > respective file descriptors.
> > > >
> > > > What we forgot was to mark stderr as unbuffered again.
> 
> I do not see how the earlier patch turned stderr from unbuffered to
> buffered, as it did not add or remove any setvbuf() call. Can you
> explain?

Certainly. I hoped that the commit message would do the job, but then, I am under time pressure these days, so it was probably a bit terse.

The hack we used to make isatty() work used to change data structures directly that are *internal* to the MSVC runtime. That is, instead of *really* redirecting stdout/stderr, we simply changed the target of the existing stdout/stderr and thereby could fool MSVC into believing that it is *still* writing to the terminal (because the bit is set) and that it is *not* a pipe (because we forcibly unset that bit).

Needless to say, this meddling with internal data structures is not only prone to break with future updates of the MSVC runtime, it is also inappropriate because the implementation may rely on certain side effects (or not so side effects) that may very well cause crashes or data loss. Imagine, for example, that the internal data structure were variable-size, based on the HANDLE type. That is totally legitimate for an internal data structure. And if we meddle with the HANDLE, we can easily cause data corruption.

As GCC is basically tied to using an older version of the MSVC runtime (unlike GLIBC, multiple versions of the MSVC runtime can coexist happily, making it relatively easy to maintain backwards-compatibility, but that concept is foreign to GCC), this used to not be a problem.

However, with our recent push to allow developing, building, debugging and performance profiling in Visual Studio, that limitation no longer holds true: if you develop with Visual Studio 2015, you will link to a newer MSVC runtime, and that runtime changed those internal data structures rather dramatically.

That means that we had to come up with a non-hacky way to redirect stdout/stderr (e.g. to parse ESC color sequences and apply them to the Win32 Console as appropriate) and still have isatty() report what we want it to report. That is, if we redirect stdout/stderr to a pipe that parses the color sequences for use in the Win32 Console, we want isatty() to report that we are writing to a Terminal Typewriter (a charming anachronism, isn't it?).

My colleague Jeff Hostetler worked on this and figured out that the only way to do this properly is to wrap isatty() and override it via a #define, and simply remember when stdout/stderr referred to a terminal before redirecting, say, to that pipe.

As my famously broken patch to override isatty() (so that /dev/null would not be treated as a terminal) *already* overrode isatty() with a custom wrapper, and as it was broken and needed fixing, I decided to reconcile the two approaches into what is now the version in `master`.

So instead of "bending" the target HANDLE of the existing stdout/stderr (which would *naturally* have kept the buffered/unbuffered nature as-is), we now redirect with correct API calls. And the patch I provided at the bottom of this mail thread reinstates the unbuffered nature of stderr now that it gets reopened.

Hopefully that makes it clear why the setvbuf() call is required now, but was previously unnecessary?

Ciao, Dscho