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

VERIFIED FIXED in Firefox 28

Status

()

Core
WebRTC
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: WG9s, Assigned: Waldo)

Tracking

(Blocks: 1 bug)

Trunk
mozilla29
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 verified, firefox29 verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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'
(Reporter)

Updated

4 years ago
Version: unspecified → Trunk
(Reporter)

Comment 1

4 years ago
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
Duplicate of this bug: 956042

Updated

4 years ago
Blocks: 784739
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

4 years ago
EXPECT_EQ and ASSERT_EQ seem to be able to deal with this.  Perhaps we can extend EXPECT_NE and ASSERT_NE similarly?
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

4 years ago
It would be nice if whoever decides to work on this bug would upstream the gtest fix.
(Reporter)

Comment 7

4 years ago
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.
Created attachment 8355401 [details] [diff] [review]
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
Attachment #8355401 - Flags: review?(tterribe)
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.
Attachment #8355401 - Flags: review?(tterribe) → review+
Jesup, what should we do to get a change like this upstream?
Flags: needinfo?(rjesup)
>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.
Can we check this patch in? :)
Yes, the upstream issues can be sorted later (leaving bug 864513 blocking this one so we don't forget it).
Depends on: 864513
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!
Flags: needinfo?(rjesup)
https://hg.mozilla.org/integration/mozilla-inbound/rev/46f4a621a23d
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.)
https://hg.mozilla.org/mozilla-central/rev/46f4a621a23d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

4 years ago
Duplicate of this bug: 957489
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: -
Attachment #8355401 - Flags: approval-mozilla-aurora?

Updated

4 years ago
Attachment #8355401 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/a36a9cb0a86f
Assignee: nobody → jwalden+bmo
status-firefox28: --- → fixed
status-firefox29: --- → fixed
Bill, can you please verify this is fixed with Firefox 28 and 29 builds?
Flags: needinfo?(wgianopoulos)
(Reporter)

Comment 22

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

Okay, thanks.
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
(Reporter)

Comment 24

4 years ago
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.
(Reporter)

Comment 25

4 years ago
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.
(Reporter)

Comment 26

4 years ago
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.
(Reporter)

Comment 27

4 years ago
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.
(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!
status-firefox28: fixed → verified
Blocks: 864513
No longer depends on: 864513
You need to log in before you can comment on or make changes to this bug.