Closed Bug 1119072 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox Build System :: General, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(14 files, 20 obsolete files)

1.37 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
1.13 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
1.14 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
7.19 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
951 bytes, patch
ehsan
: review+
Details | Diff | Splinter Review
1.20 KB, patch
cmtalbert
: review+
Details | Diff | Splinter Review
1.27 KB, patch
robert.strong.bugs
: review+
jacek
: feedback+
Details | Diff | Splinter Review
3.34 KB, patch
ted
: review+
Details | Diff | Splinter Review
3.27 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
3.70 KB, patch
Details | Diff | Splinter Review
927.71 KB, text/plain
Details
2.79 KB, patch
briansmith
: review+
benjamin
: review+
bobowen
: feedback+
Details | Diff | Splinter Review
2.14 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
8.62 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
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.
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 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.
Attachment #8545584 - Flags: review+
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.
Attachment #8545605 - Flags: review?(mh+mozilla)
Attachment #8545606 - Attachment is obsolete: true
Attachment #8545606 - Flags: review?(mh+mozilla)
Attachment #8545760 - Flags: review?(mh+mozilla)
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."
Attachment #8545764 - Flags: review?(mh+mozilla)
(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
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.
Attachment #8545586 - Attachment is obsolete: true
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.
Attached file Build log showing current progress (obsolete) —
The build now goes strong for about 15 minutes before failing with an internal compiler error. See the attached build log.
(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)
(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 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?
Attachment #8545605 - Flags: review?(mh+mozilla)
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).
Attachment #8545760 - Flags: review?(mh+mozilla)
Attachment #8545760 - Flags: review?(jld)
Attachment #8545760 - Flags: review?(benjamin)
Attachment #8545764 - Flags: review?(mh+mozilla) → review+
Attachment #8545765 - Flags: review?(mh+mozilla) → review?(rjesup)
Attachment #8545766 - Flags: review?(mh+mozilla) → review?(bobowencode)
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
Attachment #8545605 - Attachment is obsolete: true
Attachment #8546407 - Flags: review?(mh+mozilla)
(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.
Attachment #8546407 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8545765 - Attachment is obsolete: true
Attachment #8545765 - Flags: review?(rjesup)
> 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.
Attachment #8545774 - Attachment is obsolete: true
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.
Alias: VC13
Attachment #8546569 - Flags: review?(ehsan) → review+
Attachment #8546581 - Flags: review?(ehsan) → review+
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.
Attachment #8546580 - Flags: review?(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
Attachment #8546584 - Flags: review?(gps)
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.
Attachment #8545766 - Flags: review?(bobowencode) → review+
Attachment #8546578 - Flags: review?(mh+mozilla) → review+
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?
Attachment #8546587 - Flags: review?(mh+mozilla) → feedback-
Attachment #8546580 - Flags: review?(robert.strong.bugs)
Attachment #8546583 - Flags: feedback?(jacek)
Attachment #8546583 - Flags: review?(gps)
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.
Attachment #8546579 - Flags: review?(jwalden+bmo) → review+
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 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).
Attachment #8545760 - Flags: review?(jld)
Attachment #8545760 - Flags: review+
Attachment #8545760 - Flags: feedback?(bobowencode)
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.)
(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
Flags: needinfo?(brian)
I guess we probably should change the alias of this bug to VC14 instead as "13" seems to have been skipped.
(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.
Attachment #8545760 - Attachment is obsolete: true
Attachment #8545760 - Flags: review?(benjamin)
Attachment #8545760 - Flags: feedback?(bobowencode)
Attachment #8547121 - Flags: review+
Attachment #8547121 - Flags: feedback?(bobowencode)
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!
Attachment #8547121 - Flags: review?(benjamin)
r=bobowencode. I added the requested change to modifications-to-chromium-to-reapply-after-upstream-merge.txt
Attachment #8545766 - Attachment is obsolete: true
Attachment #8547122 - Flags: review+
Attachment #8546584 - Flags: review?(benjamin)
Attachment #8546583 - Flags: review?(robert.strong.bugs)
Attachment #8546583 - Flags: feedback?(jacek) → feedback+
Attachment #8546577 - Flags: review?(cpearce) → review+
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.
Attachment #8547121 - Flags: feedback?(bobowencode) → feedback+
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.
Attachment #8546584 - Flags: review?(benjamin) → review?(ted)
Attachment #8547121 - Flags: review?(benjamin) → review+
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
Attachment #8546580 - Flags: review?(robert.strong.bugs) → review+
Attachment #8546582 - Flags: review?(ctalbert) → review+
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!
Attachment #8546580 - Flags: review+ → review?(robert.strong.bugs)
Alias: VC13 → VC14
(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.
Attachment #8548000 - Flags: feedback?(brian)
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?
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #57)

vc14 works differently than earlier versions.
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!
Attachment #8548000 - Flags: feedback?(brian) → feedback+
Attachment #8546585 - Flags: review?(jacob.benoit.1) → review?(jmuizelaar)
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!
Attachment #8546576 - Flags: review?(jacob.benoit.1) → review?(jmuizelaar)
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.
Attachment #8545771 - Attachment is obsolete: true
Blocks: 1121290
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"
Assignee: nobody → brian
Attachment #8545584 - Attachment is obsolete: true
Attachment #8545764 - Attachment is obsolete: true
Attachment #8546407 - Attachment is obsolete: true
Attachment #8546578 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8549106 - Flags: review?(mh+mozilla)
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.
Attachment #8546585 - Flags: review?(jmuizelaar) → review?(bas)
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!
Attachment #8546580 - Flags: review?(robert.strong.bugs) → review+
Attachment #8549106 - Flags: review?(mh+mozilla) → review+
Attachment #8546584 - Flags: review?(ted) → review?(ehsan)
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.
Attachment #8546584 - Flags: review?(ehsan) → review?(ted)
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.
Attachment #8546585 - Flags: review?(bas) → review+
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 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
Attachment #8549106 - Attachment description: Parts 1, 2, 4, 9: Update build system to make MSVC2015 build succeed → Parts 1, 2, 4, 9, 17: Update build system to make MSVC2015 build succeed
Attachment #8549106 - Flags: checkin+
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.
Attachment #8547121 - Flags: checkin+
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
Attachment #8547122 - Flags: checkin+
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
Attachment #8546577 - Flags: checkin+
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.
Attachment #8546579 - Flags: checkin+
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
Attachment #8546580 - Flags: checkin+
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
Attachment #8546581 - Flags: checkin+
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
Attachment #8546582 - Flags: checkin+
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).
Attachment #8546587 - Attachment is obsolete: true
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.
Keywords: leave-open
Whiteboard: [waiting on reviews for gfx and breakpad changes]
Have you opened Connect issues for the ones that look like compiler bugs (e.g. parts 8 and 10)?
Flags: needinfo?(brian)
(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.
Flags: needinfo?(brian)
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.
Attachment #8548000 - Attachment is obsolete: true
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
Attachment #8549106 - Flags: checkin+
I'm moving the VC14 alias to bug 1119082.
Alias: VC14
: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
Attachment #8552163 - Flags: review?(mh+mozilla)
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).
Attachment #8546584 - Flags: review?(ted) → review+
Attachment #8552163 - Flags: review?(mh+mozilla) → review+
Whiteboard: [waiting on reviews for gfx and breakpad changes] → [waiting on reviews for gfx changes]
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.
Attachment #8546584 - Flags: checkin+
(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
(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 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.
(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.
(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.
(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.
Attachment #8546576 - Attachment is obsolete: true
Attachment #8546576 - Flags: review?(jmuizelaar)
Attachment #8558714 - Flags: review?(jmuizelaar)
appcrt140.dll and desktopcrt140.dll are removed in vs2015 ctp6
(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
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!
Attachment #8558714 - Attachment is obsolete: true
Attachment #8558714 - Flags: review?(jmuizelaar)
Attachment #8586329 - Flags: review?(gwright)
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?
(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 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.
Attachment #8586329 - Attachment is obsolete: true
Attachment #8586329 - Flags: review?(gwright)
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.
Flags: needinfo?(brian)
> Do you mind if I back out ce3638e6a659 (part 8)?
Same for part 10
(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.
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.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(brian)
Resolution: --- → FIXED
Whiteboard: [waiting on reviews for gfx changes]
(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.
Keywords: leave-open
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.