923170 - invalid operands of types ‘DrawMode’ and ‘int’ to binary ‘operator!=’

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

I can no longer make Linux desktop builds on Ubuntu 11.04. This is an entirely standard config, with only "ac_add_options --disable-gstreamer" in the .mozconfig.

gcc version 4.5.2, 64-bit Linux

 9:41.76 /home/morbo/hg/mozilla-central/layout/svg/nsSVGGlyphFrame.cpp: In member function ‘virtual nsresult nsSVGGlyphFrame::PaintSVG(nsRenderingContext*, const nsIntRect*, nsIFrame*)’:
 9:41.77 /home/morbo/hg/mozilla-central/layout/svg/nsSVGGlyphFrame.cpp:425:20: error: invalid operands of types ‘DrawMode’ and ‘int’ to binary ‘operator!=’

It looks like fallout from bug 921753.

Comment 1

[Bill Gianopoulos [:WG9s]]

IF this impacts developers other than me, then I think the severity needs to be changed from critical to blocker.

Comment 2

[Bill Gianopoulos [:WG9s]]

Until this is fixed I am no longer producing Linux builds.  Just Windows and Android for now.

Comment 3

[Bill Gianopoulos [:WG9s]]

I am setting it to blocker.  can be overruled by others.

Comment 4

[Bill Gianopoulos [:WG9s]]

Might not be bad, gives me a non changing thing to do the fair presentation at the summit i guess.

Comment 5

[(no longer active)]

Bill, can you please check if the MOZ_HAVE_CXX11_STRONG_ENUMS macro is #defined inside mfbt/TypedEnum.h?

Also, what's on line 425 on that file for you?  For me, it's:

  if (int(drawMode)) {

I don't see operator!= on that line at all. :/

Comment 6

[Gian-Carlo Pascutto [:gcp]]

1) MOZ_HAVE_CXX11_STRONG_ENUMS is defined (GCC 4.5.2, I put an #error in there to check)
2) The line is indeed the one you show.

Comment 7

[Bill Gianopoulos [:WG9s]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #6)
> 1) MOZ_HAVE_CXX11_STRONG_ENUMS is defined (GCC 4.5.2, I put an #error in
> there to check)
> 2) The line is indeed the one you show.

Same here with GCC 4.5.1.

Comment 8

[Bill Gianopoulos [:WG9s]]

If I am reading the code correctly, it is impossible for drawMode to be negative here.

Replacing

     if (int(drawMode)) {

with

     if (int(drawMode) > 0) {

appears to avoid the issue.

Comment 9

[Bill Gianopoulos [:WG9s]]

Reducing severity since there is a workaround.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Does anyone understand what's happening from a C++(11) perspective here?

Comment 11

[Bill Gianopoulos [:WG9s]]

Not really, but I suspect that perhaps since the class definition does not seem to specify 0 as a valid value, perhaps a zero non-zero test does not make sense.

Comment 12

[Gian-Carlo Pascutto [:gcp]]

It's a GCC 4.5.x bug. That version is supposed to have most strong enum stuff fixed starting from 4.5.1, which is why we enable them for it, but:

http://ideone.com/Sp2tW1    (compiles and works in gcc 4.8.1)

morbo@ubox:~$ gcc -std=c++0x -o test gcc452.cpp 
gcc452.cpp: In function ‘int main()’:
gcc452.cpp:12:18: error: invalid operands of types ‘DrawMode’ and ‘int’ to binary ‘operator!=’

Based on this, I'll make a patch to bump our support for using strong enums to require at least GCC 4.6.0. We have a fallback in place for older GCC anyway, and that should prevent this issue from reoccurring elsewhere in the code.

Comment 13

[(no longer active)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #12)
> Based on this, I'll make a patch to bump our support for using strong enums
> to require at least GCC 4.6.0. We have a fallback in place for older GCC
> anyway, and that should prevent this issue from reoccurring elsewhere in the
> code.

Sounds good to me, thanks!  Can you verify that this program compiles properly with gcc 4.6?

Comment 14

[Gian-Carlo Pascutto [:gcp]]

It works with gcc 4.6.3. I think I'd have to actually compile a gcc 4.6.0 to verify it works for that. We can either bump to 4.6.3 directly, or bump to 4.6.0 and see if someone with gcc >=4.6.0<4.6.3 starts complaining in this bug? :)

Comment 15

[(no longer active)]

(In reply to comment #14)
> It works with gcc 4.6.3. I think I'd have to actually compile a gcc 4.6.0 to
> verify it works for that. We can either bump to 4.6.3 directly, or bump to
> 4.6.0 and see if someone with gcc >=4.6.0<4.6.3 starts complaining in this bug?
> :)

I think we should just enable this on gcc 4.6.3.  It's not worth investigating too much into this, IMO.  :-)

Comment 16

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Disable strong enums on older GCC versions.

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 814237 [details] [diff] [review]
Patch 1. Disable strong enums on older GCC versions.

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

rs=me

Comment 18

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/657aa2568bc2

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 814237 [details] [diff] [review]
Patch 1. Disable strong enums on older GCC versions.

>                 GraphicsFilter filter =
>-                    isDownscale ? aDefaultFilter : GraphicsFilter::FILTER_FAST;
>+                    isDownscale ? aDefaultFilter : (const GraphicsFilter)GraphicsFilter::FILTER_FAST;
>                 aPattern->SetFilter(filter);

This part seems unrelated to the version-bump.  Did some compiler choke on this patch without this?

(Just pushing back slightly because the C-style cast is sadmaking... const_cast<> would be nicer & get us more type-safety, if we really do need a cast there.)

Comment 20

[Gian-Carlo Pascutto [:gcp]]

>Did some compiler choke on this patch without this?

The fallback path for <4.6.3 has a type mismatch without this cast. You wouldn't have seen this before unless you were compiling m-c with gcc 4.4. It got exposed because more people will be using the fallback now.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/657aa2568bc2