Closed Bug 1002729 Opened 10 years ago Closed 10 years ago

Link failure due to static const integers in WebRTC

Categories

(Core :: WebRTC, defect)

x86_64
Linux
defect
Not set
normal

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
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
Keywords: checkin-needed
Assignee: nobody → mrbkap
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: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.