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

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Alessio Placitelli [:Dexter]]

Attachment: 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.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

For the Linux failure, I think you need GCC 4.7.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

If that's a build requirement now then we should probably have the configure script fail early with an appropriate error message.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (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.

Comment 6

[Seth Fowler [:seth] [:s2h]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 7

[Seth Fowler [:seth] [:s2h]]

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.

Comment 8

[Seth Fowler [:seth] [:s2h]]

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.

Comment 9

[Seth Fowler [:seth] [:s2h]]

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.

Comment 10

[(Away)]

Yeah - I get the error with 2012 but not 2013.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Expand MSVC version bounds for decltype scope operator workaround

Here's a suitable patch for MSVC.

Comment 12

[Alessio Placitelli [:Dexter]]

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.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I can verify the patch fixes build breakage on Ubuntu 12.04.1.

Comment 14

[Robert Kaiser]

(In reply to James Willcox (:snorp) ([e-mail]) 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.

Comment 16

[Juan Gomez [:_AtilA_] (CET/CEST)]

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.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Please don't remove blo

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

cking bugs.

Comment 20

[Jeff Walden [:Waldo]]

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.

Comment 21

[Seth Fowler [:seth] [:s2h]]

Thanks for the review, Waldo!

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5cc69b88c4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/20bd30f0fb5c

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d5cc69b88c4c
https://hg.mozilla.org/mozilla-central/rev/20bd30f0fb5c

Comment 24

[Ralph Giles (:rillian)]

Confirming this fixes the flatfish build as well. Thanks!