Closed Bug 1296737 Opened 4 years ago Closed 3 years ago

make clang-cl compile with a C++14-compatible mode

Categories

(Firefox Build System :: General, defect)

All
Windows
defect
Not set

Tracking

(firefox51 wontfix, firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(1 file, 2 obsolete files)

MSVC 2015's <complex> uses C++14-specific things without any sort of guard.  clang-cl can handle said <complex>, but we tell clang-cl to compile with -std=gnu++11 at the moment, which breaks the build.  We need to fix that.
This should be fixed in clang-cl itself, right?
Flags: needinfo?(nfroyd)
(In reply to :Ehsan Akhgari from comment #1)
> This should be fixed in clang-cl itself, right?

No.  We're the ones telling clang-cl to compile with -std=gnu++11, which isn't compatible with MSVC's headers.  So the fix is on our side.

It looks like clang-cl itself was fixed to turn on C++14 features when emulating MSVC 2015 a while ago: https://llvm.org/bugs/show_bug.cgi?id=23023
Flags: needinfo?(nfroyd)
Then we should start by making clang-cl emulate MSVC 2015, it's currently still emulating 2013.
(In reply to Nathan Froyd [:froydnj] from comment #2)
> (In reply to :Ehsan Akhgari from comment #1)
> > This should be fixed in clang-cl itself, right?
> 
> No.  We're the ones telling clang-cl to compile with -std=gnu++11, which
> isn't compatible with MSVC's headers.  So the fix is on our side.

We should never pass -std to MSVC.  Why are passing it to clang-cl??

I think the fix here is to adjust -fms-compatibility-version to the correct MSVC 2015 version.

> It looks like clang-cl itself was fixed to turn on C++14 features when
> emulating MSVC 2015 a while ago: https://llvm.org/bugs/show_bug.cgi?id=23023

OK, this was what I thought you were saying is going on!
(In reply to :Ehsan Akhgari from comment #4)
> (In reply to Nathan Froyd [:froydnj] from comment #2)
> > (In reply to :Ehsan Akhgari from comment #1)
> > > This should be fixed in clang-cl itself, right?
> > 
> > No.  We're the ones telling clang-cl to compile with -std=gnu++11, which
> > isn't compatible with MSVC's headers.  So the fix is on our side.
> 
> We should never pass -std to MSVC.  Why are passing it to clang-cl??

We're never passing it to MSVC. We're passing it to clang-cl because the current toolchain checks in configure *want* clang and gcc __cplusplus version to be 201103 (and MSVC lies about it, which is why it's singled out)
Comment on attachment 8785420 [details]
Bug 1296737 - make clang-cl compile in c++14 mode;

https://reviewboard.mozilla.org/r/74618/#review73072

::: build/moz.configure/toolchain.configure:275
(Diff revision 1)
>  
>      result = try_preprocess(compiler, language, check)
>  
>      if not result:
>          raise FatalCheckError(
> -            'Unknown compiler or compiler not supported.')
> +            'Unknown compiler or compiler not supported. (failed to execute?)')

I'm not convinced the changes you've made to the error messages make things much better, so I'd rather bikeshed in a separate bug than block the main patch because of this.

::: build/moz.configure/toolchain.configure:354
(Diff revision 1)
> +        # MSVC 2015 headers include C++14 features, but don't guard them
> +        # with appropriate checks.
> +        if info.type == 'clang-cl' and info.language_version != 201402:
> +            append_flag('-std=gnu++14')
>  
>      # We force clang-cl to emulate Visual C++ 2013 Update 3 with fallback to

This comment needs an update.
Attachment #8785420 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #7)
> Comment on attachment 8785420 [details]
> Bug 1296737 - make clang-cl compile in c++14 mode;
> 
> https://reviewboard.mozilla.org/r/74618/#review73072
> 
> ::: build/moz.configure/toolchain.configure:275
> (Diff revision 1)
> >  
> >      result = try_preprocess(compiler, language, check)
> >  
> >      if not result:
> >          raise FatalCheckError(
> > -            'Unknown compiler or compiler not supported.')
> > +            'Unknown compiler or compiler not supported. (failed to execute?)')
> 
> I'm not convinced the changes you've made to the error messages make things
> much better, so I'd rather bikeshed in a separate bug than block the main
> patch because of this.

Gah, sorry, these changes were really debugging so I could figure out which error case was being hit.  I didn't really intend for them to be in this patch, though I do think they make things a bit more debuggable.  Will remove.
Doesn't clang-cl turn on C++14 mode when you're passing -fms-compatibility-version=19.00.23918 to it?  It really should.  If we ever need to pass an -std flag to clang-cl, we're doing things the wrong way.  :(

If -fms-compatibility-version=19.00.23918 doesn't turn on C++14, can you please file an issue upstream?
Flags: needinfo?(nfroyd)
I can try that.

I attempted to fix up the tests in this revision, but apparently the tests are not correct, according to try, for reasons that I do not understand.  I can't run the tests locally, either, because they complain about "C:\path\to\foo" not being equal to "c:\path\to\foo"...which is a different error than I get on try.  How can static tests that don't interact with the filesystem be so flaky?
Flags: needinfo?(nfroyd)
Comment on attachment 8785420 [details]
Bug 1296737 - make clang-cl compile in c++14 mode;

https://reviewboard.mozilla.org/r/74618/#review73454

::: build/moz.configure/toolchain.configure:352
(Diff revision 2)
> -        if info.type in ('clang-cl', 'clang', 'gcc'):
> +        if info.type in ('clang', 'gcc') and info.language_version != 201103:
>              append_flag('-std=gnu++11')
> +        # MSVC 2015 headers include C++14 features, but don't guard them
> +        # with appropriate checks.
> +        if info.type == 'clang-cl' and info.language_version != 201402:
> +            append_flag('-std=gnu++14')

Come to think of it, /maybe/ this should be c++14 for clang-cl (i.e. no GNU extensions).

::: build/moz.configure/toolchain.configure:679
(Diff revision 2)
>                     info.target_endianness or 'unknown', host_or_target_str,
>                     host_or_target.endianness))
>  
>          if info.flags:
>              raise FatalCheckError(
> -                'Unknown compiler or compiler not supported.')
> +                'Unknown compiler or compiler not supported. ' + str(info.flags))

leftover here.

::: python/mozbuild/mozbuild/test/configure/test_toolchain_configure.py:221
(Diff revision 2)
>      '_WIN64': 1,
>  }
>  
>  # Note: In reality, the -std=gnu* options are only supported when preceded by
>  # -Xclang.
> -CLANG_CL_3_9 = (CLANG_BASE('3.9.0') + VS('18.00.00000') + DEFAULT_C11 +
> +CLANG_CL_3_9 = (CLANG_BASE('3.9.0') + VS('19.00.00000') + DEFAULT_C11 +

this should remain 18, that's what clang-cl default to when you don't set -fms-compatibility-version.

::: python/mozbuild/mozbuild/test/configure/test_toolchain_configure.py:781
(Diff revision 2)
>          type='clang-cl',
>          compiler='/usr/bin/clang-cl',
>          language='C',
>      )
>      CLANGXX_CL_3_9_RESULT = CompilerResult(
> -        flags=['-fms-compatibility-version=18.00.30723', '-fallback'],
> +        flags=['-fms-compatibility-version=19.00.23918', '-fallback'],

You should be getting an error here, because the first time the configure check runs, you'd be adding -std=gnu++14, which the fake compiler doesn't support, so the second time the configure check runs, it should fail because of -std=gnu++14 not being supported.
Attachment #8785420 - Flags: review?(mh+mozilla)
Also note that conveniently, you can run python/mozbuild/mozbuild/test/configure/test_toolchain_configure.py on any platform, and it will run all the same tests. Fake compilers FTW.
Duplicate of this bug: 1268607
OK, this passes tests and I think addresses comments as well.
Attachment #8793439 - Flags: review?(mh+mozilla)
Attachment #8785420 - Attachment is obsolete: true
Comment on attachment 8793439 [details] [diff] [review]
make clang-cl compile in c++14 mode

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

::: build/moz.configure/toolchain.configure
@@ +339,5 @@
>              append_flag('-std=gnu++11')
> +        # MSVC 2015 headers include C++14 features, but don't guard them
> +        # with appropriate checks.
> +        if info.type == 'clang-cl' and info.language_version != 201402:
> +            append_flag('-std=gnu++14')

cf. previous review, this might be better with c++14 (no GNU extensions)
Attachment #8793439 - Flags: review?(mh+mozilla)
Now with -std=c++14.
Attachment #8795850 - Flags: review?(mh+mozilla)
Attachment #8793439 - Attachment is obsolete: true
Attachment #8795850 - Flags: review?(mh+mozilla) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b26d191d84e
make clang-cl compile in c++14 mode; r=glandium
https://hg.mozilla.org/mozilla-central/rev/3b26d191d84e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.