* 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.
--
Christian
