1461304 - Windows bustage eg: /js/src/wasm/WasmBinaryToAST.cpp when Gecko 62 merges to Beta on 2018-06-14

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Stefan Hindli [:stefan_hindli]]

[Tracking Requested - why for this release]:

Did central as beta simulation: https://treeherder.mozilla.org/#/jobs?repo=try&revision=218bf7a9b3a25071e68a7c008f6fd2761fb1e235&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=178304579

z:/task_1526291121/src/js/src/wasm/WasmBinaryToAST.cpp(1751): error C2220: warning treated as error - no 'object' file generated
z:/task_1526291121/src/js/src/wasm/WasmBaselineCompile.cpp(9875): error C2220: warning treated as error - no 'object' file generated
z:/task_1526291121/src/js/src/wasm/WasmIonCompile.cpp(4109): error C2220: warning treated as error - no 'object' file generated
z:/task_1526291121/src/js/src/wasm/WasmValidate.cpp(822): error C2220: warning treated as error - no 'object' file generated

This a regression from bug 1459552.

Comment 1

[Lars T Hansen [:lth]]

"z:/task_1526291121/src/js/src/wasm/WasmBinaryToAST.cpp(1751): warning C4065: switch statement contains 'default' but no 'case' labels"  Etc.

I'm not impressed by this warning; the situation will occur naturally in the wasm engine as we have various compile-time configurations controlling in-progress features.  I myself have written code this last week that would trigger this, and working around it would introduce pointless hair.

What's the argument in favor of having this warning?

Comment 2

[Julian Seward [:jseward]]

(In reply to Lars T Hansen [:lth] from comment #1)
> I'm not impressed by this warning; [..]

+1 for that.  Working around this will require significant additional
complexity in bits of code which are already difficult to understand
and reason about, due to the presence of ifdefs.  And some of it
(the Wasm verifier) is stuff we really don't want to get wrong.  So,
overall this is a net loss for us.

Is there some annotation we can put on the relevant case statements,
that tells MSVC not to emit this warning?  Or some other way to 
disable it?

Comment 3

[Julian Seward [:jseward]]

Nathan, greetings.  Do you know to whom we should enquire about
rationale and/or possible excemption to this "warning C4065" in
comment 1?  Or some other way to work around this, that does not
require making complex control flow even more complex?

Comment 4

[Nathan Froyd [:froydnj]]

We can disable particular warnings in configure (this is Gecko's, so you'd have to do the same thing for js/src/ as well):

https://searchfox.org/mozilla-central/source/old-configure.in#928-943
\

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Julien or Steven, is this something you might take on, or can you help find an owner?

Comment 6

[Julian Seward [:jseward]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #5)
> Julien or Steven, is this something you might take on,

Liz, I'll try to come up for a patch for it this week.  Sorry to be slow.

Comment 7

[Julian Seward [:jseward]]

Attachment: bug1461304-rm-windows-warning-1.diff

Fix as suggested by Nathan.

Comment 8

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

The patch has fixed the issue in today's central-as-beta simulation: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfb21b2669892bfa3f151fc3b86d0670bb2a1357&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable

Comment 9

[Julian Seward [:jseward]]

Attachment: bug1461304-rm-windows-warning-2.diff

Now with a commit message.

Comment 10

[Julian Seward [:jseward]]

Looks like there's no resulting build breakage on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f26075df05d6db5a95c41a8a4db8aa16903abc9e

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8980664 [details] [diff] [review]
bug1461304-rm-windows-warning-2.diff

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

Thanks!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2435b35bcd68
Windows bustage eg: /js/src/wasm/WasmBinaryToAST.cpp when Gecko 62 merges to Beta on 2018-06-14.  r=froydnj.

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2435b35bcd68

Comment 14

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

Verified fixed in today's central as beta simulation: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a4c5e240f48949d2d602f85cc7422e134484549&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable