Closed Bug 1002729 Opened 6 years ago Closed 6 years ago

Link failure due to static const integers in WebRTC

Categories

(Core :: WebRTC, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

Attachments

(1 file, 1 obsolete file)

Compiler:
  * g++-4.7.real (Ubuntu/Linaro 4.7.2-2ubuntu1) 4.7.2
  * GNU gold (GNU Binutils for Ubuntu 2.22.90.20120924) 1.11
Build options: --enable-debug --disable-optimize

 0:57.00 /usr/bin/ld.gold.real: error: read-only segment has dynamic relocations
 0:57.00 /home/mrbkap/work/main/mozilla/content/media/webrtc/MediaEngine.h:158: error: undefined reference to 'mozilla::MediaEngine::DEFAULT_169_VIDEO_WIDTH'
 0:57.00 /home/mrbkap/work/main/mozilla/content/media/webrtc/MediaEngine.h:158: error: undefined reference to 'mozilla::MediaEngine::DEFAULT_43_VIDEO_WIDTH'
 0:57.00 /home/mrbkap/work/main/mozilla/content/media/webrtc/MediaEngine.h:163: error: undefined reference to 'mozilla::MediaEngine::DEFAULT_169_VIDEO_HEIGHT'
 0:57.00 /home/mrbkap/work/main/mozilla/content/media/webrtc/MediaEngine.h:163: error: undefined reference to 'mozilla::MediaEngine::DEFAULT_43_VIDEO_HEIGHT'

This appears to be a pretty common problem: static const integers with no definition (only a declaration in the class) being used in ternary expressions.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8414017 - Flags: review?(rjesup)
Comment on attachment 8414017 [details] [diff] [review]
Patch

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

::: content/media/webrtc/MediaEngine.h
@@ +154,5 @@
>    }
>  private:
>    static int32_t GetDefWidth(bool aHD = false) {
> +    return aHD ? +MediaEngine::DEFAULT_169_VIDEO_WIDTH :
> +                 +MediaEngine::DEFAULT_43_VIDEO_WIDTH;

This is confusing to the reader (and someone may try to undo it).
Add a comment as in the other bug 1007xxx, and 

I suggest 
  if (aHD) { return MediaEngine::DEFAULT_169_VIDEO_WIDTH; }
  return MediaEngine::DEFAULT_43_VIDEO_WIDTH;

with normal brace styling, of course.  If so, r+

@@ +159,5 @@
>    }
>  
>    static int32_t GetDefHeight(bool aHD = false) {
> +    return aHD ? +MediaEngine::DEFAULT_169_VIDEO_HEIGHT :
> +                 +MediaEngine::DEFAULT_43_VIDEO_HEIGHT;

ditto
Attachment #8414017 - Flags: review?(rjesup) → review+
FWIW the '+' trick is used here http://stackoverflow.com/questions/5446005/why-dont-static-member-variables-play-well-with-the-ternary-operator

In either case I think this warrants a comment.

This seems to be a problem with GCC (arguably). See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13795#c3
Duplicate of this bug: 1002877
Note that GCC 4.8.2 (Fedora 19) does not show this problem, and we don't see it on the builders.  All the reports recently have been for Ubuntu running 4.7.

glandium: we're fixing this, but is this something we should be avoiding, or should we be telling people to update GCC?  Just to verify for the future.
Flags: needinfo?(mh+mozilla)
The instance of this problem that I reported in bug 1002844 is with LLVM 5.1, so this is not GCC-specific.
> glandium: we're fixing this, but is this something we should be avoiding, or
> should we be telling people to update GCC?  Just to verify for the future.

The former.
Flags: needinfo?(mh+mozilla)
Attached patch For checkinSplinter Review
Attachment #8414017 - Attachment is obsolete: true
Assignee: nobody → mrbkap
Duplicate of this bug: 1004230
Comment on attachment 8415534 [details] [diff] [review]
For checkin

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

r=me + forward from jesup
Attachment #8415534 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4f77407d1839
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1002844
You need to log in before you can comment on or make changes to this bug.