Closed Bug 1229604 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Blocks: VC14
Bug 1229604 - Use const for some struct arrays instead of constexpr to work around bug of VS2015u1.
Attachment #8694511 - Flags: review?(jdemooij)
Duplicate of this bug: 1229629
Here is my patch, which is an easy workaround :)
Bug title should be:
VS2015u1 breaks build of Assembler-x86.h & Assembler-x64.h (C2078)
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
Summary: VS2015u1 breaks build of Assembler-x64.h → VS2015u1 breaks build of Assembler-x86.h & Assembler-x64.h (C2078)
FYI, I commented on Bug 1229629 patch.
(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;
    }
(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.
Flags: needinfo?(nicolas.b.pierron)
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)
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...
(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.
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 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.
Attachment #8694511 - Flags: review?(jdemooij) → review+
(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.
Flags: needinfo?(nicolas.b.pierron)
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).
(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" ?
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
Assignee: nobody → quanxunzhen
A new patch according to Comment 8 .
Attachment #8694541 - Attachment is obsolete: true
Attachment #8695625 - Flags: review?(nicolas.b.pierron)
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 :)
Attachment #8695625 - Flags: review?(nicolas.b.pierron) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/7ac917869c21
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(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...
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.
(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.
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.
(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.
Then https://hg.mozilla.org/mozilla-central/rev/7ac917869c21 should be backed out after your patch lands.
Assignee: quanxunzhen → coolypf
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8694511 - Attachment is obsolete: true
Attachment #8695625 - Flags: checkin?
Keywords: checkin-needed
Attachment #8695625 - Flags: checkin? → checkin+
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....
Flags: needinfo?(coolypf)
(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.
Flags: needinfo?(coolypf)
Add another RegisterBits wrapper.

This problem is specific to VS2013. VS2015 does not complain here.
Attachment #8696302 - Flags: review?(nicolas.b.pierron)
Attachment #8696302 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8696302 - Flags: checkin?
Keywords: checkin-needed
Attachment #8695625 - Flags: checkin+ → checkin?
is there a try run for this change ?
Flags: needinfo?(coolypf)
(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.
Flags: needinfo?(coolypf)
(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.
canceling checkin-needed for now, seems we need a try here according to comment #34
Keywords: checkin-needed
A combined patch with MOZ_IMPLICIT added.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=123fc46b7a30
Attachment #8695625 - Attachment is obsolete: true
Attachment #8696302 - Attachment is obsolete: true
Attachment #8695625 - Flags: checkin?
Attachment #8696302 - Flags: checkin?
Attachment #8697186 - Flags: review?(quanxunzhen)
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?
(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 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.
Attachment #8697186 - Flags: review?(quanxunzhen)
(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.
Attachment #8697186 - Flags: review?(nicolas.b.pierron)
Attachment #8694511 - Attachment is obsolete: false
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.
Attachment #8697186 - Flags: review?(nicolas.b.pierron) → feedback+
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
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.
If you close it... the assignee should be me then :)
Assignee: coolypf → quanxunzhen
You need to log in before you can comment on or make changes to this bug.