938092 - Building with gcc4.5 fails if both --enable-webrtc and --enable-tests

Status: VERIFIED FIXED

(Core :: WebRTC, defect)

Description

[Bill Gianopoulos [:WG9s]]

Building Firefox with gcc4.5 fails if you enable tests and webrtc with the following:

/home/wag/mozilla/mozilla2/media/webrtc/trunk/testing/gtest/include/gtest/gtest.h:1532:136: error: ISO C++ forbids comparison between pointer and integer
make[5]: *** [rlogringbuffer_unittest.o] Error 1
make[5]: Leaving directory `/home/wag/mozilla/mozilla2/fx-obj/media/mtransport/test'
make[4]: *** [media/mtransport/test/compile] Error 2
make[4]: *** Waiting for unfinished jobs....
make[5]: Leaving directory `/home/wag/mozilla/mozilla2/fx-obj/webapprt/gtk2'
make[4]: Leaving directory `/home/wag/mozilla/mozilla2/fx-obj'
make[3]: *** [compile] Error 2
make[3]: Leaving directory `/home/wag/mozilla/mozilla2/fx-obj'

Comment 1

[Bill Gianopoulos [:WG9s]]

A bit more of the build log to see all the messages:

/home/wag/mozilla/mozilla2/media/mtransport/test/rlogringbuffer_unittest.cpp: In member function ‘virtual void RLogRingBufferTest_TestGetFree_Test::TestBody()’:
/home/wag/mozilla/mozilla2/media/mtransport/test/rlogringbuffer_unittest.cpp:54:151: warning: passing NULL to non-pointer argument 3 of ‘testing::AssertionResult testing::internal::CmpHelperNE(const char*, const char*, const T1&, const T2&) [with T1 = long int, T2 = mozilla::RLogRingBuffer*]’
/home/wag/mozilla/mozilla2/media/mtransport/test/rlogringbuffer_unittest.cpp:54:151: warning: passing NULL to non-pointer argument 3 of ‘testing::AssertionResult testing::internal::CmpHelperNE(const char*, const char*, const T1&, const T2&) [with T1 = long int, T2 = mozilla::RLogRingBuffer*]’
In file included from /home/wag/mozilla/mozilla2/media/mtransport/test/rlogringbuffer_unittest.cpp:17:0:
/home/wag/mozilla/mozilla2/media/webrtc/trunk/testing/gtest/include/gtest/gtest.h: In function ‘testing::AssertionResult testing::internal::CmpHelperNE(const char*, const char*, const T1&, const T2&) [with T1 = long int, T2 = mozilla::RLogRingBuffer*]’:
/home/wag/mozilla/mozilla2/media/mtransport/test/rlogringbuffer_unittest.cpp:54:151:   instantiated from here
/home/wag/mozilla/mozilla2/media/webrtc/trunk/testing/gtest/include/gtest/gtest.h:1532:136: error: ISO C++ forbids comparison between pointer and integer
make[5]: *** [rlogringbuffer_unittest.o] Error 1

Comment 3

[Jeff Walden [:Waldo]]

Given compiler requirements these days, nullptr is the real deal everywhere except with gcc < 4.6.  There, it's emulated as __null.  For type-deduction purposes and such, as here, __null is either int or long (32/64).  So that's why integer types here.  (Note that if you pass __null to an int/long parameter, that triggers gcc 4.5's warning about comparing pointer to integer, seen at start of comment 1.  In most Mozilla code that's a compile error because of -Werror=conversion-null, but I guess this is in the DMZ and so gets away with it.)  Then the comparison dies, pretty much for the reason stated on the box.

If we're fine modifying gtest.h, or doing a local patch -- which I guess we must be, because we did nullptr here -- we sort of might be able to do some overloading to ameliorate the problem here.  Maybe.  It would probably require something screwy to admit non-same-type comparisons where one side's pointer and the other side is int/long, and it'd be messy, but I think it's doable.

Comment 4

[(no longer active)]

EXPECT_EQ and ASSERT_EQ seem to be able to deal with this.  Perhaps we can extend EXPECT_NE and ASSERT_NE similarly?

Comment 5

[Jeff Walden [:Waldo]]

Huh:

#define EXPECT_EQ(expected, actual) \
  EXPECT_PRED_FORMAT2(::testing::internal:: \
                      EqHelper<GTEST_IS_NULL_LITERAL_(expected)>::Compare, \
                      expected, actual)

Looks like they already hit this problem there, so, yeah, should be possible.

Comment 6

[(no longer active)]

It would be nice if whoever decides to work on this bug would upstream the gtest fix.

Comment 7

[Bill Gianopoulos [:WG9s]]

Well, I just built an entirely new build system so I am no longer building on GCC 4.5 but am up to 4.8 so not sure if I can help here.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: webrtc-nullptr.diff

I needed this to be able to test a patch against gcc 4.5.  :-(

This is total awfulness copypasta.  But it appears to work with gcc 4.5, in that it compiles.  Tell me what to do to land this!  (Perhaps upstream, but given when this code was last pulled, it seems likely to me that upstreaming isn't the fast path to landing it and making it work, perhaps.)

Currently tryservering here, along with a bunch of other work:

https://tbpl.mozilla.org/?tree=Try&rev=44e3e2e6d35d

Comment 9

[Timothy B. Terriberry (:derf)]

Comment on attachment 8355401 [details] [diff] [review]
webrtc-nullptr.diff

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

A -U3 diff, Waldo?

I didn't realize supporting 4.5.x was still a goal. It's been broken for a while. Even I migrated to 4.7.3 instead of trying to fix it.

But it looks like this does the job.

::: media/webrtc/trunk/testing/gtest/include/gtest/gtest.h
@@ +1500,4 @@
>    }
>  };
>  
> +// The helper function for {ASSERT|EXPECT}_NE.

The "right" thing to do is probably to define a new variant of the GTEST_IMPL_CMP_HELPER_() macro that merges this with the Eq helper (AFAICT they are identical except for the op). But given the C++ ridiculousness that makes any of this necessary at all and the fact that it won't make the patch any smaller, I won't insist on that.

@@ +2039,5 @@
>                        EqHelper<GTEST_IS_NULL_LITERAL_(expected)>::Compare, \
>                        expected, actual)
> +#define EXPECT_NE(val1, val2) \
> +  EXPECT_PRED_FORMAT2(::testing::internal:: \
> +                      NeHelper<GTEST_IS_NULL_LITERAL_(val1)>::Compare, \

I'd've appreciated a comment that GTEST_IS_NULL_LITERAL_() wraps its parameter in sizeof(), so that it does not actually get evaluated twice. I know the corresponding code in EXEPECT_EQ() doesn't have one, but paranoia required me to check manually.

Comment 10

[Timothy B. Terriberry (:derf)]

Jesup, what should we do to get a change like this upstream?

Comment 11

[Gian-Carlo Pascutto [:gcp]]

>I didn't realize supporting 4.5.x was still a goal. It's been broken for a while. Even I migrated to 4.7.3 instead of trying to fix it

We claim to support back to gcc 4.4. I don't know what the "goal" is but aside from this bug it does still work. Everything on tbpl is gcc 4.6 or later though, so problems aren't spotted until someone actually tries with that compiler.

Comment 12

[Frank Wein [:mcsmurf]]

Can we check this patch in? :)

Comment 13

[Timothy B. Terriberry (:derf)]

Yes, the upstream issues can be sorted later (leaving bug 864513 blocking this one so we don't forget it).

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Agreed, check it in.  Filing an Issue at webrtc.org and (optional but preferred) uploading a patch to the webrtc.org trunk for code-review is all that's needed (record Issue here).  Thanks Waldo/Tim!

Comment 15

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46f4a621a23d

Comment 16

[Jeff Walden [:Waldo]]

https://groups.google.com/d/msg/googletestframework/vsUgFH09KVM/WEhnuqjYRRcJ for the upstream (in gtest, actually) issue.  No issue because googletest's issue tracker spits out a big warning in the default message saying they don't actively track issues there, so sending mail was the correct action to take.  I actually noodled on an upstream fix some on Sunday, but I ran into minor issues, and the more major issue of probably needing to include tests in it, and I decided it just wasn't worth the time.  (Particularly not if we require 4.6 to compile by then.)

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/46f4a621a23d

Comment 19

[Frank Wein [:mcsmurf]]

Comment on attachment 8355401 [details] [diff] [review]
webrtc-nullptr.diff

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 784739
User impact if declined: Developer impact only (fixes a compile failure on with Linux gcc compiler)
Testing completed (on m-c, etc.): Works fine on mozilla-central
Risk to taking this patch (and alternatives if risky): Compile time fix only, the code added here has already been used/tested in another library (and on mozilla-central), so should be quite safe
String or IDL/UUID changes made by this patch: -

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a36a9cb0a86f

Comment 21

[u279076]

Bill, can you please verify this is fixed with Firefox 28 and 29 builds?

Comment 22

[Bill Gianopoulos [:WG9s]]

I have verified his with Firefox 29  Still need to do Firefox 28.

Comment 23

[u279076]

(In reply to Bill Gianopoulos [:WG9s] from comment #22)
> I have verified his with Firefox 29  Still need to do Firefox 28.

Okay, thanks.

Comment 24

[Bill Gianopoulos [:WG9s]]

Oh dear seems my .mozconfig on the system with gcc 4.5 had --disable=test so redoing that, but I am fairly certain it will work because i think I had already tested this.

Comment 25

[Bill Gianopoulos [:WG9s]]

Oh dear that was with --disable-tests because that is what the .mozconfig on that system had.  I am rebuilding again with a valid test.

Comment 26

[Bill Gianopoulos [:WG9s]]

OK did a test that actually had both enabled and it works with gcc 4.5 using firefox 29 source.  I will try the firefox 28 source tomorrow.

Comment 27

[Bill Gianopoulos [:WG9s]]

Sorry it took me a bit to get time to do this.  I have verified that I can successfully build Firefox 28 form the current Aurora tip.

Comment 28

[u279076]

(In reply to Bill Gianopoulos [:WG9s] from comment #27)
> Sorry it took me a bit to get time to do this.  I have verified that I can
> successfully build Firefox 28 form the current Aurora tip.

Thanks!