1331102 - STL wrappers can fail to compile when <xutility> is involved

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This may be similar to bug 981264, but is happening on tip and including <new> does NOT seem to fix the problem.

Specifically, if I add a file early to UNIFIED_SOURCES list in layout/style, this makes nsStyleCoord.cpp move to the next unified filem and then I get a failure like so (with clang, because it shows the include chain, but msvc fails the same way without telling me the include chain):


20:09:12     INFO -  In file included from z:/task_1484336621/build/src/obj-firefox/layout/style/Unified_cpp_layout_style4.cpp:2:
20:09:12     INFO -  In file included from z:/task_1484336621/build/src/layout/style/nsStyleCoord.cpp:8:
20:09:12     INFO -  In file included from z:/task_1484336621/build/src/layout/style/nsStyleCoord.h:11:
20:09:12     INFO -  In file included from z:/task_1484336621/build/src/obj-firefox/dist/stl_wrappers\type_traits:25:
20:09:12     INFO -  In file included from z:/task_1484336621/build/src/obj-firefox/dist/include\mozilla/throw_msvc.h:12:
20:09:12     INFO -  In file included from z:/task_1484336621/build/src/obj-firefox/dist/include\mozilla/msvc_raise_wrappers.h:40:
20:09:12     INFO -  In file included from z:/task_1484336621/build/src/obj-firefox/dist/stl_wrappers\xutility:63:
20:09:12     INFO -  In file included from z:\task_1484336621\build\src\vs2015u3\VC\include\xutility:8:
20:09:12     INFO -  In file included from z:/task_1484336621/build/src/obj-firefox/dist/stl_wrappers\utility:63:
20:09:12     INFO -  z:\task_1484336621\build\src\vs2015u3\VC\include\utility(83,11):  error: no template named 'enable_if_t'; did you mean 'enable_if'?
20:09:12     INFO -                  class = enable_if_t<is_default_constructible<_Uty1>::value
20:09:12     INFO -                          ^
20:09:12     INFO -  z:\task_1484336621\build\src\vs2015u3\VC\include\xtr1common(57,9):  note: 'enable_if' declared here
20:09:12     INFO -          struct enable_if
20:09:12     INFO -                 ^

For now I'm working around this by adding the new file I wanted to add to SOURCES instead of UNIFIED_SOURCES, but this seems like an annoying footgun.

I did try adding "#include <new>" to msvc_raise_wrappers.h, and that did not help.

I have no idea who owns our Windows build stuff nowadays...

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

enable_if_t is a C++14 thing which clang may fail to compile without -std=c++14?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Again, msvc fails the same way.  And it's present in a build-in VS header....

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

OK, I reproduced this issue...

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

The workaround is adding "#include <new>" to the top of nsStyleCoord.cpp, not msvc_raise_wrappers.h.

Given this mess, I guess we should probably add something to the top of every unified source file to include the msvc wrappers in advance.

WDYT, glandium?

Comment 5

[Mike Hommey [:glandium]]

It would happen just as much if nsStyleCoord.cpp wasn't in UNIFIED_SOURCES, so adding something to the top of every unified source file is not really going to cut it.

> 20:09:12     INFO -  In file included from z:/task_1484336621/build/src/obj-firefox/dist/include\mozilla/msvc_raise_wrappers.h:40:

Something doesn't add up here... it should be line 36, not 40. Have you modified this file?
https://dxr.mozilla.org/mozilla-central/source/memory/mozalloc/msvc_raise_wrappers.h#36

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Well, non-unified sources are much fewer than unified sources, and this kind of issue would not appear just because a new file is added.

But yeah, it would be the best if the fundamental issue can be fixed...

Comment 7

[Boris Zbarsky [:bzbarsky]]

> it should be line 36, not 40. Have you modified this file?

Sorry, that's this part:

  I did try adding "#include <new>" to msvc_raise_wrappers.h, and that did not help.

The comment I had for that addition and the "#include <new>" itself pushed things down a few lines.  I hadn't realized I was giving a stack from my try run with that attempt, not from the vanilla tree...

Comment 8

[Manish Goregaokar [:manishearth]]

Hitting this today in bug 1338936.

See failures in https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2d887234705f65d86a6f2b4e7780c14e7aca7dd4&selectedJob=78619753 , they're caused by the first commit.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #6)
> Well, non-unified sources are much fewer than unified sources, and this kind
> of issue would not appear just because a new file is added.
> 
> But yeah, it would be the best if the fundamental issue can be fixed...

Given that people are adding lots of new files in quantum projects, and consequently there is increasing chance for developers to hit this, I think adding a workaround is better than nothing, if the fundamental issue is not going to be fixed soonish.

Nanthan, any thoughts for this issue?

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

I met this again when I'm adding new file... This is really annoying :(

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Interestingly, it seems if I put both BindingStyleRule.cpp and ServoSpecifiedValues.cpp back to UNIFIED_SOURCES, everything works again. I've pushed a try run to see if it matches my local result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=70945b8c40216a40e6c27ddd91b718204a532f53

I guess it is because the file in question is further pushed down in its unified source file, so the issue suspends. Once the file is back to the top of the next unified source, this would happen again... but anyway, we can add several more files without worrying about it for a while.

This issue still needs a solution, though.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1331102 - Move #include around to avoid triggering this issue.

Review commit: https://reviewboard.mozilla.org/r/114092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114092/

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8839431 [details]
Bug 1331102 - Move #include around to avoid triggering this issue.

https://reviewboard.mozilla.org/r/114092/#review115572

Comment 14

[Nathan Froyd [:froydnj]]

Just to make sure I understand the problem:

1. We include some standard library header.
2. ...which actually includes our wrapper header
3. ...which pulls in *other* standard library headers
4. ...which actually includes our wrapper header
5. ...which then includes the real library header
6. ...which then get confused that the definitions they need don't exist (?)

I'm a little unclear on the problem from reading the first couple of comments.

Two thoughts come to mind:

1. We could use mozilla::EnableIf in these style headers, like we're supposed to be doing.
2. Once we've wrapped a "top-level" STL include, do we need to wrap any inclusions from that as well?  Would have to think through this a little bit.

Comment 15

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] from comment #14)
> 1. We could use mozilla::EnableIf in these style headers, like we're
> supposed to be doing.

The error is on enable_if_t in a system header (utility), not a style header. The question becomes, where is enable_if_t normally defined, and why is it not defined?

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f872b2b4adee
Move #include around to avoid triggering this issue. r=heycam

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

The patch should fix this bug and it shouldn't happen again for a while. But it could show up again if someone put <type_traits> (or something else) on the top of some file.

Let's have the bug closed with my patch. I'll file a new bug for the underlying issue.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Filed the underlying issue in bug 1341500.

Comment 19

[Carsten Book [:Tomcat]]

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