Re: Crash with old Windows on new CPU - Mailing list pgsql-hackers

* Magnus Hagander wrote:

On Sat, Feb 13, 2016 at 4:45 PM, Christian Ullrich <chris@chrullrich.net>
wrote:

On February 13, 2016 4:10:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm also suspicious of the "#if _MSC_VER == 1800" tests, that is,
the code compiles on *exactly one* MSVC version.

The bug exists in only that compiler version's CRT, also, that is not the
complete version number. There may be different builds somewhere, but they
all start with 18.0.

IIRC, the CRT itself does explicit checks against _MSC_VER == 1800. As in,
they don't actually bump that number in different build numbers.

How does this work wrt mingw, though? Do we have the same problem there?
AIUI this code can never run on mingw, correct?

Not unless mingw defines _MSC_VER.

(If they do, I suggest we make Yury Zhuravlev cry and drop MinGW support instead. IMHO everyone should boycott them anyway until they come up with a working libc of their own instead of doing unspeakable things to a helpless msvcrt.dll that is not intended for use by non-system components at all. But I digress.)

I notice the code checks IsWindows7SP1OrGreater() but the comment refers to
W7SP1 *or* 2008R2 SP1. I assume this is correct, or should there actually
be a separate check for server-windows?

No, that is fine. I think it's just to keep the function name from getting too ridiculously long. The functions in <versionhelpers.h> are all named for the client versions only, and only check the version number, not the client/server capability flags. Or, rather, there is a separate function to determine that.

That would
give us some context to estimate the risks of this code executing
when it's not really needed.

Hence all the conditions. The problem is *certain* to occur under these
specific conditions (x64 code on Windows before 7SP1 on a CPU with AVX2
when built with VS2013), and under no others, and these conditions flip the
switch exactly then.

Well, it doesn't flip it based on the specifics "running on a CPU with
AVX2". But presumably turning of AVX2 if the CPU doesn't support it is a
no-op.

Precisely.

Isn't that what the buildfarm is (among other things) for?

The buildfarm doesn't really have a big spread of Windows animals,
unfortunately.

And apparently not a single one with VS 2013. OK, I'll see what I can do about setting some up soonish, at least with (server) 2008 and (client) 7. FWIW, I have a local build of 9.5.1 with this patch in daily use on 2008 now, with no complaints.