Closed Bug 1461304 Opened Last year Closed Last year

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

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- unaffected
firefox62 + verified

People

(Reporter: stefan_hindli, Assigned: jseward)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

[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.
Flags: needinfo?(jseward)
"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?
(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?
Flags: needinfo?(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?
Flags: needinfo?(nfroyd)
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
\
Flags: needinfo?(nfroyd)
Julien or Steven, is this something you might take on, or can you help find an owner?
Flags: needinfo?(sdetar)
Flags: needinfo?(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.
Flags: needinfo?(sdetar)
Flags: needinfo?(lhenry)
Flags: needinfo?(jseward)
Fix as suggested by Nathan.
Now with a commit message.
Attachment #8979437 - Attachment is obsolete: true
Attachment #8980664 - Flags: review?(nfroyd)
Comment on attachment 8980664 [details] [diff] [review]
bug1461304-rm-windows-warning-2.diff

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

Thanks!
Attachment #8980664 - Flags: review?(nfroyd) → review+
Pushed by jseward@mozilla.com:
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.
https://hg.mozilla.org/mozilla-central/rev/2435b35bcd68
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee: nobody → jseward
You need to log in before you can comment on or make changes to this bug.