Closed Bug 1443367 Opened 6 years ago Closed 6 years ago

"error C2057: expected constant expression" bustage when compiling with MSVC 2017 version 15.6

Categories

(Core :: MFBT, defect)

Product:
Core
Component:
MFBT
Platform:
Unspecified
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: RyanVM, Assigned: mozbugz)

References

Details

Attachments

(1 file)

Bug 1443367 - Rework MakeNotNull to build with VS 2017 15.6 -
59 bytes, text/x-review-board-request
n.nethercote
: review+
RyanVM
: approval-mozilla-release+
Details 
Today, Microsoft released Visual Studio 2017 version 15.6. After updating locally and clobbering, I'm hitting build bustage on m-c rev 51200c0fdadd.

c:\users\ryan\repos\mozilla\objdir-fx-64\dist\include\mozilla/NotNull.h(169): error C2057: expected constant expression
c:\users\ryan\repos\mozilla\objdir-fx-64\dist\include\gfxFontFamilyList.h(264): note: see reference to function template instantiation 'mozilla::NotNull<mozilla::SharedFontList *> mozilla::MakeNotNull<mozilla::SharedFontList*,mozilla::FontFamilyType&>(mozilla::FontFamilyType &)' being compiled
This affects other code too. Spidermonkey example:
https://treeherder.mozilla.org/logviewer.html#?job_id=166119755&repo=try&lineNumber=1501
Component: Graphics: Text → MFBT
Summary: "error C2057: expected constant expression" bustage in gfxFontFamilyList.h when compiling with MSVC 2017 version 15.6 → "error C2057: expected constant expression" bustage when compiling with MSVC 2017 version 15.6
The line in question:
https://searchfox.org/mozilla-central/source/mfbt/NotNull.h#169

Hopefully someone with more MFBT knowledge can see the issue. I would like to get this fixed in 60 still given its ESR status.
status-firefox-esr52: --- → wontfix
It's probably worth fixing for earlier version as well, since this kind of patches tend to be low risk and they are going to affect Windows developers when they need to try building a release/beta version locally.
That's... definitely a compiler bug... I cannot make any sense how that can be non-constant.

We should probably file a bug to Microsoft and maybe workaround it via conditionally remove the static_assert for the specific version of MSVC.
Just in case it didn't like our mozilla::IsArray, I tried with std::is_array instead and got the same error.

And I couldn't manage to create a small standalone piece of code to reproduce the error, so it may not be easy to get MS to fix this.
It won't be as simple as disabling the static_assert for this compiler: After doing so, the following line gives this error:
error C2440: 'initializing': cannot convert from 'mozilla::RemoveConst<mozilla::RemoveReference<unknown-type>::Type>::Type *' to 'mozilla::SharedFontList *'

So there's something VS doesn't grok anymore in `*DeclVal<SharedFontList*>()`.

I've got a working fix (by providing specializations for raw pointers), will push for review soon.
Assignee: nobody → gsquelart
Comment on attachment 8956383 [details]
Bug 1443367 - Rework MakeNotNull to build with VS 2017 15.6 -

https://reviewboard.mozilla.org/r/225260/#review231258

r=me if it compiles
Attachment #8956383 - Flags: review?(n.nethercote) → review+
Thank you for the quick review, Nick.

Try is looking good on all platforms.

We're releasing next week, so I'm guessing uplifting to Beta 59 is of low value, as few patches should need uplifting to Release 59 with VS 15.6.
But if needed, it could go as-is with little-to-no risk, just let me know or please go ahead and champion it.
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b9a8c5d66bb9
Rework MakeNotNull to build with VS 2017 15.6 - r=njn
Ryan, assuming this sticks, would you mind continuing your tests with VS 15.6 (I was able to build to the end on my machine, so hopefully it's all good now), and posting a follow-up to your m.d.platform post? Thanks.
Flags: needinfo?(ryanvm)
Ryan - I'm taking some half days this week but I should be around for the next 3 hours and again late afternoon if you need help looking at any additional failures.
Looks like this bug was the only issue. Things look green with it on a new Try push :)
Flags: needinfo?(ryanvm)
I think it's plausible that people outside Mozilla could try to build our current release with the latest Visual Studio, so I think we should keep this on the radar for possible uplift as well. Gerald, can you go ahead and request approval on this so it's up for consideration? Is it safe to say that this patch is effectively a no-op in practice from a risk standpoint?
status-firefox59: wontfix → fix-optional
Flags: needinfo?(gsquelart)
I'm going to set this to affected and track it, in case this is something we need to fix in a dot release for 59.
status-firefox59: fix-optional → affected
tracking-firefox59: --- → +
https://hg.mozilla.org/mozilla-central/rev/b9a8c5d66bb9
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment on attachment 8956383 [details]
Bug 1443367 - Rework MakeNotNull to build with VS 2017 15.6 -

Optional uplift request to 59, if needed:

Approval Request Comment
[Feature/Bug causing the regression]: New VS compiler can't build old code.
[User impact if declined]: Beta/Release 59 patches may be slightly delayed if devs need to manually patch 59 to build with VS 15.6.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not nightly, but built and automatically tested in m-c, which should be enough to prove this build-related change.
[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?]: I don't think so.
[Why is the change risky/not risky?]: Change is no-op in terms of generated code; Static assertions during build; Run-time tests of affected code.
[String changes made/needed]: None.
Flags: needinfo?(gsquelart)
Attachment #8956383 - Flags: approval-mozilla-beta?
Comment on attachment 8956383 [details]
Bug 1443367 - Rework MakeNotNull to build with VS 2017 15.6 -

TBH, my main concern is from external reporters trying to build our latest release rather than troubles backporting patches.
Attachment #8956383 - Flags: approval-mozilla-beta? → approval-mozilla-release?
Wow, MS has already responded and fixed the issue, which should be available in an upcoming release!
https://developercommunity.visualstudio.com/content/problem/209118/vs-2017-156-cl-191326128-now-cant-understand-declt.html
Comment on attachment 8956383 [details]
Bug 1443367 - Rework MakeNotNull to build with VS 2017 15.6 -

(In reply to Gerald Squelart [:gerald] from comment #22)
> Wow, MS has already responded and fixed the issue, which should be available
> in an upcoming release!

Yowza that was fast! I'll try to remember to attempt reverting this patch when testing out future updates on Try. In the mean time, approving this for 59rc2. Thanks for jumping on fixing this so fast :)
Attachment #8956383 - Flags: approval-mozilla-release? → approval-mozilla-release+
(In reply to Dorel Luca [:dluca] from comment #18)
> https://hg.mozilla.org/mozilla-central/rev/b9a8c5d66bb9

This patch is necessary to build FIREFOX_BETA_60_BASE...
Yes, due to the timing of our release cycles. Switch to FIREFOX_NIGHTLY_60_END or use the mozilla-beta repo instead.
You need to log in before you can comment on or make changes to this bug.