On Wed, Mar 9, 2016 at 4:36 PM, Christian Ullrich <ch...@chrullrich.net> wrote:
> * Magnus Hagander wrote: > > On Sat, Feb 13, 2016 at 4:45 PM, Christian Ullrich <ch...@chrullrich.net> >> wrote: >> > > On February 13, 2016 4:10:34 PM Tom Lane <t...@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. > 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/
