Closed Bug 1439004 Opened 4 years ago Closed 4 years ago

build bustage in js/src/wasm/WasmBinaryToText.cpp when Gecko 60 merges to Beta on 2018-03-01

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- verified

People

(Reporter: aryx, Assigned: sunfish)

References

Details

Attachments

(2 files)

When Gecko 60 merges to beta, the builds will fail.

Try push with build failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc1afe3d70a131e2022bc470b0b78a695fc95cee&selectedJob=162731903
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=162731903&repo=try

js/src/wasm/WasmBinaryToText.cpp:754:53: error: 'AstExtraConversionOperator' has not been declared

The code landed in bug 1435369: https://hg.mozilla.org/mozilla-central/rev/25900f3b9936#l30.12
Flags: needinfo?(sunfish)
I am also seeing this build error when I remove DEFINES['ENABLE_WASM_SATURATING_TRUNC_OPS'] = True from moz.build.

/Users/jolesen/gecko-dev/js/src/wasm/WasmBinaryToText.cpp:754:53: error: unknown type name 'AstExtraConversionOperator'; did you mean 'AstConversionOperator'?
RenderExtraConversionOperator(WasmRenderContext& c, AstExtraConversionOperator& conv)
                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~
                                                    AstConversionOperator

/Users/jolesen/gecko-dev/js/src/wasm/WasmTextToBinary.cpp:302:17: warning: enumeration value 'ExtraConversionOpcode' not handled in switch [-Wswitch]
        switch (kind_) {
                ^
/Users/jolesen/gecko-dev/js/src/wasm/WasmTextToBinary.cpp:302:17: note: add missing switch cases
        switch (kind_) {
                ^
/Users/jolesen/gecko-dev/js/src/wasm/WasmTextToBinary.cpp:4318:18: warning: enumeration value 'ExtraConversionOperator' not handled in switch [-Wswitch]
    switch (expr.kind()) {
                 ^
/Users/jolesen/gecko-dev/js/src/wasm/WasmTextToBinary.cpp:4318:18: note: add missing switch cases
    switch (expr.kind()) {
                 ^
/Users/jolesen/gecko-dev/js/src/wasm/WasmTextToBinary.cpp:4930:18: warning: enumeration value 'ExtraConversionOperator' not handled in switch [-Wswitch]
    switch (expr.kind()) {
                 ^
/Users/jolesen/gecko-dev/js/src/wasm/WasmTextToBinary.cpp:4930:18: note: add missing switch cases
    switch (expr.kind()) {
                 ^
Attached patch bug1439004.patchSplinter Review
Fixed in the attached patch. I had to pick between removing the `#ifdef`s around AstExtraConversionOperator and adding `#ifdef`s around code that uses it; I opted to remove `#ifdef`s in the patch here, because it keeps the code simpler, however I'm happy to do it either way.
Assignee: nobody → sunfish
Status: NEW → ASSIGNED
Flags: needinfo?(sunfish)
Attachment #8952492 - Flags: review?(luke)
Comment on attachment 8952492 [details] [diff] [review]
bug1439004.patch

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

Thanks!
Attachment #8952492 - Flags: review?(luke) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdf244d72f44
Fix build breakage when ENABLE_WASM_SATURATING_TRUNC_OPS is not defined. r=luke
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cdf244d72f44
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
The patch didn't catch everything and beta would still be busted:

Simulation push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5b71f018f2ab5aac5cfae2293bd9b56a6b28d6c
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=163512672&repo=try

js/src/wasm/WasmTextToBinary.cpp:301:16: error: enumeration value 'ExtraConversionOpcode' not handled in switch [-Werror=switch]

Problematic code: https://dxr.mozilla.org/mozilla-central/rev/861067332bac96a44bbf41ef366f58a30476057b/js/src/wasm/WasmTextToBinary.cpp#316-318
Status: RESOLVED → REOPENED
Flags: needinfo?(sunfish)
Resolution: FIXED → ---
My test build didn't have --enable-warnings-as-errors, and I missed the warning. Attached is a follow-up patch. I'm now testing it on try here, with a modification to undefined ENABLE_WASM_SATURATING_TRUNC_OPS:

https://hg.mozilla.org/try/rev/ca79d4fef803a0e7ea479437663eb086f0b2ab7d
Comment on attachment 8952839 [details] [diff] [review]
more-ifdefs.patch

Here's the try build URL:

https://treeherder.mozilla.org/#/jobs?repo=try&
revision=655482e95c572b9985740240581af97a3fcd70f1
Flags: needinfo?(sunfish)
Attachment #8952839 - Flags: review?(luke)
Attachment #8952839 - Flags: review?(luke) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f510195124
Fix more build breakage when ENABLE_WASM_SATURATING_TRUNC_OPS is not defined. r=luke
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a4f510195124
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.