Regression: mfbt/tests/TestMaybe.cpp (from bug 913586) fails to compile

RESOLVED FIXED in mozilla34

Status

()

Core
MFBT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Dexter, Assigned: seth)

Tracking

Trunk
mozilla34
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

4 years ago
Created attachment 8477987 [details]
build_log_extract.txt

The TestMaybe.cpp, introduced on m-c in rev 7283263a1974 (http://hg.mozilla.org/mozilla-central/rev/7283263a1974) from bug 913586, fails to compile on my system with Microsoft Visual Studio 2012.
Flags: needinfo?(seth)

Updated

4 years ago
Duplicate of this bug: 1057865
Created attachment 8478363 [details]
build failure log on Linux

Doing a build on linux with |mach build| also hit this error. Compiler version is:

c++ (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3
For the Linux failure, I think you need GCC 4.7.
If that's a build requirement now then we should probably have the configure script fail early with an appropriate error message.
(Assignee)

Comment 5

4 years ago
Created attachment 8478631 [details] [diff] [review]
(Part 1) - Expand GCC version bounds for decltype scope operator workaround

Here's a trivial patch for the linux failure. This is a known GCC bug; I just needed to expand the version bounds I used for the workaround.
Attachment #8478631 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

4 years ago
Assignee: nobody → seth
Status: NEW → ASSIGNED
(Assignee)

Comment 6

4 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> If that's a build requirement now then we should probably have the configure
> script fail early with an appropriate error message.

It's not a build requirement, don't worry.
Flags: needinfo?(seth)
(Assignee)

Comment 7

4 years ago
Alessio, could you help me debug this? What you're encountering is a known Visual Studio 2012 compiler bug which there is already a workaround for in TestMaybe.cpp, located here:

http://mxr.mozilla.org/mozilla-central/source/mfbt/tests/TestMaybe.cpp#35

It's not clear to me why this workaround doesn't work for you, even though it does work on our build infrastructure, which also uses Visual Studio 2012. Maybe for some reason Compiler.h is getting the version of your compiler wrong? You could check this by putting a #error directive after line 35 and seeing whether you hit it.
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 8

4 years ago
Kyle, maybe you're also seeing this on Visual Studio 2012. If you could check whether Compiler.h is detecting your compiler correctly (by putting a #error directive after line 35 of TestMaybe.cpp, as I suggested to Alessio in comment 7), it would be helpful to me in understanding what's going on here.
Flags: needinfo?(khuey)
(Assignee)

Updated

4 years ago
Flags: needinfo?(khuey)
(Assignee)

Comment 9

4 years ago
Actually, I suspect all that's happening here is that I'm getting confused about Visual Studio versions. Our infrastructure is using Visual Studio *2010* (version 10), and the code assumes that the bug got fixed in *VS2012* (version 11), but it probably really got fixed in *VS2013* (version 12). Thus probably all I need to do is bump the version bound for Visual Studio as well. I'll upload another patch.
Flags: needinfo?(alessio.placitelli)
Yeah - I get the error with 2012 but not 2013.
(Assignee)

Comment 11

4 years ago
Created attachment 8478767 [details] [diff] [review]
(Part 2) - Expand MSVC version bounds for decltype scope operator workaround

Here's a suitable patch for MSVC.
Attachment #8478767 - Flags: review?(jwalden+bmo)
(Reporter)

Comment 12

4 years ago
Thanks seth, this fixes the problem.

(In reply to Seth Fowler [:seth] from comment #11)
> Created attachment 8478767 [details] [diff] [review]
> (Part 2) - Expand MSVC version bounds for decltype scope operator workaround
> 
> Here's a suitable patch for MSVC.
I can verify the patch fixes build breakage on Ubuntu 12.04.1.

Comment 14

4 years ago
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #3)
> For the Linux failure, I think you need GCC 4.7.

I know you're patching this anyhow, but just FWIW, gcc 4.6 is required for ICS-based B2G devices/builds. So as long as we do not want to officially unsupport those completely, we unfortunately need to keep gcc 4.6 working.
Duplicate of this bug: 1057742
It's breaking Flatfish builds too (it uses gcc-4.6 based toolchain), the patch provided by Seth is working for me too.
I checked too that gcc-4.7 doesn't need the workaround.
Blocks: 903304
No longer blocks: 913586

Updated

4 years ago
Duplicate of this bug: 1058434
Comment on attachment 8478631 [details] [diff] [review]
(Part 1) - Expand GCC version bounds for decltype scope operator workaround

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

::: mfbt/tests/TestMaybe.cpp
@@ +34,5 @@
>       template<typename T> struct Identity { typedef T type; };
>  #    define DECLTYPE(EXPR) Identity<decltype(EXPR)>::type
>  #  endif
>  #elif MOZ_IS_GCC
> +#  if MOZ_GCC_VERSION_AT_LEAST(4,7,0)

Make this (4, 7, 0) with spaces, for readability.
Attachment #8478631 - Flags: review?(jwalden+bmo) → review+

Updated

4 years ago
Attachment #8478767 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/d5cc69b88c4c
https://hg.mozilla.org/mozilla-central/rev/20bd30f0fb5c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Confirming this fixes the flatfish build as well. Thanks!
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.