Compiler warnings for nICEr and nrappkit

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
Rank:
29
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: mjf, Assigned: mjf)

Tracking

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(4 attachments)

Here are the compiler warnings I see on OS X builds with clang 8.1.0:
5:27.84 warning: media/mtransport/third_party/nICEr/src/ice/ice_component.c:998:5 [-Wmissing-prototypes] no previous prototype for function 'nr_ice_component_can_candidate_tcptype_pair'
5:27.84 warning: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1015:5 [-Wmissing-prototypes] no previous prototype for function 'nr_ice_component_can_candidate_addr_pair'
5:27.84 warning: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1152:5 [-Wmissing-prototypes] no previous prototype for function 'nr_ice_pre_answer_enqueue'
5:27.84 warning: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1326:5 [-Wmissing-prototypes] no previous prototype for function 'nr_ice_component_refresh_consent'
5:27.84 warning: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1340:6 [-Wmissing-prototypes] no previous prototype for function 'nr_ice_component_consent_calc_consent_timer'
5:27.84 warning: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1433:5 [-Wmissing-prototypes] no previous prototype for function 'nr_ice_component_setup_consent'
5:27.84 warning: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:919:14 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
5:27.84 warning: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c:730:5 [-Wimplicit-function-declaration] implicit declaration of function 'nr_ice_component_consent_destroy' is invalid in C99
5:27.84 warning: media/mtransport/third_party/nICEr/src/net/transport_addr.c:478:26 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_build.c:328:53 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c:246:25 [-Wsign-compare] comparison of integers of different signs: 'int' and 'UINT4' (aka 'unsigned int')
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c:256:5 [-Wimplicit-function-declaration] implicit declaration of function 'nr_ice_accumulate_count' is invalid in C99
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c:282:25 [-Wsign-compare] comparison of integers of different signs: 'int' and 'UINT4' (aka 'unsigned int')
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c:405:29 [-Wsign-compare] comparison of integers of different signs: 'int' and 'UINT4' (aka 'unsigned int')
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_codec.c:92:28 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_codec.c:108:28 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_codec.c:124:28 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_codec.c:155:28 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_codec.c:172:28 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_codec.c:189:28 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_codec.c:231:25 [-Wsign-compare] comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_codec.c:564:17 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_codec.c:640:21 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_codec.c:971:24 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_codec.c:974:28 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_codec.c:1220:19 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_codec.c:1393:40 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_codec.c:1467:39 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_hint.c:70:39 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.84 warning: media/mtransport/third_party/nICEr/src/stun/stun_hint.c:170:39 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.85 warning: media/mtransport/third_party/nICEr/src/stun/stun_hint.c:187:39 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.85 warning: media/mtransport/third_party/nICEr/src/stun/stun_hint.c:204:39 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.85 warning: media/mtransport/third_party/nICEr/src/stun/stun_hint.c:224:39 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.85 warning: media/mtransport/third_party/nICEr/src/stun/stun_msg.c:77:16 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
5:27.85 warning: media/mtransport/third_party/nICEr/src/stun/stun_msg.c:228:56 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.85 warning: media/mtransport/third_party/nICEr/src/stun/stun_util.c:97:29 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
5:27.85 warning: media/mtransport/third_party/nICEr/src/stun/stun_util.c:217:24 [-Wsign-compare] comparison of integers of different signs: 'unsigned long' and 'int'
5:27.85 warning: media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c:260:9 [-Wimplicit-function-declaration] implicit declaration of function 'nr_ice_accumulate_count' is invalid in C99
5:27.85 warning: media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c:905:15 [-Wsign-compare] comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int'
Assignee: nobody → mfroman
Rank: 29
Priority: -- → P2
Friendly reminder to review or postpone :-)
Flags: needinfo?(ekr)
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
Summary: Compiler warnings for nICEr → Compiler warnings for nICEr and nrappkit
Comment on attachment 8911893 [details]
Bug 1374699 - fixing compiler warnings for nrappkit.

https://reviewboard.mozilla.org/r/183264/#review195528

::: media/mtransport/third_party/nrappkit/src/log/r_log.c:199
(Diff revision 1)
>        log_types[i].level[j]=LOG_LEVEL_UNDEFINED;
>  
>        if(NR_reg_initted()){
>  
>          if(snprintf(dest_prefix,sizeof(NR_registry),
> -          "logging.%s.facility",log_destinations[j].dest_name)>=sizeof(NR_registry))
> +          "logging.%s.facility",log_destinations[j].dest_name)>=(int)sizeof(NR_registry))

For all these, you need to upcast to size_t not downcast to int

::: media/mtransport/third_party/nrappkit/src/log/r_log.c
(Diff revision 1)
>  
>      *lt_level=level;
> -
> -    _status=0;
> -  abort:
> -    return;

Please do not make this change. We deliberately use ABORT(), etc. to make room for cleanup code and because you can log on ABORT()

::: media/mtransport/third_party/nrappkit/src/log/r_log.c:564
(Diff revision 1)
>  
>        /* Get the data out of the registry */
>        for(i=0; i<LOG_NUM_DESTINATIONS; i++){
>          /* set callback for default level */
>          if(snprintf(reg_key,sizeof(reg_key),"%s.%s.level",LOGGING_REG_PREFIX,
> -          log_destinations[i].dest_name)>=sizeof(reg_key))
> +          log_destinations[i].dest_name)>=(int)sizeof(reg_key))

You should upcast to size_t, not downcast to int, here and below.

::: media/mtransport/third_party/nrappkit/src/registry/registry_local.c:997
(Diff revision 1)
>  NRREGSET(nr_reg_local_set_uint4,    NR_REG_TYPE_UINT4,    UINT4)
>  NRREGSET(nr_reg_local_set_int8,     NR_REG_TYPE_INT8,     INT8)
>  NRREGSET(nr_reg_local_set_uint8,    NR_REG_TYPE_UINT8,    UINT8)
>  NRREGSET(nr_reg_local_set_double,   NR_REG_TYPE_DOUBLE,   double)
>  
> +#undef NRREGSET // undef so it doesn't collide with registry.c version

What's going on here? these are separate source files. See also above.

::: tools/rewriting/ThirdPartyPaths.txt
(Diff revision 1)
>  media/libtheora/
>  media/libtremor/
>  media/libvorbis/
>  media/libvpx/
>  media/libyuv/
> -media/mtransport/third_party/nrappkit

What does this change do?
Attachment #8911893 - Flags: review?(ekr) → review-
Comment on attachment 8911894 [details]
Bug 1374699 - fix .gyp file formatting by detabbing.

https://reviewboard.mozilla.org/r/183266/#review195534
Attachment #8911894 - Flags: review?(ekr) → review+
Comment on attachment 8911895 [details]
Bug 1374699 - make warnings errors on nICEr and nrappkit builds.

https://reviewboard.mozilla.org/r/183268/#review195536

::: media/mtransport/third_party/nrappkit/nrappkit.gyp:186
(Diff revision 1)
>  
>                   'sources': [
>                        './src/port/darwin/include/csi_platform.h',
>                   ],
>                }],
>                

Maybe fix this while you're in here.
Attachment #8911895 - Flags: review?(ekr) → review+
Comment on attachment 8887243 [details]
Bug 1374699 - fixing compiler warnings for nICEr.

https://reviewboard.mozilla.org/r/158044/#review195538

I see a lot of unnecessary name changes here. Please revert those so that this is easier to review, and then I will take a look.

::: media/mtransport/third_party/nrappkit/src/util/libekr/r_data.c:93
(Diff revision 2)
>  #include <string.h>
>  
> -int r_data_create(dp,d,l)
> +int r_data_create(dp,d,len)
>    Data **dp;
>    const UCHAR *d;
> -  int l;
> +  size_t len;

Please don't just wantonly rename stuff. The convention in this code is for short names and it makes reading this diff much harder.

::: tools/rewriting/ThirdPartyPaths.txt:43
(Diff revision 2)
>  media/libtheora/
>  media/libtremor/
>  media/libvorbis/
>  media/libvpx/
>  media/libyuv/
> -media/mtransport/third_party/
> +media/mtransport/third_party/nrappkit

What does this do?
Attachment #8887243 - Flags: review?(ekr) → review-
Comment on attachment 8887243 [details]
Bug 1374699 - fixing compiler warnings for nICEr.

https://reviewboard.mozilla.org/r/158044/#review195538

> Please don't just wantonly rename stuff. The convention in this code is for short names and it makes reading this diff much harder.

Sorry to double up a readablity fix with the compiler warnings fix.  I'll revert the name changes.

> What does this do?

Normally during mach builds, warnings are spit out mid-stream and then again at the end of the build process.  The ThirdPartyPaths.txt file causes the second set of warnings emitted at the end of the build to be rolled up into a line that looks like:
 2:57.61 (suppressed 63 warnings in media/mtransport/third_party)
 
In this case, I'm ending the rollup for media/mtransport/third_party/nICEr, but leaving it for nrappkit since I haven't fixed those warnings in this patch.
Comment on attachment 8911893 [details]
Bug 1374699 - fixing compiler warnings for nrappkit.

https://reviewboard.mozilla.org/r/183264/#review195528

> What's going on here? these are separate source files. See also above.

This is a side effect of unified builds.  I can either leave the undefs in registry.c and registry_local.c or I can add these two files to the list of non-unified sources in media/mtransport/third_party/moz.build.  Please let me know which option you'd prefer.

> What does this change do?

More complete explanation in the nICEr review, but here I'm ending the warnings rollup for all of media/mtransport/third_party now that I've fixed the warnings in nrappkit.
Comment on attachment 8887243 [details]
Bug 1374699 - fixing compiler warnings for nICEr.

https://reviewboard.mozilla.org/r/158044/#review233590

Closer, but still some iseuss.

::: media/mtransport/nricectx.cpp:739
(Diff revision 3)
>  void NrIceCtx::AccumulateStats(const NrIceStats& stats) {
> -  nr_ice_accumulate_count(&(ctx_->stats.stun_retransmits),
> +  nr_accumulate_count(&(ctx_->stats.stun_retransmits),
> -                          stats.stun_retransmits);
> +                      stats.stun_retransmits);
> -  nr_ice_accumulate_count(&(ctx_->stats.turn_401s), stats.turn_401s);
> -  nr_ice_accumulate_count(&(ctx_->stats.turn_403s), stats.turn_403s);
> -  nr_ice_accumulate_count(&(ctx_->stats.turn_438s), stats.turn_438s);
> +  nr_accumulate_count(&(ctx_->stats.turn_401s), stats.turn_401s);
> +  nr_accumulate_count(&(ctx_->stats.turn_403s), stats.turn_403s);
> +  nr_accumulate_count(&(ctx_->stats.turn_438s), stats.turn_438s);



::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:822
(Diff revision 3)
>        ABORT(r);
>      }
>  
>      _status=0;
>    abort:
> -    return(r);
> +    return(_status);

Good

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c
(Diff revision 3)
> -      // don't rollover, just stop accumulating at MAX value
> -      *orig_count = UINT2_MAX;
> -    } else {
> -      *orig_count += new_count;
> -    }
> -  }

What happened here?

::: media/mtransport/third_party/nICEr/src/stun/stun_codec.h:46
(Diff revision 3)
>  
>  typedef struct nr_stun_attr_codec_ {
>      char     *name;
>      int     (*print)(nr_stun_attr_info *attr_info, char *msg, void *data);
> -    int     (*encode)(nr_stun_attr_info *attr_info, void *data, int offset, int buflen, UCHAR *buf, int *attrlen);
> -    int     (*decode)(nr_stun_attr_info *attr_info, int attrlen, UCHAR *buf, int offset, int buflen, void *data);
> +    int     (*encode)(nr_stun_attr_info *attr_info, void *data, UINT2 offset, UINT2 buflen, UCHAR *buf, UINT2 *attrlen);
> +    int     (*decode)(nr_stun_attr_info *attr_info, UINT2 attrlen, UCHAR *buf, UINT2 offset, UINT2 buflen, void *data);

I don't think you should be using UINT2 for lengths, even of things which cannot be > 2^16-1. That's what size_t is for

::: media/mtransport/third_party/nICEr/src/stun/stun_msg.c:241
(Diff revision 3)
>  
>  int
>  nr_stun_message_add_nonce_attribute(nr_stun_message *msg, char *nonce)
>  NR_STUN_MESSAGE_ADD_ATTRIBUTE(
>      NR_STUN_ATTR_NONCE,
> -    { strlcpy(attr->u.nonce, nonce, sizeof(attr->u.nonce)); }
> +    // prepend (void) to ignore return value

I don't thinkt his comment is necessary here or elsewhere

::: tools/rewriting/ThirdPartyPaths.txt:43
(Diff revision 3)
>  media/libtheora/
>  media/libtremor/
>  media/libvorbis/
>  media/libvpx/
>  media/libyuv/
> -media/mtransport/third_party/
> +media/mtransport/third_party/nrappkit

What is going on here? You shouldn't be rewriting nICEr either.
Attachment #8887243 - Flags: review?(ekr) → review-
Comment on attachment 8911893 [details]
Bug 1374699 - fixing compiler warnings for nrappkit.

https://reviewboard.mozilla.org/r/183264/#review195528

> This is a side effect of unified builds.  I can either leave the undefs in registry.c and registry_local.c or I can add these two files to the list of non-unified sources in media/mtransport/third_party/moz.build.  Please let me know which option you'd prefer.

I'd prefer these be non-unified builds. This code was designed to one file per translation unit.

> More complete explanation in the nICEr review, but here I'm ending the warnings rollup for all of media/mtransport/third_party now that I've fixed the warnings in nrappkit.

OK. What I want to make sure is that this code isn't reformatted in some sort of mass rewrite pass. Isn't this file used for that as well?
Comment on attachment 8911893 [details]
Bug 1374699 - fixing compiler warnings for nrappkit.

https://reviewboard.mozilla.org/r/183264/#review195528

OK, so r+ once you 

1. de-unify these files 
2. Verify that you're not leaving things open to restyling

I can't figure out how to do that with Mozreview, but here it is
Comment on attachment 8887243 [details]
Bug 1374699 - fixing compiler warnings for nICEr.

https://reviewboard.mozilla.org/r/158044/#review233590

> What happened here?

This function was moved to stun_util.{h|c} because it was going to be used in more than just nICEr/src/ice/*

> I don't think you should be using UINT2 for lengths, even of things which cannot be > 2^16-1. That's what size_t is for

I used UINT2 here because I was concerned that the size of size_t is platform dependent.  If you are comfortable with that, I'll make that change.

> What is going on here? You shouldn't be rewriting nICEr either.

Ah - I wasn't aware of that part of this file's usage.  I'll revert this change (and for nrappkit).
Comment on attachment 8887243 [details]
Bug 1374699 - fixing compiler warnings for nICEr.

https://reviewboard.mozilla.org/r/158044/#review233590

> I used UINT2 here because I was concerned that the size of size_t is platform dependent.  If you are comfortable with that, I'll make that change.

Why is it a problem if it is platform dependent
Comment on attachment 8887243 [details]
Bug 1374699 - fixing compiler warnings for nICEr.

https://reviewboard.mozilla.org/r/158044/#review233590

> Why is it a problem if it is platform dependent

I was concerned that might cause interop issues.  If not, then size_t is definitely better.
Comment on attachment 8887243 [details]
Bug 1374699 - fixing compiler warnings for nICEr.

https://reviewboard.mozilla.org/r/158044/#review233590

> I was concerned that might cause interop issues.  If not, then size_t is definitely better.

I suppose after consideration, int could also be platform dependent, so there isn't much worry there.
Michael, I think we're done here.
Flags: needinfo?(ekr)
Comment on attachment 8887243 [details]
Bug 1374699 - fixing compiler warnings for nICEr.

https://reviewboard.mozilla.org/r/158044/#review245456

Since Ekr is done, I'll give the final LGTM
Attachment #8887243 - Flags: review+
Comment on attachment 8911893 [details]
Bug 1374699 - fixing compiler warnings for nrappkit.

https://reviewboard.mozilla.org/r/183264/#review245458

Since Ekr is done, I'll give the final LGTM
Attachment #8911893 - Flags: review+
Attachment #8887243 - Flags: review?(ekr)
Attachment #8911893 - Flags: review?(ekr)
Attachment #8887243 - Flags: review?(ekr)
Attachment #8911893 - Flags: review?(ekr)
Pushed by mfroman@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/92040641407c
fixing compiler warnings for nICEr. r=drno
https://hg.mozilla.org/integration/autoland/rev/d54c0f9794a4
fixing compiler warnings for nrappkit. r=drno
https://hg.mozilla.org/integration/autoland/rev/f99813328ce6
fix .gyp file formatting by detabbing. r=ekr
https://hg.mozilla.org/integration/autoland/rev/b32423756cb7
make warnings errors on nICEr and nrappkit builds. r=ekr
You need to log in before you can comment on or make changes to this bug.