Closed Bug 768968 Opened 12 years ago Closed 4 years ago

Clean up warnings/turn on -Werror for imported code

Categories

(Core :: WebRTC, defect, P5)

defect

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: ekr, Assigned: pkerr)

References

(Blocks 1 open bug)

Details

Attachments

(11 files)

1018 bytes, patch
jesup
: review+
Details | Diff | Splinter Review
55.65 KB, patch
jesup
: review+
Details | Diff | Splinter Review
15.44 KB, patch
jesup
: review+
rillian
: review-
derf
: review-
Details | Diff | Splinter Review
1.48 KB, patch
jesup
: review-
Details | Diff | Splinter Review
8.18 KB, patch
jesup
: review-
jmvalin
: review-
Details | Diff | Splinter Review
16.76 KB, patch
jesup
: review-
Details | Diff | Splinter Review
1.79 KB, patch
jesup
: review+
Details | Diff | Splinter Review
2.11 KB, patch
jesup
: review-
Details | Diff | Splinter Review
8.78 KB, patch
jesup
: review+
Details | Diff | Splinter Review
38.76 KB, patch
jesup
: review+
bwc
: review-
Details | Diff | Splinter Review
15.47 KB, patch
jesup
: review+
kinetik
: review-
Details | Diff | Splinter Review
A bunch of our code imports (sipcc, nrappkit, nicer, etc.) generate warnings with our stock compile flags. We should:

1. Fix the things that appear to be defects.
2. Adjust the compile flags so that they don't warn on idioms that are persistent but generate warnings. E.g., nrappkit used if (r = foo()) {} all the time but this generates warnings with -Wparentheses. We're not going to fix this.
3. Turn on -Werror

This way we don't need to wade through a sea of warnings to find the real errors.
OS: Mac OS X → All
Hardware: x86 → All
QA Contact: jsmith
Whiteboard: [WebRTC], [blocking-webrtc-]
This should be a good starter bug I think.
Assignee: nobody → paulrkerr
I have patches that clear the warnings on sipcc and nrappkit when compiled with clang xcode 5.02. Currently clearing nICEr. I need to check against gcc at some point.
Bug 768968: fixes to suppress compiler warnings from /media files
Bug 768968: fixes to suppress compiler warnings from /media files
Bug 768968: fixes to suppress compiler warnings from /media files
Bug 768968: fixes to suppress compiler warnings from /media files
Bug 768968: fixes to suppress compiler warnings from /media files
Bug 768968: fixes to suppress compiler warnings from /media files
Bug 768968: fixes to suppress compiler warnings from /media files
I am getting a three warnings on ISO C90 mixed declarations (-Wdeclaration-after-statement) on try with Linux 64 and Android builds. I found a similar warning issue in https://bugzilla.mozilla.org/show_bug.cgi?id=737678. Can we just turn the warnings off for these files and allow post c99 style code? There is no problem with the Win32 build despite the code throwing these warnings.
I just recently did a try where I ran into Windows build errors with declarations after statements - https://tbpl.mozilla.org/?tree=Try&rev=075bbd721c39
Is the code you are talking about building for Win32?
Ethan, errors in your try do not appear to be caused by having declarations following statements. I am not seeing any errors in my WinXP Debug try build https://tbpl.mozilla.org/?tree=Try&rev=133d36921ddd. (I am not running a WinXP Opt.) I get warnings about having declarations appearing after code statements in modules that get compiled for Linux 64 and Android that would generate errors if they were compiled with MSVC. These code segments do not appear to compile in the WinXP build and I want to remove -Wdeclaration-after-statement for these modules to eliminate these warnings in the /media branch.
> Ethan, errors in your try do not appear to be caused by having declarations following statements.

That's exactly what they are.  MSVC still does not support this and is not very clear in its error messages.  

Could you specify exactly which files you want to turn off this warning for?
(In reply to Ethan Hugg [:ehugg] from comment #14)
> > Ethan, errors in your try do not appear to be caused by having declarations following statements.
> 
> That's exactly what they are.  MSVC still does not support this and is not
> very clear in its error messages.  
> 
> Could you specify exactly which files you want to turn off this warning for?

Sorry, my error, I took the messages too literally. The file I want to turn off the warning for is:
media/webrtc/signaling/src/sipcc/cpr/common/cpr_ipc.c
The code segments are wrapped in "#ifndef SIP_OS_WINDOWS" sections. Which makes me wonder why Ethan's build is failing.
(In reply to Paul Kerr [:pkerr] from comment #15)
> (In reply to Ethan Hugg [:ehugg] from comment #14)
> > > Ethan, errors in your try do not appear to be caused by having declarations following statements.
> > 
> > That's exactly what they are.  MSVC still does not support this and is not
> > very clear in its error messages.  
> > 
> > Could you specify exactly which files you want to turn off this warning for?
> 
> Sorry, my error, I took the messages too literally. The file I want to turn
> off the warning for is:
> media/webrtc/signaling/src/sipcc/cpr/common/cpr_ipc.c
> The code segments are wrapped in "#ifndef SIP_OS_WINDOWS" sections. Which
> makes me wonder why Ethan's build is failing.

The alternate approach is that I disable that warning within the !SIP_OS_WINDOWS blocks of code using a #pragma. This would bind the suppression of the warning to the areas in the module that should not be compiled by MSVC.
OK, sorry I wasn't clear.  There currently is no build error.  That try was from this bug - https://bugzilla.mozilla.org/show_bug.cgi?id=969493
Which is the patch that created cpr_ipc.c.  I was just trying to point out that MSVC still does not allow declarations after statements which you of course already know.

The reason I'm interested in this warning issue is that it is very common when working in Mozilla C code to put a patch together on Linux and not find out that it breaks Win32 until you do a try.  I would actually prefer that the Linux builds break on decl-after-statement until such a time that MSVC catches up with the C spec.

If cpr_ipc.c is the only file that throws this warning I would like to fix it.  It's a bit confusing because it's a merge between previously separate Win32 and OSX versions of the file.  I'm going to CC JIB on this who was the reviewer of that patch in case he has an opinion on this.
>The alternate approach is that I disable that warning within the !SIP_OS_WINDOWS blocks of code using a >#pragma. This would bind the suppression of the warning to the areas in the module that should not be >compiled by MSVC.

Disabling it locally would be fine with me, I just wouldn't want to disable it for the whole module.
(In reply to Ethan Hugg [:ehugg] from comment #18)
> >The alternate approach is that I disable that warning within the !SIP_OS_WINDOWS blocks of code using a >#pragma. This would bind the suppression of the warning to the areas in the module that should not be >compiled by MSVC.
> 
> Disabling it locally would be fine with me, I just wouldn't want to disable
> it for the whole module.

One of the goals of this bug is to turn on -Werror, which would trigger a build error on Linux if code uses declarations after the first statement. It appears to me that the best approach would be to disable the warning for only those code segments explicitly marked for non-windows targets and not the module as whole.
(In reply to Ethan Hugg [:ehugg] from comment #17)

Given the overall goal of triggering a build error on Linux for code using declarations after the first statement, I don't have a strong opinion whether we temporarily disable the warning or temporarily fix the non-windows code in cpr_ipc.c to not induce the warning. Your call, but yes feel free to ignore the bad advice in my earlier review. ;-)
Attachment #8389249 - Flags: review?(rjesup)
Attachment #8389251 - Flags: review?(rjesup)
Attachment #8389252 - Flags: review?(rjesup)
Attachment #8389253 - Flags: review?(rjesup)
Attachment #8389254 - Flags: review?(rjesup)
Attachment #8389255 - Flags: review?(rjesup)
Attachment #8389256 - Flags: review?(rjesup)
Attachment #8389257 - Flags: review?(rjesup)
Attachment #8392199 - Flags: review?(rjesup)
Attachment #8392201 - Flags: review?(rjesup)
Attachment #8392202 - Flags: review?(rjesup)
Comment on attachment 8389249 [details] [diff] [review]
Part 07: Remove unsued variable

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

r+ with nit: Remove instead of commenting out.
Attachment #8389249 - Flags: review?(rjesup) → review+
Comment on attachment 8389251 [details] [diff] [review]
Part 06: Replace instances of int with size_t where negitive offsets could cause buffer underruns. Fix signed vs unsigned comparisons

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

r+ with nits, including the size_t/size_t result type changes.

Put the updated patch up with an r=jesup added to the summary

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c
@@ +156,5 @@
>   abort:
>      return(_status);
>    }
>  
> +static int nr_ice_ctx_set_local_addrs(nr_ice_ctx *ctx,nr_local_addr *addrs,int ct)

static is ok here, but was there a reason for it?

@@ +622,5 @@
>  
>  static int nr_ice_random_string(char *str, int len)
>    {
>      unsigned char bytes[100];
> +    unsigned needed;

This is mostly ok (needed = len/2) since if len < 0, needed will be large_int (modulo I think this is officially undefined behavior), and the "if (needed>sizeof(bytes))" will ABORT().

Really the API is wrong.

@@ +627,3 @@
>      int r,_status;
>  
>      if(len%2) ABORT(R_BAD_ARGS);

change to if (len < 0 || len%2) ABORT(...) to avoid undefined behavior

::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
@@ +553,5 @@
>      if (state == str->ice_state)
>        return 0;
>  
> +    assert((size_t)state < sizeof(nr_ice_media_stream_states)/sizeof(char *));
> +    assert((size_t)(str->ice_state) < sizeof(nr_ice_media_stream_states)/sizeof(char *));

Isn't size_t/size_t == unsigned int?  though that likely won't throw a warning when compared to size_t, but probably the case should be to (unsigned int)

::: media/mtransport/third_party/nICEr/src/stun/stun_codec.c
@@ +1210,3 @@
>  
>      *info = 0;
>      for (i = 0; i < sizeof(attrs)/sizeof(*attrs); ++i) {

sizeof()/sizeof() should yield an unsigned int, not a size_t.

::: media/mtransport/third_party/nICEr/src/stun/stun_msg.c
@@ +222,5 @@
>  nr_stun_message_add_message_integrity_attribute(nr_stun_message *msg, Data *password)
>  NR_STUN_MESSAGE_ADD_ATTRIBUTE(
>      NR_STUN_ATTR_MESSAGE_INTEGRITY,
>      {
> +        if (sizeof(attr->u.message_integrity.password) < (size_t)password->len)

Given all the casts of password->len, would it make sense to change len to size_t?

::: media/mtransport/third_party/nICEr/src/stun/stun_msg.h
@@ +136,5 @@
>           * attribute */
>          UCHAR                           largest_possible_attribute[NR_STUN_MAX_MESSAGE_SIZE];
>      } u;
>      nr_stun_encoded_attribute          *encoding;
> +    size_t                             encoding_length;

nit: add space to restore style for indent used in this file
Attachment #8389251 - Flags: review?(rjesup) → review+
Comment on attachment 8389252 [details] [diff] [review]
Part 05: libvpx - multiple unsigned vs signed comparison fixes

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

(unsigned int) is preferred to (unsigned).  (libvpx has 2 (unsigned)s, and 123 (unsigned int)s)

I got tired and didn't mark all of them in the patch

Note a couple should (canonically) be (BLOCK_SIZE)

r+ with these nits corrected

::: media/libvpx/vp8/encoder/onyx_if.c
@@ +1441,5 @@
>  
>  void vp8_change_config(VP8_COMP *cpi, VP8_CONFIG *oxcf)
>  {
>      VP8_COMMON *cm = &cpi->common;
> +    unsigned last_w, last_h, prev_number_of_layers;

unsigned int

@@ +1698,5 @@
>          cm->Width = (hs - 1 + cpi->oxcf.Width * hr) / hs;
>          cm->Height = (vs - 1 + cpi->oxcf.Height * vr) / vs;
>      }
>  
> +    if (last_w != (unsigned)cpi->oxcf.Width || last_h != (unsigned)cpi->oxcf.Height)

(unsigned int)

@@ +1707,2 @@
>          ((cm->Height + 15) & 0xfffffff0) !=
> +	 (unsigned)cm->yv12_fb[cm->lst_fb_idx].y_height ||

ditto

@@ +5471,5 @@
>      if ( cpi->cyclic_refresh_mode_enabled )
>          return -1;
>  
>      // Check number of rows and columns match
> +    if ((unsigned)cpi->common.mb_rows != rows || (unsigned)cpi->common.mb_cols != cols)

ditto

@@ +5527,5 @@
>  }
>  
>  int vp8_set_active_map(VP8_COMP *cpi, unsigned char *map, unsigned int rows, unsigned int cols)
>  {
> +    if (rows == (unsigned)cpi->common.mb_rows && cols == (unsigned)cpi->common.mb_cols)

ditto

::: media/libvpx/vp9/encoder/vp9_onyx_if.c
@@ +4155,5 @@
>    signed char feature_data[SEG_LVL_MAX][MAX_SEGMENTS];
>    struct segmentation *seg = &cpi->common.seg;
>    int i;
>  
> +  if ((unsigned)cpi->common.mb_rows != rows || (unsigned)cpi->common.mb_cols != cols)

(unsigned int) for both

@@ +4200,5 @@
>  int vp9_set_active_map(VP9_PTR comp, unsigned char *map,
>                         unsigned int rows, unsigned int cols) {
>    VP9_COMP *cpi = (VP9_COMP *) comp;
>  
> +  if (rows == (unsigned)cpi->common.mb_rows && cols == (unsigned)cpi->common.mb_cols) {

(unsigned int) for both

::: media/libvpx/vp9/encoder/vp9_rdopt.c
@@ +1907,5 @@
>          if (has_second_rf && this_mode == NEWMV &&
>              mbmi->interp_filter == EIGHTTAP) {
>            // adjust src pointers
>            mi_buf_shift(x, i);
> +          if ((unsigned)cpi->sf.comp_inter_joint_search_thresh <= bsize) {

(BLOCK_SIZE) not (unsigned)

@@ +2153,5 @@
>      this_mv.as_int = (i < MAX_MV_REF_CANDIDATES) ?
>          mbmi->ref_mvs[ref_frame][i].as_int : x->pred_mv[ref_frame].as_int;
>  
>      max_mv = MAX(max_mv,
> +                 (unsigned)MAX(abs(this_mv.as_mv.row), abs(this_mv.as_mv.col)) >> 3);

(unsigned int)

@@ +2685,5 @@
>        // Initialize mv using single prediction mode result.
>        frame_mv[refs[0]].as_int = single_newmv[refs[0]].as_int;
>        frame_mv[refs[1]].as_int = single_newmv[refs[1]].as_int;
>  
> +      if ((unsigned)cpi->sf.comp_inter_joint_search_thresh <= bsize) {

(BLoCK_SIZE)

::: media/libvpx/vp9/vp9_cx_iface.c
@@ +503,5 @@
>       * usage value. If the current usage value isn't found, use the
>       * values for usage case 0.
>       */
>      for (i = 0;
> +         extracfg_map[i].usage && (unsigned)extracfg_map[i].usage != cfg->g_usage;

"unsigned int"

@@ +583,5 @@
>      new_qc = (new_qc == MODE_BESTQUALITY)
>               ? MODE_SECONDPASS_BEST
>               : MODE_SECONDPASS;
>  
> +  if ((unsigned)ctx->oxcf.Mode != new_qc) {

"unsigned int"
Attachment #8389252 - Flags: review?(rjesup) → review+
Comment on attachment 8389252 [details] [diff] [review]
Part 05: libvpx - multiple unsigned vs signed comparison fixes

Note the summary change (or so something like it) so the logs are more useful (and it's easier to see which patch is what on the bug).

Also asking for review from either rillian (who is on PTO) or derf or gmaxwell (any one of 3)
Attachment #8389252 - Attachment description: Part 05: multiple unsigned vs signed comparision fixes → Part 05: libvpx - multiple unsigned vs signed comparison fixes
Attachment #8389252 - Flags: review?(tterribe)
Attachment #8389252 - Flags: review?(gmaxwell)
Attachment #8389252 - Flags: review?(giles)
Attachment #8389253 - Flags: review?(rjesup) → review+
derf: who/where is upstream for libtheora handled?
Flags: needinfo?(tterribe)
Attachment #8389253 - Attachment description: Part 04: ENUM unsigned compare fix → Part 04: libtheora - ENUM unsigned compare fix
Comment on attachment 8389254 [details] [diff] [review]
Part 03: fix signed vs unsigned comparision warnings

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

r- - I want to at least change the type for j in all but the first usage in resample.c and all in resample_sse.h

::: media/libspeex_resampler/src/resample.c
@@ +627,5 @@
>        }
>        for (i=0;i<st->den_rate;i++)
>        {
>           spx_int32_t j;
> +         for (j=0;(spx_uint32_t)j<st->filt_len;j++)

I'd like to convert j to spx_uint32_t - jmspeex, can we do this?

@@ +892,5 @@
>          spx_uint32_t ichunk = (ilen > xlen) ? xlen : ilen;
>          spx_uint32_t ochunk = olen;
>   
>          if (in) {
> +	   for(j=0;(unsigned)j<ichunk;++j)

convert j to spx_uint32_t instead

@@ +897,3 @@
>                x[j+filt_offs]=in[j*istride];
>          } else {
> +          for(j=0;(unsigned)j<ichunk;++j)

same

@@ +948,5 @@
>         olen -= omagic;
>       }
>       if (! st->magic_samples[channel_index]) {
>         if (in) {
> +         for(j=0;(unsigned)j<ichunk;++j)

ditto

@@ +955,5 @@
>  #else
>             x[j+st->filt_len-1]=in[j*istride_save];
>  #endif
>         } else {
> +         for(j=0;(unsigned)j<ichunk;++j)

ditto

@@ +965,5 @@
>         ichunk = 0;
>         ochunk = 0;
>       }
>  
> +     for (j=0;(unsigned)j<ochunk+omagic;++j)

ditto

::: media/libspeex_resampler/src/resample_sse.h
@@ +43,5 @@
>     float ret;
>     if (1)
>     {
>        __m128 sum = _mm_setzero_ps();
> +      for (i=0;(unsigned)i<len;i+=8)

convert int i to unsigned int i instead

@@ +68,5 @@
>    if (1)
>    {
>      __m128 sum = _mm_setzero_ps();
>      __m128 f = _mm_loadu_ps(frac);
> +    for(i=0;(unsigned)i<len;i+=2)

ditto

@@ +104,5 @@
>     int i;
>     double ret;
>     __m128d sum = _mm_setzero_pd();
>     __m128 t;
> +   for (i=0;(unsigned)i<len;i+=8)

ditto

@@ +130,5 @@
>    __m128 f = _mm_loadu_ps(frac);
>    __m128d f1 = _mm_cvtps_pd(f);
>    __m128d f2 = _mm_cvtps_pd(_mm_movehl_ps(f,f));
>    __m128 t;
> +  for(i=0;(unsigned)i<len;i+=2)

ditto
Attachment #8389254 - Flags: review?(rjesup)
Attachment #8389254 - Flags: review?(jmvalin)
Attachment #8389254 - Flags: review-
> 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c
> @@ +156,5 @@
> >   abort:
> >      return(_status);
> >    }
> >  
> > +static int nr_ice_ctx_set_local_addrs(nr_ice_ctx *ctx,nr_local_addr *addrs,int ct)
> 
> static is ok here, but was there a reason for it?
> 

The warning: "no previous prototype for function 'nr_ice_ctx_set_local_addrs'". This function does not appear to be exported from this module (not in .h). I made it explicitly local to kill the warning and mark it as module internal.
(In reply to Randell Jesup [:jesup] from comment #25)
> Comment on attachment 8389251 [details] [diff] [review]
> Part 06: Replace instances of int with size_t where negitive offsets could
> cause buffer underruns. Fix signed vs unsigned comparisons
> 
> Review of attachment 8389251 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with nits, including the size_t/size_t result type changes.
> 
> Put the updated patch up with an r=jesup added to the summary
> 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c
> @@ +156,5 @@
> >   abort:
> >      return(_status);
> >    }
> >  
> > +static int nr_ice_ctx_set_local_addrs(nr_ice_ctx *ctx,nr_local_addr *addrs,int ct)
> 
> static is ok here, but was there a reason for it?
> 
> @@ +622,5 @@
> >  
> >  static int nr_ice_random_string(char *str, int len)
> >    {
> >      unsigned char bytes[100];
> > +    unsigned needed;
> 
> This is mostly ok (needed = len/2) since if len < 0, needed will be
> large_int (modulo I think this is officially undefined behavior), and the
> "if (needed>sizeof(bytes))" will ABORT().
> 
> Really the API is wrong.
> 
> @@ +627,3 @@
> >      int r,_status;
> >  
> >      if(len%2) ABORT(R_BAD_ARGS);
> 
> change to if (len < 0 || len%2) ABORT(...) to avoid undefined behavior
> 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
> @@ +553,5 @@
> >      if (state == str->ice_state)
> >        return 0;
> >  
> > +    assert((size_t)state < sizeof(nr_ice_media_stream_states)/sizeof(char *));
> > +    assert((size_t)(str->ice_state) < sizeof(nr_ice_media_stream_states)/sizeof(char *));
> 
> Isn't size_t/size_t == unsigned int?  though that likely won't throw a
> warning when compared to size_t, but probably the case should be to
> (unsigned int)
> 
> ::: media/mtransport/third_party/nICEr/src/stun/stun_codec.c
> @@ +1210,3 @@
> >  
> >      *info = 0;
> >      for (i = 0; i < sizeof(attrs)/sizeof(*attrs); ++i) {
> 
> sizeof()/sizeof() should yield an unsigned int, not a size_t.
> 
> ::: media/mtransport/third_party/nICEr/src/stun/stun_msg.c
> @@ +222,5 @@
> >  nr_stun_message_add_message_integrity_attribute(nr_stun_message *msg, Data *password)
> >  NR_STUN_MESSAGE_ADD_ATTRIBUTE(
> >      NR_STUN_ATTR_MESSAGE_INTEGRITY,
> >      {
> > +        if (sizeof(attr->u.message_integrity.password) < (size_t)password->len)
> 
> Given all the casts of password->len, would it make sense to change len to
> size_t?
> 
The Data type appears to be a generic struct which in this case is used to pass the password. I wanted to keep the fix localized. I can change the definition of the len field and see if that causes trouble in any other modules.
 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
> @@ +553,5 @@
> >      if (state == str->ice_state)
> >        return 0;
> >  
> > +    assert((size_t)state < sizeof(nr_ice_media_stream_states)/sizeof(char *));
> > +    assert((size_t)(str->ice_state) < sizeof(nr_ice_media_stream_states)/sizeof(char *));
> 
> Isn't size_t/size_t == unsigned int?  though that likely won't throw a
> warning when compared to size_t, but probably the case should be to
> (unsigned int)
> 
Doesn't size_t/size_t result in a size_t? size_t might be tyepdefed to unsigned int, but it can by any unsigned integer type. If the divisor was a 1, then the quotient needs to be as wide as the dividend.
(In reply to Randell Jesup [:jesup] from comment #28)
> derf: who/where is upstream for libtheora handled?

Upstream is at <https://svn.xiph.org/trunk/theora>. A number of us have commit access there.

However, the changes in the patch in Part 4 directly contradict the comment immediately above that code.
Flags: needinfo?(tterribe)
(In reply to Paul Kerr [:pkerr] from comment #32)
>  
> > ::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
> > @@ +553,5 @@
> > >      if (state == str->ice_state)
> > >        return 0;
> > >  
> > > +    assert((size_t)state < sizeof(nr_ice_media_stream_states)/sizeof(char *));
> > > +    assert((size_t)(str->ice_state) < sizeof(nr_ice_media_stream_states)/sizeof(char *));
> > 
> > Isn't size_t/size_t == unsigned int?  though that likely won't throw a
> > warning when compared to size_t, but probably the case should be to
> > (unsigned int)
> > 
> Doesn't size_t/size_t result in a size_t? size_t might be tyepdefed to
> unsigned int, but it can by any unsigned integer type. If the divisor was a
> 1, then the quotient needs to be as wide as the dividend.

Aha.  While it would make sense for size_t/size_t to yield an non-specific unsigned value, it doesn't:
For arithmetic operators: "An operation on two operands of the same type returns the same type"  So never mind those comments
Comment on attachment 8389253 [details] [diff] [review]
Part 04: libtheora - ENUM unsigned compare fix

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

My apologies... I totally focused on the code and didn't read the preceding comment.
Attachment #8389253 - Flags: review+ → review-
(In reply to Randell Jesup [:jesup] from comment #35)
> Comment on attachment 8389253 [details] [diff] [review]
> Part 04: libtheora - ENUM unsigned compare fix
> 
> Review of attachment 8389253 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My apologies... I totally focused on the code and didn't read the preceding
> comment.

Same here.

If the goal of being able to run under -Werror is to be met, then perhaps we should turn specific warnings off per module or wrap the code after a comment such as above in a #pragma controlled section that suppresses the warning.
Comment on attachment 8389255 [details] [diff] [review]
Part 02: fix signed vs unsigned comparision warnings

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

General: unsigned -> unsigned int
r- for the snprintf thing.   You could suggest to ekr that we change the function name, or change the return value to reflect normal operation of this lib routine.

::: media/mtransport/third_party/nrappkit/src/log/r_log.c
@@ +198,5 @@
> +	size_req=snprintf(dest_prefix,sizeof(NR_registry),
> +	                 "logging.%s.facility",log_destinations[j].dest_name);
> +	/* a negative value is returned on an error, a positive value represents
> +	   the size required to construct the string. */
> +	if(size_req<0||(size_t)size_req>=sizeof(NR_registry))

nsappkit's snprintf() does NOT return the normal return codes.  Yes, this is confusing and a bad idea....  see util.c.

::: media/mtransport/third_party/nrappkit/src/registry/registry.c
@@ +357,5 @@
>      assert(!strcasecmp(typenames[NR_REG_TYPE_BYTES],    "Data"));
>      assert(!strcasecmp(typenames[NR_REG_TYPE_STRING],   "string"));
>      assert(!strcasecmp(typenames[NR_REG_TYPE_REGISTRY], "registry"));
>      assert(sizeof(typenames)/sizeof(*typenames) == (NR_REG_TYPE_REGISTRY+1));
> +    assert(sizeof(typenames)/sizeof(*typenames) <= INT_MAX);

Given the previous line, is this assert actually useful?

::: media/mtransport/third_party/nrappkit/src/util/libekr/r_bitfield.c
@@ +96,5 @@
>      *setp=0;
>      return(0);
>    }
>  
>  int r_bitfield_set(set,bit)

Really???  Ugh.  But that isn't the issue here

::: media/mtransport/third_party/nrappkit/src/util/libekr/r_bitfield.h
@@ +54,5 @@
>       UINT4 base;
>  } r_bitfield;
>  
> +int r_bitfield_set(r_bitfield *,unsigned bit);
> +int r_bitfield_isset(r_bitfield *,unsigned bit);

unsigned int
Attachment #8389255 - Flags: review?(rjesup) → review-
Comment on attachment 8389256 [details] [diff] [review]
Part 01: Replace static with inline function defs

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

A bit ugly, but ok
Attachment #8389256 - Flags: review?(rjesup) → review+
Comment on attachment 8389257 [details] [diff] [review]
Part 00: Replace static var with string const for log tag

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

Does this generate a warning?  If not and it can't be suppressed, then ok, we could land this - but i don't seen the point in optimizing it for "we have only one logging call" and then have to undo it when we add a second.
Attachment #8389257 - Flags: review?(rjesup) → review-
(In reply to Paul Kerr [:pkerr] from comment #36)
> If the goal of being able to run under -Werror is to be met, then perhaps we
> should turn specific warnings off per module or wrap the code after a
> comment such as above in a #pragma controlled section that suppresses the
> warning.

We could also re-evaluate the decision in that comment. At the time I wrote it, the compiler bugs I was primarily concerned about were from MSVC6, which is fortunately mostly a historical curiosity at this point.

But if you go that way, you still need to handle platforms where enums are signed, and the comment needs to be updated.
Comment on attachment 8392199 [details] [diff] [review]
Part 08: Prevent macro redefinition or undefine macro names that are reused in other modules

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

Split vp8 files into a separate patch for review by vp8 people.

r+ for the rest if added to non-unified sources.

::: media/libvpx/vp8/decoder/onyxd_int.h
@@ +130,5 @@
>  
>  int vp8_create_decoder_instances(struct frame_buffers *fb, VP8D_CONFIG *oxcf);
>  int vp8_remove_decoder_instances(struct frame_buffers *fb);
>  
> +#ifndef CHECK_MEM_ERROR

Are these defs always the same?  If so, undef and re-define.

::: media/mtransport/third_party/nrappkit/src/registry/registry.c
@@ +136,5 @@
>  NRREGGET(NR_reg_get_int8,     get_int8,     INT8)
>  NRREGGET(NR_reg_get_uint8,    get_uint8,    UINT8)
>  NRREGGET(NR_reg_get_double,   get_double,   double)
>  
> +#undef NRREGGET

Add to non-unified sources, or get agreement from ekr

::: media/mtransport/third_party/nrappkit/src/registry/registry_local.c
@@ +899,5 @@
>  NRREGGET(nr_reg_local_get_uint8,    NR_REG_TYPE_UINT8,    UINT8)
>  NRREGGET(nr_reg_local_get_double,   NR_REG_TYPE_DOUBLE,   double)
>  
> +#undef NRREGGET
> +

ditto

::: media/webrtc/trunk/webrtc/modules/audio_device/linux/audio_device_alsa_linux.cc
@@ +2391,5 @@
>    return (state != 0);
>  }
>  }  // namespace webrtc
> +
> +#undef LATE

unlikely to be taken by upstream; instead add it to the non-unified sources list in moz.build in media/webrtc with a comment

::: media/webrtc/trunk/webrtc/modules/audio_device/linux/audio_device_pulse_linux.cc
@@ +3136,5 @@
>    return (state != 0);
>  }
>  }
> +
> +#undef LATE

ditto

::: media/webrtc/trunk/webrtc/modules/audio_device/linux/audio_mixer_manager_alsa_linux.cc
@@ +1315,5 @@
>  }
>  
>  }
> +
> +#undef LATE

ditto

::: media/webrtc/trunk/webrtc/modules/audio_device/linux/audio_mixer_manager_pulse_linux.cc
@@ +1265,5 @@
>  }
>  
>  }
> +
> +#undef LATE

ditto
Attachment #8392199 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #39)
> Comment on attachment 8389257 [details] [diff] [review]
> Part 00: Replace static var with string const for log tag
> 
> Review of attachment 8389257 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does this generate a warning?  If not and it can't be suppressed, then ok,
> we could land this - but i don't seen the point in optimizing it for "we
> have only one logging call" and then have to undo it when we add a second.

It generates a warning because the log tag is a static local variable but the function is defined as inline. This function is exported in the .h file. If the compiler does create an inline version in another module the log tag will not be available. In the original code, there are two points were a log message is generated and both use a different tag. The first uses the static variable and the second a string literal.
Comment on attachment 8392201 [details] [diff] [review]
Part 09: fixes for Linux 64 Debug build warnings

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

Please split out nestegg/opus/speex/vpx into separate patches for those owners to review.

::: media/libopus/src/opus_decoder.c
@@ +214,5 @@
>     int pcm_transition_silk_size;
>     VARDECL(opus_val16, pcm_transition_silk);
>     int pcm_transition_celt_size;
>     VARDECL(opus_val16, pcm_transition_celt);
> +   opus_val16 *pcm_transition = 0;

It so happens this appears safe without it (though it requires walking codepaths to be sure).  So upstream may not want to take the (minor) hit to initialize to NULL.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
@@ -572,5 @@
>        remote_addr_matched = 0;
>  
>        if(pair->remote->component->component_id!=comp->component_id)
>          goto next_pair;
> -      component_id_matched = 1;

bwc/ekr: cruft, stub for later filling out?

@@ -900,5 @@
>      nr_ice_cand_pair *p2;
>  
> -    if(!comp->nominated)
> -      fire_cb=1;
> -

bwc: was this cruft, or is this the sign of a bug?

::: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c
@@ +426,5 @@
>  int nr_stun_client_process_response(nr_stun_client_ctx *ctx, UCHAR *msg, int len, nr_transport_addr *peer_addr)
>    {
>      int r,_status;
>      char string[256];
> +    char *username = 0;

This looks like a possible real bugfix (garbage pointer) - ekr?

::: media/mtransport/third_party/nICEr/src/stun/stun_server_ctx.c
@@ -392,5 @@
> -    }
> -    else {
> -        hmacPassword = 0;
> -    }
> -

Check with ekr that this isn't a stub for later additions

::: media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c
@@ +572,5 @@
>  
>  static void nr_turn_client_allocate_cb(NR_SOCKET s, int how, void *arg)
>  {
>    nr_turn_stun_ctx *ctx = (nr_turn_stun_ctx *)arg;
> +  nr_turn_stun_ctx *refresh_ctx = 0;

The errors/return-code checks keep this from being a bug
Attachment #8392201 - Flags: review?(rjesup)
Attachment #8392201 - Flags: review?(docfaraday)
Attachment #8392201 - Flags: review+
ekr: Please see comments on part 9 with questions for you
Flags: needinfo?(ekr)
Comment on attachment 8392202 [details] [diff] [review]
Part 10: fixes for Android Debug build warnings

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

Split off non-sipcc stuff into separate patches.  r+ on the sipcc part

kinetik: the cubeb stuff looks like potential bugs (didn't deeply review)

::: media/libcubeb/src/cubeb_audiotrack.c
@@ +317,3 @@
>  
> +  rv = ctx->klass.get_output_samplingrate(&_rate, 3 /* MUSIC */);
> +  *rate = (uint32_t)_rate;

This looks like a real bug...

::: media/libcubeb/src/cubeb_opensl.c
@@ +277,5 @@
>    /* Depending on which method we called above, we can get a zero back, yet have
>     * a non-error return value, especially if the audio system is not
>     * ready/shutting down (i.e. when we can't get our hand on the AudioFlinger
>     * thread). */
> +  if (*rate == 0) {

This looks like a real bug

::: media/omx-plugin/OmxPlugin.cpp
@@ +280,5 @@
>    switch (aColorFormat) {
>      case OMX_COLOR_FormatCbYCrY:
>      case OMX_COLOR_FormatYUV420Planar:
>      case OMX_COLOR_FormatYUV420SemiPlanar:
> +//    case OMX_QCOM_COLOR_FormatYVU420PackedSemiPlanar32m4ka:

why?

@@ +726,5 @@
>    // multiple of 16 but the data itself is too small to fit. What we do is check
>    // to see if the video size patches the raw width and height. If so we can
>    // use those figures instead.
>  
> +  if (aSize == (size_t)mVideoWidth * (size_t)mVideoHeight * 3 / 2) {

(size_t) (mVideoWidth * mVideoHeight * 3 / 2)
width and height aren't size_t's.
Attachment #8392202 - Flags: review?(rjesup)
Attachment #8392202 - Flags: review?(kinetik)
Attachment #8392202 - Flags: review+
(In reply to Randell Jesup [:jesup] from comment #38)
> Comment on attachment 8389256 [details] [diff] [review]
> Part 01: Replace static with inline function defs
> 
> Review of attachment 8389256 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A bit ugly, but ok

The source warning was: "[-Wunused-function] unused function 'sdp_rtcp_fb_nack_to_bitmap'". These function definitions when included in c++ modules trigger this error if they are marked as static probably because it treats it as local linkage and then doesn't see them used in the module. Marking them as inline silences the warning but won't work for a pre-C99 compiler.
Comment on attachment 8392201 [details] [diff] [review]
Part 09: fixes for Linux 64 Debug build warnings

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

I just reviewed the nICEr and nrappkit stuff; I can look at the rest if desired, but it would take a while to get enough context in my head to review that code effectively.

r- mostly for the changes in media/mtransport/third_party/nICEr/src/stun/stun_msg.c

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c
@@ +528,5 @@
>  
>  void nr_ice_candidate_pair_restart_stun_nominated_cb(NR_SOCKET s, int how, void *cb_arg)
>    {
>      nr_ice_cand_pair *pair=cb_arg;
> +    int r,_status=0;

Hmm. It would probably fit better to just make this function return(_status) like everything else, even though nothing will actually use it. Removing the abort stuff entirely would remove our ability to enable R_TRACE_ERRORS and get logging when things fail here, so that's not a great option. Is this causing warnings?

@@ +549,5 @@
>  
>  static void nr_ice_candidate_pair_restart_stun_controlled_cb(NR_SOCKET s, int how, void *cb_arg)
>    {
>      nr_ice_cand_pair *pair=cb_arg;
> +    int r,_status=0;

See above.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
@@ -572,5 @@
>        remote_addr_matched = 0;
>  
>        if(pair->remote->component->component_id!=comp->component_id)
>          goto next_pair;
> -      component_id_matched = 1;

This (along with local_addr_matched and remote_addr_matched) looks like cruft to me. Perhaps it was used for debug logging at one time?

@@ +872,5 @@
>     to do the procedures from S 7.2.1 in nr_ice_component_stun_server_cb.
>   */
>  static int nr_ice_component_stun_server_default_cb(void *cb_arg,nr_stun_server_ctx *stun_ctx,nr_socket *sock, nr_stun_server_request *req, int *dont_free, int *error)
>    {
> +    int r, _status=0;

This pattern is used all over the place in nICEr, is this particular instance causing warnings? I do notice that we are ignoring the value of _status when we return, which is does not look correct in this case.

@@ -900,5 @@
>      nr_ice_cand_pair *p2;
>  
> -    if(!comp->nominated)
> -      fire_cb=1;
> -

I suspect that the ICE connected callback used to happen in this function, and this variable was used to determine whether it should be called. Right now, the connected callback happens in nr_ice_media_stream_component_nominated.

::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
@@ +337,5 @@
>  /* S 5.8 -- run the highest priority WAITING pair or if not available
>     FROZEN pair */
>  static void nr_ice_media_stream_check_timer_cb(NR_SOCKET s, int h, void *cb_arg)
>    {
> +    int r,_status=0;

Another one of these.

::: media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c
@@ +163,5 @@
>  {
>    nr_socket_buffered_stun *sock = (nr_socket_buffered_stun *)obj;
>  
>    int r, _status;
> +  size_t written = 0;

Is this triggering a warning? nr_socket_buffered_stun_write sets this iff it returns 0, which is a prerequisite for us using it.

::: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c
@@ +202,5 @@
>  }
>  
>  static void nr_stun_client_timer_expired_cb(NR_SOCKET s, int b, void *cb_arg)
>    {
> +    int _status=0;

Another one of these.

::: media/mtransport/third_party/nICEr/src/stun/stun_msg.c
@@ +206,5 @@
>  NR_STUN_MESSAGE_ADD_ATTRIBUTE(
>      NR_STUN_ATTR_ERROR_CODE,
>      {
>          attr->u.error_code.number = number;
> +        r=strlcpy(attr->u.error_code.reason, reason, sizeof(attr->u.error_code.reason));

A couple of things:

- r is ignored, which is a good thing because...
- strlcpy doesn't return an error code, but a strlen

@@ +234,5 @@
>  int
>  nr_stun_message_add_nonce_attribute(nr_stun_message *msg, char *nonce)
>  NR_STUN_MESSAGE_ADD_ATTRIBUTE(
>      NR_STUN_ATTR_NONCE,
> +    { r=strlcpy(attr->u.nonce, nonce, sizeof(attr->u.nonce)); }

See above.

@@ +241,5 @@
>  int
>  nr_stun_message_add_realm_attribute(nr_stun_message *msg, char *realm)
>  NR_STUN_MESSAGE_ADD_ATTRIBUTE(
>      NR_STUN_ATTR_REALM,
> +    { r=strlcpy(attr->u.realm, realm, sizeof(attr->u.realm)); }

See above.

@@ +248,5 @@
>  int
>  nr_stun_message_add_server_attribute(nr_stun_message *msg, char *server_name)
>  NR_STUN_MESSAGE_ADD_ATTRIBUTE(
>      NR_STUN_ATTR_SERVER,
> +    { r=strlcpy(attr->u.server_name, server_name, sizeof(attr->u.server_name)); }

See above.

@@ +262,5 @@
>  int
>  nr_stun_message_add_username_attribute(nr_stun_message *msg, char *username)
>  NR_STUN_MESSAGE_ADD_ATTRIBUTE(
>      NR_STUN_ATTR_USERNAME,
> +    { r=strlcpy(attr->u.username, username, sizeof(attr->u.username)); }

See above.

::: media/mtransport/third_party/nICEr/src/stun/stun_server_ctx.c
@@ -392,5 @@
> -    }
> -    else {
> -        hmacPassword = 0;
> -    }
> -

It looks to me like this is used in nr_stun_form_success_response for the client side case. I suspect this is cruft.

::: media/mtransport/third_party/nrappkit/src/log/r_log.c
@@ +277,5 @@
>  static void r_log_facility_change_cb(void *cb_arg, char action, NR_registry name)
>    {
>      int *lt_level=(int *)cb_arg;
>      int level;
> +    int r,_status=0;

Another of these.

::: media/mtransport/third_party/nrappkit/src/util/libekr/r_macros.h
@@ +108,5 @@
>  #define ERETURN(a) do {int _r=a; if(!_r) _r=-1; REPORT_ERROR_("ERETURN",_r); return(_r);} while(0)
>  #endif
>  
>  #ifndef ABORT
> +#define ABORT(a) do { _status=a; if(!_status) _status=-1; REPORT_ERROR_("ABORT",_status); goto abort;} while(0)

While this seems like a simplification to me, I can't help but think I am missing some context that explains why it was the way it was. One thing that occurs to me is that when R_TRACE_ERRORS is set, we printf _r as %d; if _status happens to be a different width, we could be in trouble. Of course, with -Werror we'd notice...
Attachment #8392201 - Flags: review?(docfaraday) → review-
(In reply to Byron Campen [:bwc] from comment #47)
> Comment on attachment 8392201 [details] [diff] [review]
> Part 09: fixes for Linux 64 Debug build warnings
> 
> Review of attachment 8392201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I just reviewed the nICEr and nrappkit stuff; I can look at the rest if
> desired, but it would take a while to get enough context in my head to
> review that code effectively.
> 
> r- mostly for the changes in
> media/mtransport/third_party/nICEr/src/stun/stun_msg.c
> 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c
> @@ +528,5 @@
> >  
> >  void nr_ice_candidate_pair_restart_stun_nominated_cb(NR_SOCKET s, int how, void *cb_arg)
> >    {
> >      nr_ice_cand_pair *pair=cb_arg;
> > +    int r,_status=0;
> 
> Hmm. It would probably fit better to just make this function return(_status)
> like everything else, even though nothing will actually use it. Removing the
> abort stuff entirely would remove our ability to enable R_TRACE_ERRORS and
> get logging when things fail here, so that's not a great option. Is this
> causing warnings?
> 
> @@ +549,5 @@
> >  
> >  static void nr_ice_candidate_pair_restart_stun_controlled_cb(NR_SOCKET s, int how, void *cb_arg)
> >    {
> >      nr_ice_cand_pair *pair=cb_arg;
> > +    int r,_status=0;
> 
> See above.
> 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
> @@ -572,5 @@
> >        remote_addr_matched = 0;
> >  
> >        if(pair->remote->component->component_id!=comp->component_id)
> >          goto next_pair;
> > -      component_id_matched = 1;
> 
> This (along with local_addr_matched and remote_addr_matched) looks like
> cruft to me. Perhaps it was used for debug logging at one time?
> 
> @@ +872,5 @@
> >     to do the procedures from S 7.2.1 in nr_ice_component_stun_server_cb.
> >   */
> >  static int nr_ice_component_stun_server_default_cb(void *cb_arg,nr_stun_server_ctx *stun_ctx,nr_socket *sock, nr_stun_server_request *req, int *dont_free, int *error)
> >    {
> > +    int r, _status=0;
> 
> This pattern is used all over the place in nICEr, is this particular
> instance causing warnings? I do notice that we are ignoring the value of
> _status when we return, which is does not look correct in this case.
> 
> @@ -900,5 @@
> >      nr_ice_cand_pair *p2;
> >  
> > -    if(!comp->nominated)
> > -      fire_cb=1;
> > -
> 
> I suspect that the ICE connected callback used to happen in this function,
> and this variable was used to determine whether it should be called. Right
> now, the connected callback happens in
> nr_ice_media_stream_component_nominated.
> 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
> @@ +337,5 @@
> >  /* S 5.8 -- run the highest priority WAITING pair or if not available
> >     FROZEN pair */
> >  static void nr_ice_media_stream_check_timer_cb(NR_SOCKET s, int h, void *cb_arg)
> >    {
> > +    int r,_status=0;
> 
> Another one of these.
> 
> ::: media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c
> @@ +163,5 @@
> >  {
> >    nr_socket_buffered_stun *sock = (nr_socket_buffered_stun *)obj;
> >  
> >    int r, _status;
> > +  size_t written = 0;
> 
> Is this triggering a warning? nr_socket_buffered_stun_write sets this iff it
> returns 0, which is a prerequisite for us using it.
> 
> ::: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c
> @@ +202,5 @@
> >  }
> >  
> >  static void nr_stun_client_timer_expired_cb(NR_SOCKET s, int b, void *cb_arg)
> >    {
> > +    int _status=0;
> 
> Another one of these.
> 
> ::: media/mtransport/third_party/nICEr/src/stun/stun_msg.c
> @@ +206,5 @@
> >  NR_STUN_MESSAGE_ADD_ATTRIBUTE(
> >      NR_STUN_ATTR_ERROR_CODE,
> >      {
> >          attr->u.error_code.number = number;
> > +        r=strlcpy(attr->u.error_code.reason, reason, sizeof(attr->u.error_code.reason));
> 
> A couple of things:
> 
> - r is ignored, which is a good thing because...
> - strlcpy doesn't return an error code, but a strlen
> 
> @@ +234,5 @@
> >  int
> >  nr_stun_message_add_nonce_attribute(nr_stun_message *msg, char *nonce)
> >  NR_STUN_MESSAGE_ADD_ATTRIBUTE(
> >      NR_STUN_ATTR_NONCE,
> > +    { r=strlcpy(attr->u.nonce, nonce, sizeof(attr->u.nonce)); }
> 
> See above.
> 
> @@ +241,5 @@
> >  int
> >  nr_stun_message_add_realm_attribute(nr_stun_message *msg, char *realm)
> >  NR_STUN_MESSAGE_ADD_ATTRIBUTE(
> >      NR_STUN_ATTR_REALM,
> > +    { r=strlcpy(attr->u.realm, realm, sizeof(attr->u.realm)); }
> 
> See above.
> 
> @@ +248,5 @@
> >  int
> >  nr_stun_message_add_server_attribute(nr_stun_message *msg, char *server_name)
> >  NR_STUN_MESSAGE_ADD_ATTRIBUTE(
> >      NR_STUN_ATTR_SERVER,
> > +    { r=strlcpy(attr->u.server_name, server_name, sizeof(attr->u.server_name)); }
> 
> See above.
> 
> @@ +262,5 @@
> >  int
> >  nr_stun_message_add_username_attribute(nr_stun_message *msg, char *username)
> >  NR_STUN_MESSAGE_ADD_ATTRIBUTE(
> >      NR_STUN_ATTR_USERNAME,
> > +    { r=strlcpy(attr->u.username, username, sizeof(attr->u.username)); }
> 
> See above.
> 
> ::: media/mtransport/third_party/nICEr/src/stun/stun_server_ctx.c
> @@ -392,5 @@
> > -    }
> > -    else {
> > -        hmacPassword = 0;
> > -    }
> > -
> 
> It looks to me like this is used in nr_stun_form_success_response for the
> client side case. I suspect this is cruft.
> 
> ::: media/mtransport/third_party/nrappkit/src/log/r_log.c
> @@ +277,5 @@
> >  static void r_log_facility_change_cb(void *cb_arg, char action, NR_registry name)
> >    {
> >      int *lt_level=(int *)cb_arg;
> >      int level;
> > +    int r,_status=0;
> 
> Another of these.
> 
> ::: media/mtransport/third_party/nrappkit/src/util/libekr/r_macros.h
> @@ +108,5 @@
> >  #define ERETURN(a) do {int _r=a; if(!_r) _r=-1; REPORT_ERROR_("ERETURN",_r); return(_r);} while(0)
> >  #endif
> >  
> >  #ifndef ABORT
> > +#define ABORT(a) do { _status=a; if(!_status) _status=-1; REPORT_ERROR_("ABORT",_status); goto abort;} while(0)
> 
> While this seems like a simplification to me, I can't help but think I am
> missing some context that explains why it was the way it was. One thing that
> occurs to me is that when R_TRACE_ERRORS is set, we printf _r as %d; if
> _status happens to be a different width, we could be in trouble. Of course,
> with -Werror we'd notice...

The warnings are of two types:
1) Ignoring a return code. This happens in the macros that uses stlen. I used the available locally declared r variable to quiet the warning.

2) Setting and then not using a variable. This is caused by the ABORT macro: it assumes that a variable _status have been declared somewhere outside of the macro and always sets it to the value of _r. In some of the functions where ABORT is called _status is never used. This is mainly in void functions but does happen in one int returning function where the return value is always set to 0. This leads to generating code that is required to declare _status to be able to use ABORT.

What I did was use _status for the same purpose as _r so that _status would always be 'used'. For ABORT to work, _status must be in scope, but you're correct in that it doesn't necessarily have to be an int. And, we have this dependency on _status and ABORT. I am open to suggestions.
Comment on attachment 8389252 [details] [diff] [review]
Part 05: libvpx - multiple unsigned vs signed comparison fixes

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

I think some of these changes need to be re-thought, so I'd like to see the patch again after you do so.

::: media/libvpx/vp8/encoder/onyx_if.c
@@ +1441,5 @@
>  
>  void vp8_change_config(VP8_COMP *cpi, VP8_CONFIG *oxcf)
>  {
>      VP8_COMMON *cm = &cpi->common;
> +    unsigned last_w, last_h, prev_number_of_layers;

Um... last_w and last_h should still be ints, no? Only prev_number_of_layers is compared with an unsigned value.

@@ +1698,5 @@
>          cm->Width = (hs - 1 + cpi->oxcf.Width * hr) / hs;
>          cm->Height = (vs - 1 + cpi->oxcf.Height * vr) / vs;
>      }
>  
> +    if (last_w != (unsigned)cpi->oxcf.Width || last_h != (unsigned)cpi->oxcf.Height)

If you keep the types of these as int above, you don't need these casts.

@@ +1707,2 @@
>          ((cm->Height + 15) & 0xfffffff0) !=
> +	 (unsigned)cm->yv12_fb[cm->lst_fb_idx].y_height ||

Both cm->Width and y_width are ints, not unsigned. The expression is getting promoted to unsigned because the constant "0xfffffff0" does not fit in a signed integer when treated as a positive number.

I think the better solution is to replace it with "~15", which avoids the unnecessary promotion, and eliminates the need for the casts.

@@ +5471,5 @@
>      if ( cpi->cyclic_refresh_mode_enabled )
>          return -1;
>  
>      // Check number of rows and columns match
> +    if ((unsigned)cpi->common.mb_rows != rows || (unsigned)cpi->common.mb_cols != cols)

Are you sure this doesn't create a situation where someone could initialize a context with mb_rows and mb_cols set to an invalid, negative value (e.g., -1), and then pass the validation checks here with a large positive value? The actual allocation code does not appear to check, and if both mb_rows and mb_cols are negative, will likely succeed, since most allocations compute their size as the product of these two values.

@@ +5527,5 @@
>  }
>  
>  int vp8_set_active_map(VP8_COMP *cpi, unsigned char *map, unsigned int rows, unsigned int cols)
>  {
> +    if (rows == (unsigned)cpi->common.mb_rows && cols == (unsigned)cpi->common.mb_cols)

See above.

::: media/libvpx/vp8/vp8_cx_iface.c
@@ +611,5 @@
>           * usage value. If the current usage value isn't found, use the
>           * values for usage case 0.
>           */
>          for (i = 0;
> +             extracfg_map[i].usage && (unsigned)extracfg_map[i].usage != cfg->g_usage;

The test in vpx/src/vpx_encoder.c:173 goes the other way (casts g_usage to (int) instead of casting usage to (unsigned)). We should be consistent.

@@ +730,5 @@
>          new_qc = (new_qc == MODE_BESTQUALITY)
>                   ? MODE_SECONDPASS_BEST
>                   : MODE_SECONDPASS;
>  
> +    if ((unsigned)ctx->oxcf.Mode != new_qc)

I think the better fix is to change the type of new_qc to int. There's no real reason for it to be different from Mode that I can see.

::: media/libvpx/vp9/encoder/vp9_onyx_if.c
@@ +1554,5 @@
>    }
>  }
>  
>  VP9_PTR vp9_create_compressor(VP9_CONFIG *oxcf) {
> +  unsigned int i;

I think the better fix is to leave this as int and cast the loop bound in the first loop that uses i to int.

In general compilers do a better job of optimizing loops with signed loop counters (because overflow is undefined behavior, they can assume it never happens). It doesn't matter in initialization code like this, but it's a good habit to get into.

@@ +1969,5 @@
>  #ifdef MODE_TEST_HIT_STATS
>      if (cpi->pass != 1) {
>        double norm_per_pixel_mode_tests = 0;
>        double norm_counts[BLOCK_SIZES];
> +      unsigned int i;

Ditto, especially because i gets shadowed below several times by signed ints. The shadowing is confusing enough without changing the type of the variable.

@@ +4155,5 @@
>    signed char feature_data[SEG_LVL_MAX][MAX_SEGMENTS];
>    struct segmentation *seg = &cpi->common.seg;
>    int i;
>  
> +  if ((unsigned)cpi->common.mb_rows != rows || (unsigned)cpi->common.mb_cols != cols)

Same comments as for VP8.

@@ +4200,5 @@
>  int vp9_set_active_map(VP9_PTR comp, unsigned char *map,
>                         unsigned int rows, unsigned int cols) {
>    VP9_COMP *cpi = (VP9_COMP *) comp;
>  
> +  if (rows == (unsigned)cpi->common.mb_rows && cols == (unsigned)cpi->common.mb_cols) {

See above.

::: media/libvpx/vp9/encoder/vp9_rdopt.c
@@ +740,5 @@
>    MACROBLOCKD *const xd = &x->e_mbd;
>    MB_MODE_INFO *const mbmi = &xd->mi_8x8[0]->mbmi;
>    vp9_prob skip_prob = vp9_get_pred_prob_mbskip(cm, xd);
>    int64_t rd[TX_SIZES][2];
> +  unsigned int n, m;

Signedness of enum values is implementation-defined. These should properly be of type TX_SIZE, not unsigned int.

@@ +843,5 @@
>    MACROBLOCKD *const xd = &x->e_mbd;
>    MB_MODE_INFO *const mbmi = &xd->mi_8x8[0]->mbmi;
>    vp9_prob skip_prob = vp9_get_pred_prob_mbskip(cm, xd);
>    int64_t rd[TX_SIZES][2];
> +  unsigned int n, m;

Ditto.

@@ +1907,5 @@
>          if (has_second_rf && this_mode == NEWMV &&
>              mbmi->interp_filter == EIGHTTAP) {
>            // adjust src pointers
>            mi_buf_shift(x, i);
> +          if ((unsigned)cpi->sf.comp_inter_joint_search_thresh <= bsize) {

Or just change the type of comp_inter_joint_search_Thresh to BLOCK_SIZE directly. I don't see any real reason for it to be int.

@@ +2685,5 @@
>        // Initialize mv using single prediction mode result.
>        frame_mv[refs[0]].as_int = single_newmv[refs[0]].as_int;
>        frame_mv[refs[1]].as_int = single_newmv[refs[1]].as_int;
>  
> +      if ((unsigned)cpi->sf.comp_inter_joint_search_thresh <= bsize) {

See above.

::: media/libvpx/vp9/vp9_cx_iface.c
@@ +503,5 @@
>       * usage value. If the current usage value isn't found, use the
>       * values for usage case 0.
>       */
>      for (i = 0;
> +         extracfg_map[i].usage && (unsigned)extracfg_map[i].usage != cfg->g_usage;

See above.

@@ +583,5 @@
>      new_qc = (new_qc == MODE_BESTQUALITY)
>               ? MODE_SECONDPASS_BEST
>               : MODE_SECONDPASS;
>  
> +  if ((unsigned)ctx->oxcf.Mode != new_qc) {

See above.

::: media/libvpx/vpx/src/svc_encodeframe.c
@@ +227,5 @@
>    int retval = 0;
>    va_list ap;
>    SvcInternal *const si = get_svc_internal(svc_ctx);
>  
> +  if ((unsigned)level > svc_ctx->log_level) {

For jesup: "unsigned int".
Attachment #8389252 - Flags: review?(tterribe) → review-
> 
> The warnings are of two types:
> 1) Ignoring a return code. This happens in the macros that uses stlen. I
> used the available locally declared r variable to quiet the warning.
> 

   Does prepending a "(void)" not work? Reusing r strikes me as kinda dangerous here.

> 2) Setting and then not using a variable. This is caused by the ABORT macro:
> it assumes that a variable _status have been declared somewhere outside of
> the macro and always sets it to the value of _r. In some of the functions
> where ABORT is called _status is never used. This is mainly in void
> functions but does happen in one int returning function where the return
> value is always set to 0. This leads to generating code that is required to
> declare _status to be able to use ABORT.
> 
> What I did was use _status for the same purpose as _r so that _status would
> always be 'used'. For ABORT to work, _status must be in scope, but you're
> correct in that it doesn't necessarily have to be an int. And, we have this
> dependency on _status and ABORT. I am open to suggestions.

    I'm inclined to just convert them all over to returning an error code; we aren't terribly concerned with ABI movement with nICEr, and I'm pretty sure most of these aren't exported anyway. Or maybe "(void)" would work too. (Possibly even in ABORT?)
> ::: media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c
> @@ +163,5 @@
> >  {
> >    nr_socket_buffered_stun *sock = (nr_socket_buffered_stun *)obj;
> >  
> >    int r, _status;
> > +  size_t written = 0;
> 
> Is this triggering a warning? nr_socket_buffered_stun_write sets this iff it
> returns 0, which is a prerequisite for us using it.

One of the compilers almost certainly can't figure out that the other function set it to 0.  Paul?
(In reply to Randell Jesup [:jesup] from comment #51)
> > ::: media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c
> > @@ +163,5 @@
> > >  {
> > >    nr_socket_buffered_stun *sock = (nr_socket_buffered_stun *)obj;
> > >  
> > >    int r, _status;
> > > +  size_t written = 0;
> > 
> > Is this triggering a warning? nr_socket_buffered_stun_write sets this iff it
> > returns 0, which is a prerequisite for us using it.
> 
> One of the compilers almost certainly can't figure out that the other
> function set it to 0.  Paul?

Exactly. While I can satisfy myself that this value is initialized before used, the compiler for the Linux x64 build generates an "xxx may not be initialized before used" warning. Many of the warnings in the last two patches, parts 9 and 10, are either "set and never used" or "used and never set" type warnings, along with the ever present signed/unsigned comparisons.
Flags: needinfo?(ekr)
(In reply to Randell Jesup [:jesup] from comment #39)
> Comment on attachment 8389257 [details] [diff] [review]
> Part 00: Replace static var with string const for log tag
> 
> Review of attachment 8389257 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does this generate a warning?  If not and it can't be suppressed, then ok,
> we could land this - but i don't seen the point in optimizing it for "we
> have only one logging call" and then have to undo it when we add a second.

On review, my fix does block all the logTags defined in each module that includes sdp_private.h. As it stands, if the compiler does generate an inline copy for sdp_verify_sdp_ptr and the definition of logTag is missing a compiler error will be generated. However, if the compiler ignores the inline and links to sdp_verify_sdp_ptr it will use the logTag definition in sdp_main.c. This may differ between platforms.
> 
> @@ +5471,5 @@
> >      if ( cpi->cyclic_refresh_mode_enabled )
> >          return -1;
> >  
> >      // Check number of rows and columns match
> > +    if ((unsigned)cpi->common.mb_rows != rows || (unsigned)cpi->common.mb_cols != cols)
> 
> Are you sure this doesn't create a situation where someone could initialize
> a context with mb_rows and mb_cols set to an invalid, negative value (e.g.,
> -1), and then pass the validation checks here with a large positive value?
> The actual allocation code does not appear to check, and if both mb_rows and
> mb_cols are negative, will likely succeed, since most allocations compute
> their size as the product of these two values.
> 
The situation here is an explicit version to the type casting that the compiler will do when making the comparison. As long as the unsigned versions are equal this will pass. To cover your concern, a check for a negative value can be made before (left of) each cast and compare test in the if.
Attachment #8389254 - Flags: review?(jmvalin) → review-
Comment on attachment 8389252 [details] [diff] [review]
Part 05: libvpx - multiple unsigned vs signed comparison fixes

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

Note that for the third-party code in media we carry a separate patch file in the tree to simplify re-application of changes after an update (and merging changes upstream). In the next round please include a copy of the changes in a .patch file and add it to the 'apply_patches' function in update.py.

Not otherwise reviewing for content; derf seems to have that covered.
Attachment #8389252 - Flags: review?(giles) → review-
(In reply to Ralph Giles (:rillian) from comment #55)
> Comment on attachment 8389252 [details] [diff] [review]
> Part 05: libvpx - multiple unsigned vs signed comparison fixes
> 
> Review of attachment 8389252 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Note that for the third-party code in media we carry a separate patch file
> in the tree to simplify re-application of changes after an update (and
> merging changes upstream). In the next round please include a copy of the
> changes in a .patch file and add it to the 'apply_patches' function in
> update.py.
> 
> Not otherwise reviewing for content; derf seems to have that covered.

I have a pending version of patches that apply independently to each library versus the per-target compiler pass in my original set.
Comment on attachment 8392202 [details] [diff] [review]
Part 10: fixes for Android Debug build warnings

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

I'm only reviewing the cubeb bits.  r- based on these having already been addressed in other bugs.

::: media/libcubeb/src/cubeb_audiotrack.c
@@ +295,5 @@
>  audiotrack_get_min_latency(cubeb * ctx, cubeb_stream_params params, uint32_t * latency_ms)
>  {
>    /* We always use the lowest latency possible when using this backend (see
>     * audiotrack_stream_init), so this value is not going to be used. */
> +  int rv, count;

Declare this on a new line please.

@@ +300,2 @@
>  
> +  rv = audiotrack_get_min_frame_count(ctx, &params, &count);

Looks like this was fixed in bug 988827.

@@ +303,5 @@
>      return CUBEB_ERROR;
>    }
>  
>    /* Convert to milliseconds. */
> +  *latency_ms = (uint32_t)count * 1000 / params.rate;

Space between cast and count.

@@ +317,3 @@
>  
> +  rv = ctx->klass.get_output_samplingrate(&_rate, 3 /* MUSIC */);
> +  *rate = (uint32_t)_rate;

This was fixed in bug 986793.

::: media/libcubeb/src/cubeb_opensl.c
@@ +277,5 @@
>    /* Depending on which method we called above, we can get a zero back, yet have
>     * a non-error return value, especially if the audio system is not
>     * ready/shutting down (i.e. when we can't get our hand on the AudioFlinger
>     * thread). */
> +  if (*rate == 0) {

This has already been fixed in trunk (bug 980052), which version are you working against?
Attachment #8392202 - Flags: review?(kinetik) → review-
 
> This has already been fixed in trunk (bug 980052), which version are you
> working against?

This worked started on the 4-MAR-2014 version of mozilla-central.
Is there more here that's still relevant?  Lots of warning-fixes have landed in the last year from dholbert, etc.  Or should we close it and move any bits remaining and still relevant to new bugs?
Rank: 55
Flags: needinfo?(pkerr)
Priority: -- → P5
QA Contact: jsmith
Whiteboard: [WebRTC], [blocking-webrtc-]
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #59)
> Is there more here that's still relevant?  Lots of warning-fixes have landed
> in the last year from dholbert, etc.  Or should we close it and move any
> bits remaining and still relevant to new bugs?

Let me take a look at this patch. If I recall, I had gone a bit too wide in my warning fixes in the tree. I will take a focused look at just signalling/src to see what is left.
Flags: needinfo?(pkerr)
backlog: --- → webRTC+
Attachment #8389252 - Flags: review?(gmaxwell)

The patches here are quite out of date, and our warning situation seems to have improved since this was originally filed.

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: