1119072 - Update build system to support building with Visual Studio 2015 (VC2015)

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

It's good to change the build system early to support building with VS2015, so that people can start experimenting with it. In particular, it would be good to get Gecko building with VS2015 before it RTMs so that any compiler bugs found have a higher chance of getting fixed.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 1: Stop rejecting VS2015 during configure

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: NOT FOR CHECKIN: Temporarily disable STL wrappers

Without this patch, when VS2015 comnpiles the <exception> header, it fails to recognize the _THROW0() macro, causing a syntax error. The strange thing is that there is a "#inclue <xstddef>" in <exception> which should cause _THROW0() to always be defined. It may be a bug in VS2015's "#pragma once" support.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8545584 [details] [diff] [review]
Part 1: Stop rejecting VS2015 during configure

Review of attachment 8545584 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +492,5 @@
> +        elif test "$_CC_MAJOR_VERSION" = "19"; then
> +            _CC_SUITE=14
> +            MSVS_VERSION=2015
> +            MSVC_C_RUNTIME_DLL=msvcr140.dll
> +            MSVC_CXX_RUNTIME_DLL=msvcp140.dll

Did they really skip 13 like buildings wouldn't have a 13th floor, or were they attempting to sync with the year, but failed to release in 2014?

::: js/src/configure.in
@@ +422,5 @@
> +        elif test "$_CC_MAJOR_VERSION" = "19"; then
> +            _CC_SUITE=14
> +            MSVS_VERSION=2015
> +            MSVC_C_RUNTIME_DLL=msvcr140.dll
> +            MSVC_CXX_RUNTIME_DLL=msvcp140.dll

only _CC_SUITE is needed in js/src/configure.in.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2: Disable warnings added after VS2013

I'm not sure if js/src/configure.in needs something similar to this or not. Without this, there are a lot of warnings, which become errors for people with warnings-as-errors set, due to variable shadowing.

It seems VS2013 doesn't support -Wv, and I didn't want to duplicate the VS version conditional logic, which is why this isn't with all the "-Wd" stuff elsewhere in configure.in.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 3: Paper over the use of the deprecated-in-VS2015 hash_set & hash_map headers

See also https://code.google.com/p/chromium/issues/detail?id=350104

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 3: Silence warning about deprecated use of hash_set and hash_map in Chromium code [v2]

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 4: Disable warning 4091 in VS2015 builds to work around VS2015 incompatibility with Platform SDK 8.1

From http://blogs.msdn.com/b/vcblog/archive/2014/06/06/c-14-stl-features-fixes-and-breaking-changes-in-visual-studio-14-ctp1.aspx:

"The Standard has always forbidden containers of const elements (e.g. vector<const T>, set<const T>).  (C++98/03's prohibition was crystal clear: elements must be Assignable, which const T isn't.  C++11/14's prohibition is obscurely hidden, but it's there.)  Previously, VC accepted such containers due to non-Standard machinery in std::allocator.  We've removed that machinery, so such containers now fail to compile."

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #7)
> Created attachment 8545764 [details] [diff] [review]
> Part 4: Disable warning 4091 in VS2015 builds to work around VS2015
> incompatibility with Platform SDK 8.1
> 
> From
> http://blogs.msdn.com/b/vcblog/archive/2014/06/06/c-14-stl-features-fixes-
> and-breaking-changes-in-visual-studio-14-ctp1.aspx:

Sorry, that's the wrong link. The correct link is in the patch: https://connect.microsoft.com/VisualStudio/feedback/details/888527/warnings-on-dbghelp-h

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 5: Don't redefine snprintf in VS2015 builds since VS2015 finally has it

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 6: Change a non-conforming usage of a const value type to a non-const value type, which VS2015 rightly rejects

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: NOT FOR CHECKIN: Temporarily disable exception handling override logic

This patch is better than the one I wrote in comment 2. If you're not working on the specific issue of the build failure in <exception> due to _THROW0() not being defined, then I recommend that you apply this patch so that you can skip over that error and concentrate on the problem you're trying to work on.

The patch in comment 2 has a lot of negative side effects as far as the build is concerned, because it causes us to not wrap the #define of NOMINMAX around the Windows headers, which causes uses of std::min & std::max to fail.

This version keeps the STL wrappers mechanism intact, but disables the overriding of _Raise.

I noticed that the logic for replacing _Raise includes <xstddef> and <exception>, and the exact problem we're having is that <exception> seems to not always recognize the macros defined in <xstddef>. So, I think it is likely that the overriding of _Raise needs to be redone for VS2015. But, that's beyond my current knowledge level. Help is appreciated.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: NOT FOR CHECKIN: Work around bug 1119080 by doing something VERY WRONG

The problem is that the compiler wants to use the copy constructor instead of the move constructor, and the copy constructor has been deleted using MOZ_DELETE. This patch works around that by WRONGLY defining that copy constructor so that the code can continue to compile while we invstigate the proper fix in bug 1119080.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Build log showing current progress

The build now goes strong for about 15 minutes before failing with an internal compiler error. See the attached build log.

Comment 14

[Mike Hommey [:glandium]]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #13)
> Created attachment 8545774 [details]
> Build log showing current progress
> 
> The build now goes strong for about 15 minutes before failing with an
> internal compiler error. See the attached build log.

Report it (follow the instructions cl.exe writes)

Comment 15

[(not currently active) Ted Mielczarek]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #1)
> Created attachment 8545584 [details] [diff] [review]
> Part 1: Stop rejecting VS2015 during configure

Just FYI this patch is going to conflict with the patch in bug 1117900.

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8545605 [details] [diff] [review]
Part 2: Disable warnings added after VS2013

Review of attachment 8545605 [details] [diff] [review]:
-----------------------------------------------------------------

Could you copy/paste in the bug the doc about /Wv if there is one in msvc itself, because it's not on MSDN yet, and reflag me then?

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 8545760 [details] [diff] [review]
Part 3: Silence warning about deprecated use of hash_set and hash_map in Chromium code [v2]

Review of attachment 8545760 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe we should take https://chromium.googlesource.com/chromium/src/base/+/8d4ef52e0f9cf444de637e650dbcedfe70f9aece instead. Anyways, I'm not a peer of that code (and we really should dedup the chromium stuff).

Comment 18

[(Away)]

Comment on attachment 8545584 [details] [diff] [review]
Part 1: Stop rejecting VS2015 during configure

Review of attachment 8545584 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +491,5 @@
>              MSVC_CXX_RUNTIME_DLL=msvcp120.dll
> +        elif test "$_CC_MAJOR_VERSION" = "19"; then
> +            _CC_SUITE=14
> +            MSVS_VERSION=2015
> +            MSVC_C_RUNTIME_DLL=msvcr140.dll

Does this file actually exist? I haven't kept up for the last few weeks, but last I checked they were going to change the naming scheme. http://blogs.msdn.com/b/vcblog/archive/2014/06/10/the-great-crt-refactoring.aspx

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2: Disable warnings added after VS2013 [v2]

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Major [:dmajor] (UTC+13) from comment #18)
> Comment on attachment 8545584 [details] [diff] [review]
> Part 1: Stop rejecting VS2015 during configure
> 
> Review of attachment 8545584 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: configure.in
> @@ +491,5 @@
> >              MSVC_CXX_RUNTIME_DLL=msvcp120.dll
> > +        elif test "$_CC_MAJOR_VERSION" = "19"; then
> > +            _CC_SUITE=14
> > +            MSVS_VERSION=2015
> > +            MSVC_C_RUNTIME_DLL=msvcr140.dll
> 
> Does this file actually exist? I haven't kept up for the last few weeks, but
> last I checked they were going to change the naming scheme.
> http://blogs.msdn.com/b/vcblog/archive/2014/06/10/the-great-crt-refactoring.
> aspx

Yes, I'm aware of that. It seems like there will need to be more changes to the build system in order to deal with the fact that the standard library is split into multiple pieces in VS2015. However, it may be a while before we even get to the point where we can test the linking stuff, since there are many, many patches needed to get it to just build. So, I suggest that for now we just stick with what I wrote.

Comment 21

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8545765 [details] [diff] [review]
Part 5:  Don't redefine snprintf in VS2015 builds since VS2015 finally has it

I will do the snprintf changes in a separate bug, because there are many patches to review.

Comment 22

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 5: Work around problem with cmath and M_PI on VS2015

Comment 23

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 7: Work around internal compiler error in Skia

Comment 24

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 8: Work around internal compiler error in VS2015 (libstagefright)

Comment 25

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 9: Supress new VS2015 warnings in js/src/configure.in

Comment 26

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 10: Work around internal compiler error in VS2015 (js)

Comment 27

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 11: Do not conflate struct _stat with struct stat in MSVC 2015

Comment 28

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 12: Explicitly cast char16_t to wchar_t in dom/media/gmp/GMPChild.cpp

Comment 29

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 13: #undef NOMINMAX in testing/tools/screenshot/win32-screenshot.cpp so it compiles with MSVC 2015

Comment 30

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 14: Fix prototype of MakeCommandLine to match definition

Comment 31

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 15: Replace uses of hash_map with unordered_map (toolkit)

Comment 32

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 16: Replace uses of hash_set with unodered_set (gfx)

Comment 33

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 17: Use vcruntime140.dll, not msvcr140.dll

Comment 34

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: NOT FOR CHECKIN: Temporarily disable exception handling override logic [v3]

Comment 35

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Full clobber build log showing current progress [v2]

> 23:00.37 We know it took a while, but your build finally finished successfully!
> To view resource usage of the build, run |mach resource-usage|.
> To take your build for a test drive, run: |mach run|

mach gtest "pkix*" && mach xpshell-test security/manager/ssl/tests/unit both pass.

mach run starts the browser and it appears to work correctly, though I barely tested it.

Comment 36

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Here's a try run with all these patches added, except the ones marked NOT FOR CHECKIN: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2392be3e231

This shows I didn't break any existing builds with these changes. I'm not sure what's up with Android Mochtest-8; I think my local tree might have been based on a changeset that broke that. I'll verify that everything is OK before I land any of this.

Comment 37

[Gregory Szorc [:gps]]

Comment on attachment 8546580 [details] [diff] [review]
Part 11: Do not conflate struct _stat with struct stat in MSVC 2015

As harmless as this seems, I don't have C++ review power.

Comment 38

[Gregory Szorc [:gps]]

Comment on attachment 8546584 [details] [diff] [review]
Part 15: Replace uses of hash_map with unordered_map (toolkit)

Review of attachment 8546584 [details] [diff] [review]:
-----------------------------------------------------------------

ditto

Comment 39

[Bob Owen (:bobowen)]

Comment on attachment 8545766 [details] [diff] [review]
Part 6: Change a non-conforming usage of a const value type to a non-const value type, which VS2015 rightly rejects

Review of attachment 8545766 [details] [diff] [review]:
-----------------------------------------------------------------

Would you add a line to security\sandbox\modifications-to-chromium-to-reapply-after-upstream-merge.txt, so that we can keep track of this, please.

Chromium already has https://code.google.com/p/chromium/issues/detail?id=360799 filed for this.
Comment #3 for it, suggests they were going to fix it, but for some reason they haven't.

Comment 40

[Mike Hommey [:glandium]]

Comment on attachment 8546587 [details] [diff] [review]
Part 17: Use vcruntime140.dll, not msvcr140.dll

Review of attachment 8546587 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +492,5 @@
>              MSVC_CXX_RUNTIME_DLL=msvcp120.dll
>          elif test "$_CC_MAJOR_VERSION" = "19"; then
>              _CC_SUITE=14
>              MSVS_VERSION=2015
> +            MSVC_C_RUNTIME_DLL=vcruntime140.dll

Can you not do that and fold all the configure patches together?

Comment 41

[Jeff Walden [:Waldo]]

Comment on attachment 8546579 [details] [diff] [review]
Part 10: Work around internal compiler error in VS2015 (js)

Review of attachment 8546579 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/LIR.cpp
@@ +111,5 @@
>                  return false;
>  
> +            // MSVC 2015 cannot handle "new (&phis_[phiIndex++])"
> +            LPhi *lphi = new (&phis_[phiIndex]) LPhi(phi, inputs);
> +            ++phiIndex;

I'd prefer

  void *addr = &phis_[phiIndex];
  phiIndex++;
  LPhi *lphi = new (addr) LPhi(phi, inputs);

if it works -- with the first two lines combined if that works.  The new-expression there is awfully noisy, and just removing the increment doesn't do enough to fix that in my book.

Comment 42

[Robert Strong (they/them - no direct email)]

Comment on attachment 8546580 [details] [diff] [review]
Part 11: Do not conflate struct _stat with struct stat in MSVC 2015

Why did you go with NS_tstat_t vs. NS_tstat?

Comment 43

[Robert Strong (they/them - no direct email)]

for comment #42

Comment 44

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8545760 [details] [diff] [review]
Part 3: Silence warning about deprecated use of hash_set and hash_map in Chromium code [v2]

Review of attachment 8545760 [details] [diff] [review]:
-----------------------------------------------------------------

We're recording local changes in security/sandbox/modifications-to-chromium-to-reapply-after-upstream-merge.txt but this looks fine aside from that; r=me with that change.

FYI, there's an update to all of security/sandbox/chromium coming up relatively soon in bug 1102195, and in general our goal going forward is to track upstream reasonably closely.  f? → Bob Owen, who's been working on that bug (and who knows Windows better than I do).

Comment 45

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546587 [details] [diff] [review]
Part 17: Use vcruntime140.dll, not msvcr140.dll

Review of attachment 8546587 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +492,5 @@
>              MSVC_CXX_RUNTIME_DLL=msvcp120.dll
>          elif test "$_CC_MAJOR_VERSION" = "19"; then
>              _CC_SUITE=14
>              MSVS_VERSION=2015
> +            MSVC_C_RUNTIME_DLL=vcruntime140.dll

I am happy to fold all the configure patches together. However, I am not sure what you mean by "Can you not do that"? Is there anything else you want me to do other than fold the patches together? (Sorry to be dens; I've had the flu for the last week so my brain isn't working so well.)

Comment 46

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #42)
> Comment on attachment 8546580 [details] [diff] [review]
> Part 11: Do not conflate struct _stat with struct stat in MSVC 2015
> 
> Why did you go with NS_tstat_t vs. NS_tstat?

On Windows, NS_tstat is already defined to be something different:

	# define NS_tstat _wstat
        [...]
      + # define NS_tstat_t _stat

Comment 47

[Xidorn Quan [:xidorn] UTC+11]

I guess we probably should change the alias of this bug to VC14 instead as "13" seems to have been skipped.

Comment 48

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 3: Silence warning about deprecated use of hash_set and hash_map in Chromium code [v3]

(In reply to Mike Hommey [:glandium] from comment #17)
> Comment on attachment 8545760 [details] [diff] [review]
> Part 3: Silence warning about deprecated use of hash_set and hash_map in
> Chromium code [v2]
> 
> Review of attachment 8545760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Maybe we should take
> https://chromium.googlesource.com/chromium/src/base/+/
> 8d4ef52e0f9cf444de637e650dbcedfe70f9aece instead.

I think my original patch is a lower-risk change, and so I'd like to keep it as-is. My understanding is that Bob Own will soon resync with chromium upstream, and we'll pick up the upstream changes when that happens.

(In reply to Jed Davis [:jld] from comment #44)
> We're recording local changes in
> security/sandbox/modifications-to-chromium-to-reapply-after-upstream-merge.
> txt but this looks fine aside from that; r=me with that change.

Thanks! I added a line to that file.

Comment 49

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8547121 [details] [diff] [review]
Part 3: Silence warning about deprecated use of hash_set and hash_map in Chromium code [v3]

jld already r+d the security/sandbox change. Could you please review the (same) ipc/ part? Thanks!

Comment 50

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 6: Change a non-conforming usage of a const value type to a non-const value type, which VS2015 rightly rejects [v2]

r=bobowencode. I added the requested change to modifications-to-chromium-to-reapply-after-upstream-merge.txt

Comment 51

[Bob Owen (:bobowen)]

Comment on attachment 8547121 [details] [diff] [review]
Part 3: Silence warning about deprecated use of hash_set and hash_map in Chromium code [v3]

Review of attachment 8547121 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #48)

> I think my original patch is a lower-risk change, and so I'd like to keep it
> as-is. My understanding is that Bob Own will soon resync with chromium
> upstream, and we'll pick up the upstream changes when that happens.

Yeah, I hope to at least start that this week and as the Chromium change was 24th Oct, I should pick that one up.
You'd hope that recent changes wouldn't regress compiling with VC2015, but I'll test with it as well to try and make sure.

I won't update the ipc branch as part of the resync as currently that way madness lies.
So, it might have been better to have these as two separate patches, but given that hopefully the change to the security/sandbox code won't be needed soon, I don't think it matters much.

> (In reply to Jed Davis [:jld] from comment #44)
> > We're recording local changes in
> > security/sandbox/modifications-to-chromium-to-reapply-after-upstream-merge.
> > txt but this looks fine aside from that; r=me with that change.
> 
> Thanks! I added a line to that file.

Thanks for updating the changeset on the previous line as well.

Comment 52

[Benjamin Smedberg]

Comment on attachment 8546584 [details] [diff] [review]
Part 15: Replace uses of hash_map with unordered_map (toolkit)

This needs to go upstream in google-breakpad. Resetting review to Ted who typically deals with that.

Comment 53

[Robert Strong (they/them - no direct email)]

Comment on attachment 8546580 [details] [diff] [review]
Part 11: Do not conflate struct _stat with struct stat in MSVC 2015

>diff --git a/toolkit/mozapps/update/common/updatedefines.h b/toolkit/mozapps/update/common/updatedefines.h
>--- a/toolkit/mozapps/update/common/updatedefines.h
>+++ b/toolkit/mozapps/update/common/updatedefines.h
>@@ -33,17 +33,19 @@
> # define W_OK 02
> # define R_OK 04
> # define S_ISDIR(s) (((s) & _S_IFMT) == _S_IFDIR)
> # define S_ISREG(s) (((s) & _S_IFMT) == _S_IFREG)
> 
> # define access _access
> 
> # define putenv _putenv
>-# define stat _stat
>+# if _MSC_VER < 1900
>+#  define stat _stat
>+# endif
Just go with this...

>diff --git a/toolkit/mozapps/update/tests/TestAUSHelper.cpp b/toolkit/mozapps/update/tests/TestAUSHelper.cpp
>--- a/toolkit/mozapps/update/tests/TestAUSHelper.cpp
>+++ b/toolkit/mozapps/update/tests/TestAUSHelper.cpp
>@@ -9,17 +9,19 @@
> # include <softpub.h>
> # include <direct.h>
> # include <io.h>
>   typedef WCHAR NS_tchar;
> # define NS_main wmain
> # define F_OK 00
> # define W_OK 02
> # define R_OK 04
>-# define stat _stat
>+# if _MSC_VER < 1900
>+#  define stat _stat
>+# endif
and this

r=me with that

Comment 54

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546580 [details] [diff] [review]
Part 11: Do not conflate struct _stat with struct stat in MSVC 2015

Review of attachment 8546580 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Robert,

Thanks for your review. Unfortunately, I think there may be some misunderstanding. My understanding of your last comment is that I should strip out some parts of the patch. But, *all* the parts really are needed. Otherwise, we get errors like:

   " 'int _wstat64i32(const wchar_t *,_stat64i32 *)': cannot convert argument 2 from 'stat *' to '_stat64i32 *' "

Am I misunderstanding what you meant?

If you don't like the name I chose for the new macro, I'm happy to switch the naming to something else that works, if you can suggest a better name.

Thanks!

Comment 55

[[e-mail]]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #1)
> Created attachment 8545584 [details] [diff] [review]
> Part 1: Stop rejecting VS2015 during configure

appcrt140.dll and desktopcrt140.dll are needed, at least in my local builds.

Comment 56

[[e-mail]]

Attachment: use ls use ls instead s in format string

Comment 57

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8548000 [details] [diff] [review]
use ls use ls instead s in format string

Review of attachment 8548000 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/nsPluginsDirWin.cpp
@@ +71,4 @@
>  {
>    WCHAR keybuf[64]; // plenty for the template below, with the longest key
>                      // we use (currently "FileDescription")
> +  const WCHAR keyFormat[] = L"\\StringFileInfo\\%04X%04X\\%ls";

Thanks for the patch. Could you please explain why this change is necessary? Is VS2015 documented to work differently than earlier versions here? Or, is the code just wrong in general?

Comment 58

[[e-mail]]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #57)

vc14 works differently than earlier versions.

Comment 59

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8548000 [details] [diff] [review]
use ls use ls instead s in format string

Thanks for pointing out this issue. I think it deserves to have its own bug. Could you please file a new one about it, describing what VC14 does differently than earlier versions? In particular, I suspect that there are other places in the code that likely need to be changed if VS2015's change in semantics is intentional.

Also, could you file a bug at https://connect.microsoft.com in the Visual Studio 2015 component? I doubt they intend to change the printf semantics in this way. If you don't have a Connect account, let me know and I will file it for you.

Thanks again!

Comment 60

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546576 [details] [diff] [review]
Part 7: Work around internal compiler error in Skia

Review of attachment 8546576 [details] [diff] [review]:
-----------------------------------------------------------------

Jeff, if you think it's not a good idea to take the patch as a local-only change, then could you also please help me merge this upstream? I don't have commit privileges for Skia upstream. Thanks!

Comment 61

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8545771 [details] [diff] [review]
NOT FOR CHECKIN: Work around bug 1119080 by doing something VERY WRONG

I fixed bug 1119080 with a patch that actually improves the code instead of making it worse, so this workaround is no longer needed. Instead the patch in bug 1119080 should be used.

Comment 62

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Parts 1, 2, 4, 9, 17: Update build system to make MSVC2015 build succeed

OK, this is the last build system patch, AFAICT, and hopefully the last patch for this bug. It combines the existing parts 1, 2, 4, & 9 together as glandium requested (IIUC). It also adds support for packaging desktopcrt140.dll and appcrt140.dll, which I tested by adding this to my mozconfig:

mk_add_options WIN32_REDIST_DIR="/c/Program Files (x86)/Microsoft Visual Studio 14.0/VC/redist/x86/Microsoft.VC140.CRT"

Comment 63

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8546585 [details] [diff] [review]
Part 16: Replace uses of hash_set with unodered_set (gfx)

Review of attachment 8546585 [details] [diff] [review]:
-----------------------------------------------------------------

Bas wrote this.

Comment 64

[Robert Strong (they/them - no direct email)]

Comment on attachment 8546580 [details] [diff] [review]
Part 11: Do not conflate struct _stat with struct stat in MSVC 2015

Sorry about that Brian, I didn't think it through. I'll change the name if I can think of something better in a separate bug. Thanks!

Comment 65

[(no longer active)]

Comment on attachment 8546584 [details] [diff] [review]
Part 15: Replace uses of hash_map with unordered_map (toolkit)

Review of attachment 8546584 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry but this is upstream code that we're not supposed to modify.  Ted is really the right person to talk to here.

Comment 66

[Bas Schouten (:bas.schouten)]

Comment on attachment 8546585 [details] [diff] [review]
Part 16: Replace uses of hash_set with unodered_set (gfx)

Review of attachment 8546585 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/DrawEventRecorder.h
@@ +11,5 @@
>  #include <ostream>
>  #include <fstream>
>  
>  #if defined(_MSC_VER)
> +#include <unordered_set>

As far as I'm concerned using unordered_set here unconditionally is fine too.

Comment 67

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546585 [details] [diff] [review]
Part 16: Replace uses of hash_set with unodered_set (gfx)

Review of attachment 8546585 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the review!

::: gfx/2d/DrawEventRecorder.h
@@ +11,5 @@
>  #include <ostream>
>  #include <fstream>
>  
>  #if defined(_MSC_VER)
> +#include <unordered_set>

Unfortunately, unconditional use of unordered_set here doesn't work because Android and Mac compilers complain about it. (It seems that, of these three files, this is the only one used on Android and Mac.)

Comment 68

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8549106 [details] [diff] [review]
Parts 1, 2, 4, 9, 17: Update build system to make MSVC2015 build succeed

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc297ccecb13

Comment 69

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8547121 [details] [diff] [review]
Part 3: Silence warning about deprecated use of hash_set and hash_map in Chromium code [v3]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbdd48b8513c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e139be9249f2

Bob somebody suggested that I split this patch into two parts so that it would be easier to rebase the security/sandbox changes on top of the next Chromium import, should it be necessary. I did that.

Comment 70

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546569 [details] [diff] [review]
Part 5: Work around problem with cmath and M_PI on VS2015

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b660d44d36a

Comment 71

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8547122 [details] [diff] [review]
Part 6: Change a non-conforming usage of a const value type to a non-const value type, which VS2015 rightly rejects [v2]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c827c112df81

Comment 72

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546577 [details] [diff] [review]
Part 8: Work around internal compiler error in VS2015 (libstagefright)

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3638e6a659

Comment 73

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546579 [details] [diff] [review]
Part 10: Work around internal compiler error in VS2015 (js)

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b34b54164e

I made the suggested changes before checking this in.

Comment 74

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546580 [details] [diff] [review]
Part 11: Do not conflate struct _stat with struct stat in MSVC 2015

https://hg.mozilla.org/integration/mozilla-inbound/rev/916139e297bd

Comment 75

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546581 [details] [diff] [review]
Part 12: Explicitly cast char16_t to wchar_t in dom/media/gmp/GMPChild.cpp

https://hg.mozilla.org/integration/mozilla-inbound/rev/e357409960d7

Comment 76

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546582 [details] [diff] [review]
Part 13: #undef NOMINMAX in testing/tools/screenshot/win32-screenshot.cpp so it compiles with MSVC 2015

https://hg.mozilla.org/integration/mozilla-inbound/rev/63b3bb2a4963

Comment 77

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546583 [details] [diff] [review]
Part 14: Fix prototype of MakeCommandLine to match definition

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cdb06dce2b6

Comment 78

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546585 [details] [diff] [review]
Part 16: Replace uses of hash_set with unodered_set (gfx)

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d99239ff0f0

Comment 79

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546587 [details] [diff] [review]
Part 17: Use vcruntime140.dll, not msvcr140.dll

This was incorporated into the combined build system patch (Parts 1, 2, 4, 9, 17).

Comment 80

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I didn't land parts 7 or 15 because they haven't been reviewed yet. Please let me know what I can do to facilitate those reviews.

I landed the rest of the changes because they will help the other people working on VS2015 compatibility. Besides getting it to build, there are other issues. That further work will be tracked in bug 1119082. I will comment in bug 1119082 about the current issues I am aware of.

Comment 81

[(Away)]

Have you opened Connect issues for the ones that look like compiler bugs (e.g. parts 8 and 10)?

Comment 82

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Major [:dmajor] (UTC+13) from comment #81)
> Have you opened Connect issues for the ones that look like compiler bugs
> (e.g. parts 8 and 10)?

For the internal compiler errors with placement new expressions with the postincrement operator:
https://connect.microsoft.com/VisualStudio/feedback/details/1085821

I did NOT file a connect issue for the sprintf issue in bug 1121290.

Comment 83

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8548000 [details] [diff] [review]
use ls use ls instead s in format string

The issue that this patch addresses is now bug 1121290, and the patch was moved to that bug.

Comment 84

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8549106 [details] [diff] [review]
Parts 1, 2, 4, 9, 17: Update build system to make MSVC2015 build succeed

Backed out this patch because the build broke and I'm assuming this patch is why:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db368edfc832

Comment 85

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I'm moving the VC14 alias to bug 1119082.

Comment 86

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Fix jsshell packaging (fix build bustage caused by part 1,2,4,9,17)

:sigh: I didn't resend this that one single patch to try after cleaning it up to address the review comments, and of course it broke the build. Here are the changes I will fold into that patch to fix the build issues.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7f3ccc8afa8

Comment 87

[(not currently active) Ted Mielczarek]

Comment on attachment 8546584 [details] [diff] [review]
Part 15: Replace uses of hash_map with unordered_map (toolkit)

Review of attachment 8546584 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine, but it needs to go upstream. If you can submit a patch against Breakpad SVN at https://breakpad.appspot.com/ I can review and land it there. Otherwise I can submit it for you but it'll take me a bit (and you might need to remind me).

Comment 88

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc297ccecb13
https://hg.mozilla.org/mozilla-central/rev/fbdd48b8513c
https://hg.mozilla.org/mozilla-central/rev/e139be9249f2
https://hg.mozilla.org/mozilla-central/rev/7b660d44d36a
https://hg.mozilla.org/mozilla-central/rev/c827c112df81
https://hg.mozilla.org/mozilla-central/rev/ce3638e6a659
https://hg.mozilla.org/mozilla-central/rev/b6b34b54164e
https://hg.mozilla.org/mozilla-central/rev/916139e297bd
https://hg.mozilla.org/mozilla-central/rev/e357409960d7
https://hg.mozilla.org/mozilla-central/rev/63b3bb2a4963
https://hg.mozilla.org/mozilla-central/rev/3cdb06dce2b6
https://hg.mozilla.org/mozilla-central/rev/4d99239ff0f0
https://hg.mozilla.org/mozilla-central/rev/db368edfc832

Comment 89

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Parts 1, 2, 4, 9, 17: Update build system to support MSVC 2015

https://hg.mozilla.org/integration/mozilla-inbound/rev/8937836d3c93

Thanks for the reviews!

Comment 90

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546584 [details] [diff] [review]
Part 15: Replace uses of hash_map with unordered_map (toolkit)

https://hg.mozilla.org/integration/mozilla-inbound/rev/7377ad4937ad

Ted, I looked into contributing this patch upstream, but the process outlined in their contribution process seemed quite heavyweight for such a simple patch (e.g. requesting read/write access to their code review repo, and other stuff.) It would be great if you could upstream this for me.

Comment 91

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546584 [details] [diff] [review]
Part 15: Replace uses of hash_map with unordered_map (toolkit)

https://hg.mozilla.org/integration/mozilla-inbound/rev/7377ad4937ad

Comment 92

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8937836d3c93
https://hg.mozilla.org/mozilla-central/rev/7377ad4937ad

Comment 93

[(not currently active) Ted Mielczarek]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #90)
> Ted, I looked into contributing this patch upstream, but the process
> outlined in their contribution process seemed quite heavyweight for such a
> simple patch (e.g. requesting read/write access to their code review repo,
> and other stuff.) It would be great if you could upstream this for me.

The contribution process docs are terrible, you can do it with less overhead but it's a simple patch so I just landed it for you:
https://code.google.com/p/google-breakpad/source/detail?r=1419

Comment 94

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #93)
> The contribution process docs are terrible, you can do it with less overhead
> but it's a simple patch so I just landed it for you:
> https://code.google.com/p/google-breakpad/source/detail?r=1419

Thanks!

Comment 95

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8546576 [details] [diff] [review]
Part 7: Work around internal compiler error in Skia

Review of attachment 8546576 [details] [diff] [review]:
-----------------------------------------------------------------

Can you file a bug with microsoft about this? It seems like it would be better to fix the unreleased compiler than try to patch all of the source code.

Comment 96

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #95)
> Comment on attachment 8546576 [details] [diff] [review]
> Part 7: Work around internal compiler error in Skia
> 
> Review of attachment 8546576 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you file a bug with microsoft about this? It seems like it would be
> better to fix the unreleased compiler than try to patch all of the source
> code.

I see that you perhaps have. It would be good to include a link to the problem in the patch.

Comment 97

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #96)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #95)
> > Comment on attachment 8546576 [details] [diff] [review]
> > Part 7: Work around internal compiler error in Skia
> > 
> > Review of attachment 8546576 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Can you file a bug with microsoft about this? It seems like it would be
> > better to fix the unreleased compiler than try to patch all of the source
> > code.
> 
> I see that you perhaps have. It would be good to include a link to the
> problem in the patch.

You should be able to create a reduced test case for the the issue with creduce. In the worse case you should be able to reproduce the issue with preprocessed source code.

Comment 98

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 7: Work around internal compiler error in Skia [v2]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #97)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #96)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #95)
> > > Comment on attachment 8546576 [details] [diff] [review]
> > > Part 7: Work around internal compiler error in Skia
> > > 
> > > Review of attachment 8546576 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Can you file a bug with microsoft about this? It seems like it would be
> > > better to fix the unreleased compiler than try to patch all of the source
> > > code.
> > 
> > I see that you perhaps have. It would be good to include a link to the
> > problem in the patch.
> 
> You should be able to create a reduced test case for the the issue with
> creduce. In the worse case you should be able to reproduce the issue with
> preprocessed source code.

I agree it is better to have Microsoft fix the bug in the compiler. Finding such bugs before the release, so they can be fixed, is one of the primary reasons why I kicked off this project.

Thanks to the creduce suggestion. However, it seems like another user already provided a reduced test case that results in a stack overflow in another connect.microsoft.com bug.

I've added the link to the connect bug I filed, as you requested.

Comment 99

[[e-mail]]

appcrt140.dll and desktopcrt140.dll are removed in vs2015 ctp6

Comment 100

[Xavier Fung]

(In reply to zhoubcfan from comment #99)
> appcrt140.dll and desktopcrt140.dll are removed in vs2015 ctp6

This is covered in a recent blog post:
http://blogs.msdn.com/b/vcblog/archive/2015/03/03/introducing-the-universal-crt.aspx

Comment 101

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 7: Work around internal compiler error in Skia [v2]

George, Jeff M. suggested in bug 1119776 that you are the best person for reviewing and upstreaming my Skia patches. I've verified that this patch is still needed in CTP 6 (the latest released) for MSVC 2015. It would be great to have it checked in while we wait for the final release of MSVC 2015, so that we can continue the MSVC 2015 compatibility work that started in January. 

According to Microsoft's bug tracker, they think they've fixed the internal compiler error, but they haven't released a build for us to verify that yet. I think it is good to just upstream the chance, but I've written this patch as a private patch in case you only want to have it applied until a fixed version of MSVC 2015 is released.

Thanks in advance!

Comment 102

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8586329 [details] [diff] [review]
Part 7: Work around internal compiler error in Skia [v2]

Review of attachment 8586329 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, this flew under my radar.

Can you submit this upstream?

Comment 103

[(Away)]

(In reply to George Wright (:gw280) from comment #102)

We should test this with VS2015 RC before going to the trouble. The Connect bug claims to be fixed for a "future release" though they don't say which.

Comment 104

[(Away)]

Comment on attachment 8586329 [details] [diff] [review]
Part 7: Work around internal compiler error in Skia [v2]

This is not needed anymore; the issue has been fixed in VS2015 RC.

Comment 105

[(Away)]

Do you mind if I back out ce3638e6a659 (part 8)? The original code compiles fine with the 2015 release candidate. I don't want future readers to be misled by "VS2015 doesn't like <X>" type comments.

Comment 106

[(Away)]

> Do you mind if I back out ce3638e6a659 (part 8)?
Same for part 10

Comment 107

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Major [:dmajor] from comment #105)
> Do you mind if I back out ce3638e6a659 (part 8)? The original code compiles
> fine with the 2015 release candidate. I don't want future readers to be
> misled by "VS2015 doesn't like <X>" type comments.

I agree. I'm fine with any of the workarounds I made being backed out. It may be worth checking with the module owners of the relevant modules though.

Comment 108

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

All the patches that will land for this bug have landed.

Bug 1119082 (alias VC14) is the bug for tracking the rest of the work.

Comment 109

[(Away)]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #107)
> I agree. I'm fine with any of the workarounds I made being backed out. It
> may be worth checking with the module owners of the relevant modules though.

I pinged the original reviewers on IRC. For the JS change, I removed the comment but kept the refactoring, since it's more clear anyway.

Comment 110

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d35279fba00b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e932a6b4a67

Comment 111

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d35279fba00b