1229604 - VS2015u1 breaks build of Assembler-x86.h & Assembler-x64.h (C2078)

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

After upgrading to VS2015u1, it starts to complain about this error:
> 0:40.35 central\js\src\jit/x64/Assembler-x64.h(109): error C2078: too many initializers
> 0:40.35 central\js\src\jit/x64/Assembler-x64.h(111): error C2078: too many initializers

It is definitely a compiler bug, which I have filed https://connect.microsoft.com/VisualStudio/feedback/details/2080996 to Microsoft.

I don't see any easy workaround here.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1229604 - Use const for some struct arrays instead of constexpr to work around bug of VS2015u1.

Bug 1229604 - Use const for some struct arrays instead of constexpr to work around bug of VS2015u1.

Comment 3

[Yuan Pengfei]

Attachment: Fix error C2078 when building with Visual Studio 2015 Update 1

Here is my patch, which is an easy workaround :)

Comment 4

[Yuan Pengfei]

Bug title should be:
VS2015u1 breaks build of Assembler-x86.h & Assembler-x64.h (C2078)

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Yeah. I wasn't aware that x86 is also broken.

You probably can apply for a editbugs permission so that you can edit the title yourself in the future, see https://developer.mozilla.org/en/docs/What_to_do_and_what_not_to_do_in_Bugzilla

Comment 6

[Nicolas B. Pierron [:nbp]]

FYI, I commented on Bug 1229629 patch.

Comment 7

[Yuan Pengfei]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> FYI, I commented on Bug 1229629 patch.

I tried:

diff --git a/js/src/jit/Registers.h b/js/src/jit/Registers.h
--- a/js/src/jit/Registers.h
+++ b/js/src/jit/Registers.h
@@ -31,16 +31,22 @@ namespace jit {

 struct Register {
     typedef Registers Codes;
     typedef Codes::Encoding Encoding;
     typedef Codes::Code Code;
     typedef Codes::SetType SetType;

     Codes::Encoding reg_;
+    MOZ_CONSTEXPR Register()
+        : reg_(Encoding(0))
+    { }
+    MOZ_CONSTEXPR Register(Encoding r)
+        : reg_(r)
+    { }
     static Register FromCode(Code i) {
         MOZ_ASSERT(i < Registers::Total);
         Register r = { Encoding(i) };
         return r;
     }
     static Register FromName(const char* name) {
         Code code = Registers::FromName(name);
         Register r = { Encoding(code) };

But failed with:

mozilla\js\src\jit/Snapshots.h(136): error C2280 ...

Related code is:

    union Payload {
        uint32_t index;
        int32_t stackOffset;
        Register gpr;
        FloatRegisterBits fpu;
        JSValueType type;
    };

    Payload arg1_;
    Payload arg2_;

    static Payload payloadOfIndex(uint32_t index) {
        Payload p;              <===== error C2280
        p.index = index;
        return p;
    }

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Yuan Pengfei from comment #7)
> (In reply to Nicolas B. Pierron [:nbp] from comment #6)
> > FYI, I commented on Bug 1229629 patch.
> 
> I tried: […]

Thanks :)

> But failed with:
> 
> mozilla\js\src\jit/Snapshots.h(136): error C2280 ...
> 
> Related code is:
> 
>     union Payload {
>         uint32_t index;
>         int32_t stackOffset;
>         Register gpr;
>         FloatRegisterBits fpu;

We should have something like RegisterBits as well, but if you don't want to do it, I am ok to take over this bug within the next 2 weeks.

Comment 9

[Emanuel Hoogeveen [:ehoogeveen]]

One thing I don't really understand: why does qualifying the registers with X86Encoding:: work around the bug? Should the MSVC bug report be updated with that workaround? (the testcase in that bug doesn't look like it could be fixed in the same way)

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Hmmm, I still think just replacing the constexpr with const is the easiest and safest workaround for this issue.

But I'm fine anyway. I just hope this bug could get fixed as soon as possible, because I don't like keeping a temporary patch in my patch queue for a long time...

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #9)
> One thing I don't really understand: why does qualifying the registers with
> X86Encoding:: work around the bug? Should the MSVC bug report be updated
> with that workaround? (the testcase in that bug doesn't look like it could
> be fixed in the same way)

Well, because that is another acceptable way to initialize a struct array, and the bug doesn't exist in that path.

Comment 12

[Jan de Mooij [:jandem]]

https://reviewboard.mozilla.org/r/26803/#review24451

Thanks for the patch. Weird build failures are annoying and this change seems pretty safe and minimal. r=me with comments below addressed.

::: js/src/jit/x64/Assembler-x64.h:109
(Diff revision 1)
> -static MOZ_CONSTEXPR_VAR Register IntArgRegs[NumIntArgRegs] = { rcx, rdx, r8, r9 };
> +static const Register IntArgRegs[NumIntArgRegs] = { rcx, rdx, r8, r9 };

To make sure we don't accidentally revert this change, please add a brief comment here, something like:

// const instead of MOZ_CONSTEXPR_VAR to work around MSVC 2015u1 bug (bug 1229604).

The similar changes below probably don't need the comment.

Also, we should fix x86 as well while we're at it.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8694511 [details]
MozReview Request: Bug 1229604 - Use const for some struct arrays instead of constexpr to work around bug of VS2015u1.

https://reviewboard.mozilla.org/r/26805/#review24453

Oops, forgot to r+. See previous comments.

Comment 14

[Nicolas B. Pierron [:nbp]]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #9)
> One thing I don't really understand: why does qualifying the registers with
> X86Encoding:: work around the bug? Should the MSVC bug report be updated
> with that workaround? (the testcase in that bug doesn't look like it could
> be fixed in the same way)

Note that "rax" and "X86Encoding::rax" do not name the same variable.  "rax" refers to an definition made above

  static constexpr Register rax = { X86Encoding::rax };

while "X86Encoding::rax" is avoiding the above definition to get its result from an enum.

Comment 15

[Emanuel Hoogeveen [:ehoogeveen]]

Aha, thanks (both of you)! I didn't realize that the unqualified ones were definitions (I guess I should have, looking again - it's right there in the patch).

Comment 16

[Yuan Pengfei]

(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> > But failed with:
> > 
> > mozilla\js\src\jit/Snapshots.h(136): error C2280 ...
> > 
> > Related code is:
> > 
> >     union Payload {
> >         uint32_t index;
> >         int32_t stackOffset;
> >         Register gpr;
> >         FloatRegisterBits fpu;
> 
> We should have something like RegisterBits as well, but if you don't want to
> do it, I am ok to take over this bug within the next 2 weeks.

"Register gpr" ==> "RegisterBits gpr" ?

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ac917869c2103fcfc2dfaecbb558320e4aac929
Bug 1229604 - Use const for some struct arrays instead of constexpr to work around bug of VS2015u1. r=jandem

Comment 18

[Yuan Pengfei]

Attachment: Fix error C2078 when building with Visual Studio 2015 Update 1 - v2

A new patch according to Comment 8 .

Comment 19

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8695625 [details] [diff] [review]
Fix error C2078 when building with Visual Studio 2015 Update 1 - v2

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

Perfect, Thanks :)

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8695625 [details] [diff] [review]
Fix error C2078 when building with Visual Studio 2015 Update 1 - v2

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

::: js/src/jit/Registers.h
@@ +38,5 @@
>      Codes::Encoding reg_;
> +    MOZ_CONSTEXPR Register()
> +        : reg_(Encoding(0))
> +    { }
> +    MOZ_CONSTEXPR Register(Encoding r)

You need an "explicit" here, or alternatively, MOZ_IMPLICIT. Otherwise it would break static analysis build.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ac917869c21

Comment 22

[Yuan Pengfei]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #20)
> ::: js/src/jit/Registers.h
> @@ +38,5 @@
> >      Codes::Encoding reg_;
> > +    MOZ_CONSTEXPR Register()
> > +        : reg_(Encoding(0))
> > +    { }
> > +    MOZ_CONSTEXPR Register(Encoding r)
> 
> You need an "explicit" here, or alternatively, MOZ_IMPLICIT. Otherwise it
> would break static analysis build.

I am afraid "Architecture-x86-shared.h" violates Bug 1009631 as well...

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

FWIW, I suggest you discuss and land that patch in a new bug, because this bug has been fixed, and the new patch doesn't seem to be highly related to this bug.

Comment 24

[Yuan Pengfei]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #23)
> FWIW, I suggest you discuss and land that patch in a new bug, because this
> bug has been fixed, and the new patch doesn't seem to be highly related to
> this bug.

No.

My patch also fixes this bug, which is suggested in Bug 1229629 Comment 2 and Comment 8 of this bug.

Comment 25

[Xidorn Quan [:xidorn] UTC+11]

I suppose your original patch fixes this bug because it uses X86Encoding::???, which doesn't seem to present in the new patch anymore, so I assume this patch is more on fixing something else.

If this patch does individually (without the X86Encoding:: workaround) fix this bug without changing constexpr to const, it may be worth reverting my previous patch as well.

Comment 26

[Yuan Pengfei]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #25)
> I suppose your original patch fixes this bug because it uses
> X86Encoding::???, which doesn't seem to present in the new patch anymore, so
> I assume this patch is more on fixing something else.
> 
> If this patch does individually (without the X86Encoding:: workaround) fix
> this bug without changing constexpr to const, it may be worth reverting my
> previous patch as well.

The new patch fixes this bug without X86Encoding...
I have marked the old patch obsolete.

Comment 27

[Xidorn Quan [:xidorn] UTC+11]

Then https://hg.mozilla.org/mozilla-central/rev/7ac917869c21 should be backed out after your patch lands.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4053d8be57b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cb584405271

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for Spidermonkey failures on Windows and static failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/7467d21a2d53

Backout job: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7467d21a2d53
Failing job: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=0b7cbe365e63

One of the failing jobs: SM(p) https://treeherder.mozilla.org/logviewer.html#?job_id=18334483&repo=mozilla-inbound

RegExp.cpp
c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\js\src\jit/RegisterSets.h(291) : error C2620: 'js::jit::Int32Key::reg_' : illegal union member; type 'js::jit::Register' has a user-defined constructor or non-trivial default constructor
c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\js\src\jit/RegisterSets.h(297) : error C2614: 'js::jit::Int32Key' : illegal member initialization: 'reg_' is not a base or member
c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\js\src\jit/RegisterSets.h(309) : error C2065: 'reg_' : undeclared identifier

In the directory  /c/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/js/src
The following command failed to execute properly:
c:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/_virtualenv/Scripts/python.exe -m mozbuild.action.cl cl -FoRegExp.obj -c -D_CRT_RAND_S -DENABLE_BINARYDATA -DENABLE_SHARED_ARRAY_BUFFER -DEXPORT_JS_API -Ic:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/js/src -I. -Ic:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/intl/icu/source/common -Ic:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/intl/icu/source/i18n -I../../dist/include -Ic:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/dist/include/nspr -MD -FI ../../js/src/js-confdefs.h -DMOZILLA_CLIENT -TP -nologo -wd4345 -wd4351 -wd4800 -wd4819 -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -W3 -Gy -arch:IA32 -FS -wd4244 -wd4267 -wd4251 -we4553 -GR- -DNDEBUG -DTRIMMED -Zi -UDEBUG -DNDEBUG -O2 -Oy -wd4805 -wd4661 -we4067 -we4258 -we4275 -wd4146 -wd4273 -Fdgenerated.pdb c:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/js/src/builtin/RegExp.cpp
c:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/config/rules.mk:956: recipe for target 'RegExp.obj' failed
mozmake[3]: *** [RegExp.obj] Error 1
mozmake[3]: *** Waiting for unfinished jobs....

Comment 30

[Yuan Pengfei]

(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #29)
> Backed out for Spidermonkey failures on Windows and static failures:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7467d21a2d53
> 
> Backout job:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=7467d21a2d53
> Failing job:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=0b7cbe365e63
> 
> One of the failing jobs: SM(p)
> https://treeherder.mozilla.org/logviewer.html#?job_id=18334483&repo=mozilla-
> inbound
> 
> RegExp.cpp
> c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\js\src\jit/
> RegisterSets.h(291) : error C2620: 'js::jit::Int32Key::reg_' : illegal union
> member; type 'js::jit::Register' has a user-defined constructor or
> non-trivial default constructor
> c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\js\src\jit/
> RegisterSets.h(297) : error C2614: 'js::jit::Int32Key' : illegal member
> initialization: 'reg_' is not a base or member
> c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\js\src\jit/
> RegisterSets.h(309) : error C2065: 'reg_' : undeclared identifier
> 
> In the directory 
> /c/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/js/src
> The following command failed to execute properly:
> c:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/
> _virtualenv/Scripts/python.exe -m mozbuild.action.cl cl -FoRegExp.obj -c
> -D_CRT_RAND_S -DENABLE_BINARYDATA -DENABLE_SHARED_ARRAY_BUFFER
> -DEXPORT_JS_API
> -Ic:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/js/src -I.
> -Ic:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/intl/icu/source/
> common
> -Ic:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/intl/icu/source/
> i18n -I../../dist/include
> -Ic:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/dist/
> include/nspr -MD -FI ../../js/src/js-confdefs.h -DMOZILLA_CLIENT -TP -nologo
> -wd4345 -wd4351 -wd4800 -wd4819 -D_CRT_SECURE_NO_WARNINGS
> -D_HAS_EXCEPTIONS=0 -W3 -Gy -arch:IA32 -FS -wd4244 -wd4267 -wd4251 -we4553
> -GR- -DNDEBUG -DTRIMMED -Zi -UDEBUG -DNDEBUG -O2 -Oy -wd4805 -wd4661 -we4067
> -we4258 -we4275 -wd4146 -wd4273 -Fdgenerated.pdb
> c:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/js/src/builtin/
> RegExp.cpp
> c:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/config/rules.mk:956:
> recipe for target 'RegExp.obj' failed
> mozmake[3]: *** [RegExp.obj] Error 1
> mozmake[3]: *** Waiting for unfinished jobs....

I see. It is another union with struct Register.
I will add a new patch for it.

Comment 31

[Yuan Pengfei]

Attachment: Fix VS2013 error C2620 in RegisterSets.h

Add another RegisterBits wrapper.

This problem is specific to VS2013. VS2015 does not complain here.

Comment 32

[Carsten Book [:Tomcat]]

is there a try run for this change ?

Comment 33

[Yuan Pengfei]

(In reply to Carsten Book [:Tomcat] from comment #32)
> is there a try run for this change ?

Since I do not have access to the try server,
I tried it on my local machine with both VS2013 and VS2015.

Comment 34

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Yuan Pengfei from comment #33)
> (In reply to Carsten Book [:Tomcat] from comment #32)
> > is there a try run for this change ?
> 
> Since I do not have access to the try server,
> I tried it on my local machine with both VS2013 and VS2015.

You at least haven't fix the static checking build failure "error: bad implicit conversion constructor for 'Register'". Do you think that should be 'explicit' or 'MOZ_IMPLICIT'? That isn't completely clear to me.

Could you try building it with 'explicit' on VS compiler and see whether that works?

Also you can follow this procedure to apply for commit access for try server: https://www.mozilla.org/en-US/about/governance/policies/commit/ you can cc me on your applying bug and I can vouch for you. After that, you could use http://trychooser.pub.build.mozilla.org/ to generate the try syntax.

Comment 35

[Carsten Book [:Tomcat]]

canceling checkin-needed for now, seems we need a try here according to comment #34

Comment 36

[Yuan Pengfei]

Attachment: Fix error C2078 when building with Visual Studio 2015 Update 1 - v3

A combined patch with MOZ_IMPLICIT added.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=123fc46b7a30

Comment 37

[Xidorn Quan [:xidorn] UTC+11]

I tend to think we should always use "explicit" unless we really have good reason not to. I believe that is why we have such static analysis item. Could you explain why we have to allow implicit conversion here?

Comment 38

[Yuan Pengfei]

(In reply to Xidorn Quan [:xidorn] (UTC-5) from comment #37)
> I tend to think we should always use "explicit" unless we really have good
> reason not to. I believe that is why we have such static analysis item.
> Could you explain why we have to allow implicit conversion here?

Declaring it "explicit" breaks copy-list-initialization like:
Register r = { Encoding(i) };

See http://en.cppreference.com/w/cpp/language/explicit

Comment 39

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8697186 [details] [diff] [review]
Fix error C2078 when building with Visual Studio 2015 Update 1 - v3

In that case, I think it would be better to fix the callsite instead of marking the constructor implicit. The constructor should be marked implicit only when we really want to support implicit conversion between the two types.

You can change the callsite to
> Register r(Encoding(i));

Besides, I am not a peer of js code, so I cannot review it. You may still want to request review from :nbp.

Comment 40

[Yuan Pengfei]

(In reply to Xidorn Quan [:xidorn] (UTC-5) from comment #39)
> In that case, I think it would be better to fix the callsite instead of
> marking the constructor implicit. The constructor should be marked implicit
> only when we really want to support implicit conversion between the two
> types.
> 
> You can change the callsite to
> > Register r(Encoding(i));

I will file a new bug for this issue.

Comment 41

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8697186 [details] [diff] [review]
Fix error C2078 when building with Visual Studio 2015 Update 1 - v3

Please open a new bug to address this issue, and I will review this patch there.

This issue should have been closed because the issue described in comment 0 is no longer present, as it got fixed in comment 17, and re-made in comment 29.

Yes, the solution is not the cleanest and this is now a different issue, which should be fixed in a different bug.

Comment 42

[Xidorn Quan [:xidorn] UTC+11]

The idea of still doing it in this bug is, given comment 26, the new patch independently fixes this bug as well. And given the new patch may be more sensible, we want to backout my patch landed in comment 17 as soon as the new patch lands. But if you prefer closing this bug and do it in a new bug, I'm fine anyway.

Comment 43

[Xidorn Quan [:xidorn] UTC+11]

If you close it... the assignee should be me then :)