Closed Bug 1331939 Opened 3 years ago Closed 3 years ago

Tidying up WasmBinary*.h breaks compilation with mingw-w64: error: expected primary-expression before ‘(’ token

Categories

(Core :: JavaScript Engine, defect)

52 Branch
All
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: gk, Assigned: tjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor])

Attachments

(2 files)

Bug 1316625 broke compilation with mingw-w64:

In file included from /home/firefox/win52/mozilla-aurora/obj-mingw/js/src/Unified_cpp_js_src39.cpp:47:0:
/home/firefox/win52/mozilla-aurora/js/src/wasm/WasmTextToBinary.cpp: In function ‘bool EncodeGlobalType(js::wasm::Encoder&, const js::wasm::AstGlobal*)’:
/home/firefox/win52/mozilla-aurora/js/src/wasm/WasmTextToBinary.cpp:4407:56: error: expected primary-expression before ‘(’ token
            e.writeVarU32(global->isMutable() ? uint32_t(GlobalFlags::IsMutable) : 0);
                                                        ^
/home/firefox/win52/mozilla-aurora/js/src/wasm/WasmTextToBinary.cpp:4407:57: error: reference to ‘GlobalFlags’ is ambiguous
            e.writeVarU32(global->isMutable() ? uint32_t(GlobalFlags::IsMutable) : 0);
                                                         ^
In file included from /home/firefox/win52/toolchain/mingw-w64/i686-w64-mingw32/include/windows.h:70:0,
                 from /home/firefox/win52/mozilla-aurora/js/src/jswin.h:16,
                 from /home/firefox/win52/mozilla-aurora/js/src/wasm/WasmSignalHandlers.cpp:215,
                 from /home/firefox/win52/mozilla-aurora/obj-mingw/js/src/Unified_cpp_js_src39.cpp:20:
/home/firefox/win52/toolchain/mingw-w64/i686-w64-mingw32/include/winbase.h:1078:26: note: candidates are: UINT GlobalFlags(HGLOBAL)
   WINBASEAPI UINT WINAPI GlobalFlags (HGLOBAL hMem);
                          ^
In file included from /home/firefox/win52/mozilla-aurora/js/src/wasm/WasmTypes.h:38:0,
                 from /home/firefox/win52/mozilla-aurora/js/src/wasm/WasmJS.h:24,
                 from /home/firefox/win52/mozilla-aurora/js/src/wasm/WasmJS.cpp:19,
                 from /home/firefox/win52/mozilla-aurora/obj-mingw/js/src/Unified_cpp_js_src39.cpp:2:
/home/firefox/win52/mozilla-aurora/js/src/wasm/WasmBinaryConstants.h:105:12: note:                 enum class js::wasm::GlobalFlags
 enum class GlobalFlags
            ^
If you prefix all the uses of GlobalFlags [1] with wasm::, so these explicitly appear as wasm::GlobalFlags, does it fix the issue?

[1] http://searchfox.org/mozilla-central/search?q=GlobalFlags%3A%3A&case=false&regexp=false&path=
(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> If you prefix all the uses of GlobalFlags [1] with wasm::, so these
> explicitly appear as wasm::GlobalFlags, does it fix the issue?
> 
> [1]
> http://searchfox.org/mozilla-central/
> search?q=GlobalFlags%3A%3A&case=false&regexp=false&path=

I have currently only mozilla-aurora handy (which is why the description contains references to that branch) but changing the respective files there and adding "wasm::" solved the problem for me.
tjr: I guess you want to fix WasmValidate.cpp as well?
To avoid this being a reoccurring problem, it seems better to just rename our wasm::GlobalFlags to something else, like GlobalTypeImmediate.
Comment on attachment 8828380 [details]
Bug 1331939 Rename GlobalFlags to GlobalTypeImmediate to fix MinGW build

https://reviewboard.mozilla.org/r/105814/#review108008

Thanks!
Attachment #8828380 - Flags: review?(luke) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9a2eef2935bf
Rename GlobalFlags to GlobalTypeImmediate to fix MinGW build r=luke
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9a2eef2935bf
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Tom, do you think we could get your fix uplifted to aurora and beta? If so, could you file the necessary requests? It seems the risk for breakage is pretty low and it would allow us carrying one patch less. Your patch is applying cleanly for aurora. For beta I attached an adapted one (although I only had the git repo handy...).
Flags: needinfo?(tom)
Comment on attachment 8832385 [details] [diff] [review]
Backported patch for mozilla-beta

Approval Request Comment
[User impact if declined]: Tor will have to carry another patch in their tree

[Is this code covered by automated tests?]: If you consider compilation an automated test, yes.

[Has the fix been verified in Nightly?]: Yes

[Needs manual test from QE? If yes, steps to reproduce]: No

[List of other uplifts needed for the feature/fix]: None

[Is the change risky?]: No.

[Why is the change risky/not risky?]: It's not risky. If the compilation is successful everything is fine. 

[String changes made/needed]: None.
Flags: needinfo?(tom)
Attachment #8832385 - Flags: approval-mozilla-beta?
Attachment #8832385 - Flags: approval-mozilla-aurora?
Comment on attachment 8832385 [details] [diff] [review]
Backported patch for mozilla-beta

Fix a compilation error with mingw-w64. Aurora53+.
Attachment #8832385 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
needs rebasing for aurora:

applying 0001-Backport-of-tjr-s-patch-for-bug-1331939-for-mozilla-.patch
unable to find 'js/src/wasm/WasmBinaryFormat.cpp' for patching
(use '--prefix' to apply patch relative to the current directory)
1 out of 1 hunks FAILED -- saving rejects to file js/src/wasm/WasmBinaryFormat.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 0001-Backport-of-tjr-s-patch-for-bug-1331939-for-mozilla-.patch
Tomcats-MacBook-Pro-300:mozilla-aurora Tomcat$
Flags: needinfo?(tom)
The attached patch is needed for mozilla-beta. For mozilla-aurora the patch landed on mozilla-central should work.
Flags: needinfo?(tom)
Comment on attachment 8832385 [details] [diff] [review]
Backported patch for mozilla-beta

mingw build fix, beta52+
Attachment #8832385 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Looks like Tomcat uplifted the patch that landed on Aurora to Beta instead of the attached Beta patch. I pushed a follow-up fix for the bustage that caused.
https://hg.mozilla.org/releases/mozilla-beta/rev/f78e406f5ef5cf13522b70e981561b1280dc917a
You need to log in before you can comment on or make changes to this bug.