1459988 - disallow MSVC 15.7+ from being used to build Firefox

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: disallow MSVC 15.7+ from being used to build Firefox

Bikeshedding on the precise error messages welcome.

RyanVM, want to confirm this fails configure?

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8974090 [details] [diff] [review]
disallow MSVC 15.7+ from being used to build Firefox

Yes, this blocks 15.7.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

I think the "right" fix here is to do this patch and also to patch the build system to detect the 15.6 toolchain if installed (it's an optional component available in the Visual Studio Installer). Then we can direct users in the error message and build docs to make sure it's installed.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

To be clear, "C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.13.26128" is the one we want to be using here (which is thankfully the same for all versions of the 15.6 toolset).

Comment 5

[Makoto Kato [:m_kato]]

build is successful with --disable-webrtc, so I would like to allow 15.7 if using --disable-webrtc.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 8974090 [details] [diff] [review]
disallow MSVC 15.7+ from being used to build Firefox

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

This seems fine. I do think maybe the error message should mention the specific issue here (that there's a compiler bug) and link to a bug if one exists.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb8e6bef734f
disallow MSVC 15.7+ from being used to build Firefox; r=ted.mielczarek

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/bb8e6bef734f

Comment 9

[Adrian Wielgosik [:adrian17]]

I added the note to use 15.6 on
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites

Comment 10

[Liqueur Librazy]

tried vs 15.8 and got another error compiling.

21:07.40 c:/mozilla-central/xpcom/ds/nsGkAtoms.cpp(13): error C2131: expression did not evaluate to a constant
21:07.40 mozmake.EXE[5]: *** [c:/mozilla-central/config/rules.mk:1033: nsGkAtoms.obj] Error 2
21:07.41 mozmake.EXE[4]: *** [c:/mozilla-central/config/recurse.mk:74: xpcom/ds/target] Error 2

Comment 11

[Kelsey Gilbert [:jgilbert]]

What is the actual issue here, for the curious?

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Jeff Gilbert [:jgilbert] from comment #11)
> What is the actual issue here, for the curious?

There was bug 1458247 at one point, an ICE in webrtc code; according to bug 1458247 comment 12, 15.8 preview 3 can compile at least esr60, but then crashes on startup.  I don't know whether the webrtc issue is resolved for mozilla-central.

There's also comment 10, which is a flat-out bug.

I think at one point, RyanVM was seeing PGO errors with 15.7 previews?  I know there was https://developercommunity.visualstudio.com/content/problem/211796/internal-compiler-error-building-win64-firefox-w-p.html but I guess that got resolved somewhere along the way, at least with 15.6.  Hard to say if 15.7 or 15.8 are any better, since we haven't been able to do a full build yet.

I don't know whether the webrtc issue and the nsGkAtoms issue have been reported to Microsoft or not.

Comment 13

[Kelsey Gilbert [:jgilbert]]

We should really ensure this gets fixed.

Comment 14

[VT]

Hello,

Will the build system be able to detect and use the older version of MSVC (14.13.26128) when installed side-by-side with the latest one (14.14.26428 in 15.7.5), as described by RyanVM at bug 1459988, comment 3?

I have both versions installed, but would prefer not to uninstall the latest one or wait until Microsoft fixes the MSVC bug(s).

Comment 15

[Nathan Froyd [:froydnj]]

(In reply to VT from comment #14)
> Will the build system be able to detect and use the older version of MSVC
> (14.13.26128) when installed side-by-side with the latest one (14.14.26428
> in 15.7.5), as described by RyanVM at bug 1459988, comment 3?
> 
> I have both versions installed, but would prefer not to uninstall the latest
> one or wait until Microsoft fixes the MSVC bug(s).

We have not implemented this feature yet.

Comment 16

[VT]

(In reply to Nathan Froyd [:froydnj] from comment #15)
> (In reply to VT from comment #14)
> > Will the build system be able to detect and use the older version of MSVC
> > (14.13.26128) when installed side-by-side with the latest one (14.14.26428
> > in 15.7.5), as described by RyanVM at bug 1459988, comment 3?
> > 
> > I have both versions installed, but would prefer not to uninstall the latest
> > one or wait until Microsoft fixes the MSVC bug(s).
> 
> We have not implemented this feature yet.

Are there any plans to implement it in the immediate future or any suggestions on how to manually force the build system to choose a particular MSVC version? 🙂

Comment 17

[Jennifer Yao]

(In reply to Nathan Froyd [:froydnj] from comment #12)
> (In reply to Jeff Gilbert [:jgilbert] from comment #11)
> > What is the actual issue here, for the curious?
> 
> There was bug 1458247 at one point, an ICE in webrtc code; according to bug
> 1458247 comment 12, 15.8 preview 3 can compile at least esr60, but then
> crashes on startup.  I don't know whether the webrtc issue is resolved for
> mozilla-central.
> 
> There's also comment 10, which is a flat-out bug.
> 
> I think at one point, RyanVM was seeing PGO errors with 15.7 previews?  I
> know there was
> https://developercommunity.visualstudio.com/content/problem/211796/internal-
> compiler-error-building-win64-firefox-w-p.html but I guess that got resolved
> somewhere along the way, at least with 15.6.  Hard to say if 15.7 or 15.8
> are any better, since we haven't been able to do a full build yet.
> 
> I don't know whether the webrtc issue and the nsGkAtoms issue have been
> reported to Microsoft or not.

Hi, MSVC compiler dev here. Regarding comment 10, I think I may know what is causing that issue -- from what I've seen (without pulling the entire project), the error is being emitted for a constexpr variable whose initializer contains 'offsetof'. We've seen similar diagnostics emitted when projects redefine their own 'offsetof' macro to an expression containing a 'reinterpret_cast' or a C-style cast that is interpreted as a 'reinterpret_cast'; per the C++ standard, 'reinterpret_cast' is not permitted in constant expressions.

May I ask whether you are still experiencing this issue? And, if so, whether your project uses the C library 'offsetof' (or another one that you've defined elsewhere)?

If a non-conforming 'offsetof' is indeed the cause, the solution would be to use '__builtin_offsetof' (which is usable in constant expressions).

Additionally, for this bug and any other MSVC bugs that you are experiencing (including the ones you mentioned), I suggest filing a report in Developer Community (https://developercommunity.visualstudio.com/content/problem/post.html?space=62). We are always looking for ways to improve our compiler, and your feedback would be greatly appreciated.

Jennifer Yao, Visual C++ Team

Comment 18

[Mike Hommey [:glandium]]

About comment 10, the code was:

extern constexpr GkAtoms gGkAtoms = {
  #define GK_ATOM(name_, value_) NS_STATIC_ATOM_INIT_STRING(value_)
  #include "nsGkAtomList.h"
  #undef GK_ATOM
  {
    #define GK_ATOM(name_, value_) \
      NS_STATIC_ATOM_INIT_ATOM(nsStaticAtom, GkAtoms, name_, value_)
    #include "nsGkAtomList.h"
    #undef GK_ATOM
  }
};

with NS_STATIC_ATOM_INIT_STRING defined as:
  #define NS_STATIC_ATOM_INIT_STRING(value_) \
    u"" value_,

and NS_STATIC_ATOM_INIT_ATOM as:
  #define NS_STATIC_ATOM_INIT_ATOM(type_, detailClass_, name_, value_) \
    type_(u"" value_, \
        sizeof(value_) - 1, \
        offsetof(detailClass_, \
                 mAtoms[static_cast<size_t>(detailClass_::Atoms::name_)]) - \
        offsetof(detailClass_, name_##_string)),


Note that this code has changed in the meanwhile, so the issue may not happen anymore. But indeed, that was using offsetof. It doesn't look like that offsetof was coming from a #define, though.