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

RESOLVED FIXED in Firefox 54

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: bz, Assigned: xidorn)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox53 affected, firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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...
enable_if_t is a C++14 thing which clang may fail to compile without -std=c++14?
Again, msvc fails the same way.  And it's present in a build-in VS header....
OK, I reproduced this issue...
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?
Flags: needinfo?(mh+mozilla)
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
Flags: needinfo?(mh+mozilla)
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...
> 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...
(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?
Flags: needinfo?(nfroyd)
I met this again when I'm adding new file... This is really annoying :(
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 hidden (mozreview-request)

Comment 13

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

https://reviewboard.mozilla.org/r/114092/#review115572
Attachment #8839431 - Flags: review?(cam) → review+
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.
Flags: needinfo?(nfroyd)
(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

11 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f872b2b4adee
Move #include around to avoid triggering this issue. r=heycam
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.
Assignee: nobody → xidorn+moz
See Also: → bug 1341500
Filed the underlying issue in bug 1341500.

Comment 19

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f872b2b4adee
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.