Re: Crash with old Windows on new CPU

On Wed, Mar 9, 2016 at 4:36 PM, Christian Ullrich <chris(at)chrullrich(dot)net>
wrote:

> * Magnus Hagander wrote:
>
> On Sat, Feb 13, 2016 at 4:45 PM, Christian Ullrich <chris(at)chrullrich(dot)net>
>> wrote:
>>
>
> On February 13, 2016 4:10:34 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.
>

The question is then - should it, under some conditions?

> (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.)

Haha, well, at least they're not cygwin...

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

Presumably the link is documented somewhere (the docs don't seem to say
anything about it).

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

Having that added to the buildfarm would definitely be very useful!

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/