Pcsx2: GSdx: VS2013 generates bad code for _xgetbv(0)

[turtleli]

AVX2 builds of GSdx compiled with VS2013 crash on my system when I use the SW renderer. I bisected the crash and found it was introduced by commit 0f5529be18eb0e93cb951925d4e793f5192b8f73, but that made no sense to me.

I looked at the disassembly and found that the generated code was incorrect - it didn't touch the eax register at all after the xgetbv instruction. However, changing the xbyak code from:

uint64 bv = getXfeature();
if ((bv & 6) == 6) {

to

if ((_xgetbv(0) & 6) == 6) {

so that _xgetbv(0) is directly used fixes the code generation issue. Fun.

I'm making a note of this instead of fixing it. One other thing that we've used that VS2013 doesn't support is %z length modifiers in printfs. I'll leave fixing this stuff to someone who wants to keep VS2013 support.

[mirh]

Update 5?

[turtleli]

I was using update 5.

[mirh]

Mhh.. and seems like microsoft stopped to care about it long time ago.

[gregory38]

Tbh I was surprised that %z compile. Which is dumb for a runtime feature. Anyway what is the status of VS2017 ? They released the RC recently but I didn't find any info on the final version.

[mirh]

https://connect.microsoft.com/VisualStudio/feedback/details/806338/vc-printf-and-scanf-should-support-z
https://connect.microsoft.com/VisualStudio/feedback/details/2083820/printf-format-specifier-zu-is-supported-but-not-documented

If %zu is comparable, then they kind of support this already in VS15. Otherwise no news in the foreseeable future.

[gregory38]

AFAIU, the issue is VS2013. We said in the past that we will try to support at least the 2 latest versions of VS. It seems VS2017 will be released soon. I'm not sure, it worth to bother with VS2013. Besides based on their own developers, it is likely far of the C++ standard

• https://www.reddit.com/r/cpp/comments/5dai1i/vs_2017_rc_is_now_available/da30hxd/

@refractionpcsx2 any opinion on the matter ?

[avih]

As this is easy to fix (and the %z is probably less critical), why not just fix it?

[gregory38]

We could. Or we could drop it. (as Linux dev, I won't chose for the Win dev)

As a side note, it is not limited to those issues. From time to time some PRs compile fine on VS2015 but fail on VS2013 (because the latter doesn't fully support C+11).

[refractionpcsx2]

@refractionpcsx2 any opinion on the matter ?

Drop 2013 as soon as we have a 2017 project ;) Seriously though, only the appveyor builds use 2013, the official releases are done with 2015 and appveyor also does 2015 builds, so technically we could just stop those 2013 versions being built. If people want to build it themselves in 2013, that's up to them, but soon support for it will be dropped.

There's little point in wasting time on it or potentially reducing performance for the sake of something we aren't going to be using much longer.

If however the fix is that easy, takes 2 seconds to do and doesn't have any further impact, then i don't see why not.

[mirh]

We said in the past that we will try to support at least the 2 latest versions of VS.

I remember having heard about this, but to be honest it seems dumb.
If it doesn't pull back the project.. What's the problem?
I don't see why one shouldn't support vs2008 (in a world where it hadn't downsides)
In this case, there starts to be a burden (and unjustified by any particular gain, like say #998) but the fix seems something so trivial.

[refractionpcsx2]

There is a good reason not to support old versions and right now that's the use of new C++ standards which aren't available in the older versions of Visual Studio (C++13 is it?)

[gregory38]

It is C++11 which is now 6 year old ;)

C++14 is out but it requires GCC5 on Linux (and latest Debian stable is 4.9). That being said some useful feature are available on 4.9. However I guess it requires a recent Visual studio. The most interesting feature for me is Single-quotation-mark as a digit separator so you can write 0x100'0001

C++17 is the future and not out yet.

[gregory38]

@mirh http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0061r1.html (C++2017)

[refractionpcsx2]

It is C++11 which is now 6 year old ;)

yeah exactly, it wouldn't work on VS2010, at least a vast majority of it doesn't looking at microsofts support list, even 2012 is sketchy at best https://msdn.microsoft.com/en-us/library/hh567368(v=vs.110).aspx

So in this case, supporting anything older than 2013 would be silly and we may only continue to use newer features, so that would them obsolete older versions.

admittedly these days it is less of a problem as we have one solution and one project file (for each project of course), unlike when we started doing this and we had a separate solution and project files for each version of visual studio, that was a nightmare to maintain :P

[mirh]

There is a good reason not to support old versions and right now that's the use of new C++ standards which aren't available in the older versions of Visual Studio (C++13 is it?)

Well, you can't even say they are managed to fully support C99 by now.. But more or less, let's say C++11 is implemented for the most part starting in VS2013, yes.
C++14 is lackluster even in VS2015 then.
Anyway, mine was just a hypothetical, obviously.

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0061r1.html (C++2017)

Lol.
I can't really understand if they already implemented it or not though.
P.S: you are writing too faaast

[refractionpcsx2]

He mentions SDL 2.0 in that post, that defo works in 2015 as I've used it on my Chip16 emulator :P

[gregory38]

VS2013 isn't fully compliant with C++11. Mostly but not fully. It isn't critical but it is annoying to dev some valid codes to find that VS2013 didn't manage to compile it.

C++14 isn't that bad in VS2015. You have 50%. For me the really useful extension is the digit separators. It can greatly help to read big number.

As a side note, VS2013 has been broken since 22nd November and nobody notice it.

[avih]

As a side note, VS2013 has been broken since 22nd November and nobody notice it.

To notice it one would have to build using vs2013 AND use the AVX2 GSdx. When I build (using VS2013) I typically use DEVEL config to debug, which is SSE2.

[gregory38]

Sure. But the question on test coverage of the VS2013 build is still legitimate.

[avih]

Every compiler on every platform will always have some bugs and will never fully support the latest standard available. We've settled on c++11 for now, vs2013 mostly supports it, and the fix for the OP issue is trivial. Let's just fix it.

If it becomes harder to support it due to extensive use of unsupported features, sure, it'd get dropped, but as long as it's easy, it'd be nicer if it would still be supported.

[mirh]

^
Anyway, even if compilers haven't full standard support, this ' thingy seems basically supported everywhere.
And.. depending on how much gregory care, that might count so much to be a the reason to drop vs2013.

[turtleli]

Personally I'd vote to drop VS2013 immediately:

• The fix may be trivial, but debugging and finding the fix was still time consuming. I only noticed there was a fix that didn't require inline assembly after realising the stuff in x86emitter was somehow detecting AVX properly. And honestly I went quite out of my way to find the fix - I could have just stopped looking after I figured out the issue and saved myself the time.
• Most people just use the VS2015 builds, and now that the Skylake issue seems fixed, I don't really see a reason for supporting VS2013 since the community editions of Visual Studio are free for Open Source projects anyway.
• I'd rather not tell any random contributor who opens a PR that only fails to compile with VS2013 to fix the VS2013 build that barely anyone uses anyway. IMO it's an unnecessary restriction.

I remember having heard about this, but to be honest it seems dumb.
If it doesn't pull back the project.. What's the problem?
I don't see why one shouldn't support vs2008 (in a world where it hadn't downsides)

Dev/tester coverage/workload. Someone actually has to able to test/fix things to make sure it compiles AND actually works.

[mirh]

Mhh, nobody has to if the alternative is just dropping it when it breaks and nobody cares anymore.
The only difference I see between what we are saying is that I'd wait for that moment if any, while you prefer to anticipate.

Kudos for the remainder insights then.

[gregory38]

 And.. depending on how much gregory care, that might count so much to be a the reason to drop vs2013.

I don't care. Just saying that you can't hold down the progress. C++11 was a huge improvement for the C++ community. I'm not the guy that rush on the new hype but I think we have now good compiler supports everywhere. C+11 already brings us major improvement on code quality.

Unfortunately VS2013 still miss several big features such as constexpr, noexcept. (note I don't say we need them). I'm reluctant to spend time to add C++11 improvement because I fear that I will need to redo my code (even if it will work 75% of the time).

[Orphis]

Technically, the last 2 updates of VS are really 2015u2 and 2015u3.

Keeping 2013 around is just holding back projects for no reason. You should rather say that you're supporting the VS release from last year, which gives 1 year for everyone to upgrade to the latest, which isn't unreasonable (or 6 months or whatever).

[gregory38]

Well you know, some people were traumatized by XP during theirs childhoods ;)

Anyway, from https://blogs.msdn.microsoft.com/vcblog/2017/02/06/stl-fixes-in-vs-2017-rtm/

February 6, 2017

VS 2017 RTM will be released soon. VS 2017 RC is available now

IIRC, RTM is the final version. So I guess it is matter of a couple of weeks. Maybe one can update the build to support VS2017 (if it isn't already the case). And then we could drop VS2013.

[Orphis]

Still, you should reconsider that arbitrary "last 2 releases" and require VS2015u3 only for now and allow VS2017 when it's released, then fully switch to it.

[mirh]

And you should start to get it up on your server 😛

EDIT: VS2017 launches on March 7

[Orphis]

I have Windows 2008 R2 on the server, it won't run VS2017 :(
I'll try to figure out something though!

[Dokman]

@Orphis it run on Windows Server 2008 R2
https://www.visualstudio.com/en-us/productinfo/vs2017-compatibility-vs

[Orphis]

The other page I looked at said Server 2012 R2 was required. Well, that's good news, but I'll still consider upgrading :)

[mirh]

Mhh.. you are right, but it doesn't make sense for 2008 R2 (=W7) not to be supported

[Orphis]

That's the page I've been looking at, yes. I can give it a try when the RTM is released!

[FlatOutPS2]

The page @Orphis was looking at is the correct one. The page that does mention Server 2008 support only lists operating systems you can develop applications for with VS 2017, it isn't for actually running VS 2017.

Windows Sever 2008 is only supported by the foundation server and additional tools of VS 2017.

[mirh]

Windows Sever 2008

Take notice Server 2008 is not the same of 2008 R2.
Whilst 2008 R2 do is the same of 7, so that's a genuine wtf.

EDIT: it works (I mean, these links are about problems in the first place, but that would be normal even for MS RTM products 😋 )

[FlatOutPS2]

Take notice Server 2008 is not the same of 2008 R2.
Whilst 2008 R2 do is the same of 7, so that's a genuine wtf.

Yes, I meant R2. As it is supported for additional products of VS2017(unlike the regular Server 2008), there's probably some issue that's preventing it from being officially included for the main application. It might still be included in the future(maybe even for the RTM).

[Orphis]

I still plan on upgrading the machine anyway, so it doesn't matter too much!

[gregory38]

Here a summary of the IRC talk

• Port Cmake to Windows, validate it with VS2015.
• Wait VS2017 release. Validate Cmake with it.
• Drop VS2013.
• Drop windows XML files.

[gregory38]

Note: I will add the missing file from cmake. So core + main plugin will be clean. However, 3rdparty + some plugins aren't handled yet by cmake.

[gregory38]

So VS2017 is out. The build seems to be fine. So I propose that we

• replace VS2013 by VS2017 in appveyor build
• drop VS2013 support and close this issue

Cmake support will be nice but it is low priority.

[avih]

drop VS2013 support

I won't argue either way, but I'd like to see VS2013 "passive" support stay (i.e. don't intentionally break the project files for VS2013 etc), with a disclaimer or whatever that it's not officially supported and might be broken.

Once something happens which fully breaks VS2013, let it be (unless someone is interested in maintaining it, of course), but till this happens, I hope it won't be removed intentionally.

[gregory38]

The fact is VS2013 builds are already partially brokken. Some new C++ feature such as PR #1812 might break it further (except if we add extra define). I don't think we have any specific VS2013 compilation file nowadays (but I could be wrong). I won't draw my hammer but I'm afraid it is time to upgrade. What is the issue actually ?

[avih]

Not an issue but rather a preference - I prefer to keep backward compatibility where possible. If it breaks then it breaks, but I'm only saying let's not break it intentionally.

[gregory38]

Not an issue but rather a preference

By any chance did you try VS2017 ? Maybe you will like it.

[avih]

By any chance did you try VS2017 ? Maybe you will like it.

I haven't tried it but I'm sure I'll like it. How's that related?

[FlatOutPS2]

Why wouldn't we want to break it intentionally if it we know it will improve the code?

[refractionpcsx2]

By any chance did you try VS2017 ? Maybe you will like it.

Just grabbing it now myself. Did you know VS2017 has support to compile Linux executables? :)

[gregory38]

Just grabbing it now myself. Did you know VS2017 has support to compile Linux executables? :)

Hum. They integrated the linux console, next step is to include the kernel ;) I think we have a kind of VS support on Linux nowadays.

[Orphis]

What do you think that "Bash" on Windows is? They have reimplemented the Linux kernel on Windows and run a big chunk of userland already!
Why do you need to keep 2013 support for? Nostalgia? If it's holding back people, there's no reason to keep it around. VS2015 Community is free and you've had 2 years to download it.

[refractionpcsx2]

Well VS 2017 Community happily recompiled the entire PCSX2 solution first time, which is good.

Totally off topic but:
Downside is I've found a game which is over 40fps slower on OpenGL with all the fancy enhancements of OpenGL disabled lol, although I suspect it's because DX is skipping some stuff, looks like the GPU is busy AF on OpenGL though. Crash N Burn if you wondered.

[avih]

If it's holding back people, there's no reason to keep it aroun

When did I say to keep it even if it holds back people? I explicitly said that if it breaks due to progress then let it break. The only thing I hoped for is that if it does _not_ hold people back, then don't remove it intentionally.

Please don't put words in my mouth.

[mirh]

Why wouldn't we want to break it intentionally if it we know it will improve the code?

You are talking about improvements breaking it, which is fine and acceptable. He's "complaining" about giving it the middle finger just because two "seems a good number" of supported versions.

[Orphis]

The problem is that if you keep the projects around, you would expect them to work.
If you remove them, you don't have any more expectations and one less thing to worry about.

[avih]

The problem is that if you keep the projects around, you would expect them to work.

That's a problem of expectations management. Which can be solved with a statement "VS2013 might work, but you're on your own, and we won't break it intentionally but it could break accidentally, and we won't put much effort, if at all, to fix it".

[gregory38]

Let's take a PR that uses C++11 features that I know isn't supported by vs2013. What do I do ?

[gregory38]

Ps we don't have vs2013 project files

[FlatOutPS2]

Let's take a PR that uses C++11 features that I know isn't supported by vs2013. What do I do ?

Merge it and be happy. Then we definitely don't need to support the ancient studio anymore.

[mirh]

That's an example of "rightful drop".
But too bad [at least the one I think you are thiking] had OP already thinking of that and caring to include the right ifdefs 😛

[avih]

Let's take a PR that uses C++11 features that I know isn't supported by vs2013. What do I do ?

That's quite literally dropping VS2013 intentionally. If you think that PR is great for the code, take it and feel free to ignore VS2013. But please don't take it to intentionally break VS2013.

[ssakash]

@avih

"VS2013 might work, but you're on your own, and we won't break it intentionally but it could break accidentally, and we won't put much effort, if at all, to fix it".

Since we are dropping VS2013 build configuration from Appveyor, how would a Linux developer like Gregory know when his commits fail to build in VS2013? He could just sneak in some C++11 feature on his commits to break VS2013 and no one would notice. :P

Though honestly, I think we won't break it intentionally sounds close to we maintain it but we won't break it for some trivial code improvements, it still has the little burden of maintaining it.

[turtleli]

Can't we just immediately break VS2013 support and remove the compatibility stuff (I spot two instances, one in GSdx, one in PCSX2)? What's the point in keeping support for it?

[FlatOutPS2]

how would a Linux developer like Gregory know when his commits fail to build in VS2013

How would a Windows developer know? I doubt any of us will install VS2013 and just to check if our changes break it. :p

[mirh]

how would a Linux developer like Gregory know when his commits fail to build in VS2013?

He wouldn't and he shouldn't either?

He could just sneak in some C++11 feature on his commits to break VS2013 and no one would notice.

Amen. That was the purpose of the "intentional" adverb I guess.

(If a) it wasn't just avx-specific and b) @turtleli hadn't done such a good analysis up here) this issue would already be a good example of a case for "the drop".
If nobody cares anymore (and fixing doesn't seem worth the hassle), peace and bye bye.

I didn't think "compatibility stuff" was all that burden atm then?

[gregory38]

Yes it is burden. I often pushed a branch to check compilation on VS2013 and redo my code accordingly. C++11 isn't a new thing anymore. C++14 is out, and C++17 is nearly out too.

[danilaml]

Why is there even a need to support VS2013? Everyone can get VS2015/2017 community edition for free to compile the code if they need for some reason. VS2013 has long been obsolete. It's not like pcsx2 is a lib that needs to support as much compiler as possible.

[sudonim1]

Dropping support for 2013 seems fine now for all the reasons stated, is anyone even against this?

[refractionpcsx2]

I'm not against dropping it, users can update to 2017 if there's a problem, it's free and works fine.

[mirh]

Nope, I think we are all fine now (given what gregory said and 5c1023a3328cb433aed27b0069d4274878bd9a50)

EDIT: then indeed.. I guess from dev side there really isn't any point in [tools] retrocompatibility, for its own sake.

[turtleli]

Nope, I think we are all fine now (given what gregory said and 5c1023a)

I don't see what that commit has to do with it except for further highlighting how crappy VS2013 is on some things (hh is in the C99 spec, which is ancient).

Anyways, I'll remove the VS2013 build from AppVeyor shortly. Adding VS2017 to AppVeyor might have to wait slightly - the AppVeyor VS2017 image doesn't contain:

• the Windows 8.1 SDK - we could probably just move to the Windows 10 SDK
• the old DirectX SDK - either we add a few files to the repository (5 header files) or we ask AppVeyor if they can add the DX SDK to the image.

[mirh]

https://github.com/appveyor/ci/issues/1386

[gregory38]

So RIP VS2013.

@avih I'm afraid you will need to enjoy a faster, lighter and more powerful Visual Studio :+1:

[gregory38]

Can this non-issue be closed ?

[lightningterror]

On a side note the wiki page can be updated now. @bositman
http://wiki.pcsx2.net/index.php/PCSX2_Documentation/Compiling_on_Windows