12.38 - 17.35% compiler warnings (windows2012-32, windows2012-64) regression on push 329e64cecd9822d76fff76c0ef42b0f75ede30dd (Thu Aug 9 2018)

RESOLVED FIXED in Firefox 63

Status

()

defect
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: igoldan, Assigned: myk)

Tracking

(Blocks 1 bug, {regression})

Trunk
mozilla63
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox61 unaffected, firefox62 unaffected, firefox63 fixed)

Details

Attachments

(2 attachments)

We have detected a build metrics regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=329e64cecd9822d76fff76c0ef42b0f75ede30dd

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

 17%  compiler warnings windows2012-64 asan debug     899.00 -> 1,055.00
 17%  compiler warnings windows2012-64 asan opt       908.00 -> 1,064.00
 17%  compiler warnings windows2012-64 debug plain    924.00 -> 1,080.00
 17%  compiler warnings windows2012-64 debug          926.00 -> 1,082.00
 17%  compiler warnings windows2012-64 debug static-analysis928.00 -> 1,084.00
 17%  compiler warnings windows2012-64 opt static-analysis934.00 -> 1,090.00
 17%  compiler warnings windows2012-64 opt plain      934.00 -> 1,090.00
 17%  compiler warnings windows2012-64 opt rusttests  934.00 -> 1,090.00
 17%  compiler warnings windows2012-64 pgo            934.00 -> 1,090.00
 17%  compiler warnings windows2012-64 opt            934.00 -> 1,090.00
 13%  compiler warnings windows2012-32 debug          890.00 -> 1,002.00
 12%  compiler warnings windows2012-32 opt rusttests  898.00 -> 1,010.00
 12%  compiler warnings windows2012-32 pgo            898.00 -> 1,010.00
 12%  compiler warnings windows2012-32 opt            898.00 -> 1,010.00
 12%  compiler warnings windows2012-32 debug static-analysis899.00 -> 1,011.00
 12%  compiler warnings windows2012-32 opt static-analysis905.00 -> 1,017.00


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=14920

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Automated_Performance_Testing_and_Sheriffing/Build_Metrics
Product: Testing → Toolkit
Flags: needinfo?(myk)
Assignee

Comment 1

11 months ago
There are a few new Rust warnings in the added crate synstructure v0.9.0:

>    Compiling synstructure v0.9.0
> …
> … warning: use of deprecated item 'proc_macro2::TokenStream::empty': please use TokenStream::new
> …    --> third_party\rust\synstructure\src\lib.rs:544:21
> …     |
> … 544 |         let mut t = TokenStream::empty();
> …     |                     ^^^^^^^^^^^^^^^^^^
> …     |
> …     = note: #[warn(deprecated)] on by default
> … warning: use of deprecated item 'proc_macro2::TokenStream::empty': please use TokenStream::new
> …    --> third_party\rust\synstructure\src\lib.rs:624:21
> …     |
> … 624 |         let mut t = TokenStream::empty();
> …     |                     ^^^^^^^^^^^^^^^^^^
> … warning: use of deprecated item 'proc_macro2::TokenStream::empty': please use TokenStream::new
> …    --> third_party\rust\synstructure\src\lib.rs:693:24
> …     |
> … 693 |         let mut body = TokenStream::empty();
> …     |                        ^^^^^^^^^^^^^^^^^^
> … warning: use of deprecated item 'proc_macro2::TokenStream::empty': please use TokenStream::new
> …     --> third_party\rust\synstructure\src\lib.rs:1081:21
> …      |
> … 1081 |         let mut t = TokenStream::empty();
> …      |                     ^^^^^^^^^^^^^^^^^^
> … warning: use of deprecated item 'proc_macro2::TokenStream::empty': please use TokenStream::new
> …     --> third_party\rust\synstructure\src\lib.rs:1135:21
> …      |
> … 1135 |         let mut t = TokenStream::empty();
> …      |                     ^^^^^^^^^^^^^^^^^^
> … warning: use of deprecated item 'proc_macro2::TokenStream::empty': please use TokenStream::new
> …     --> third_party\rust\synstructure\src\lib.rs:1189:21
> …      |
> … 1189 |         let mut t = TokenStream::empty();
> …      |                     ^^^^^^^^^^^^^^^^^^

But the majority of the new warnings are in the C code added by lmdb-sys:

> …\mdb.c:1501 [C4996] 'strerror': This function or variable may be unsafe. Consider using strerror_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
> …\mdb.c:1531 [C4996] 'sprintf': This function or variable may be unsafe. Consider using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
> …\mdb.c:1825 [C4267] 'initializing': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:1919 [C4389] '==': signed/unsigned mismatch
> …\mdb.c:1946 [C4389] '==': signed/unsigned mismatch
> …\mdb.c:2008 [C4267] '+=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:2022 [C4267] 'initializing': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:2047 [C4267] '=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:2158 [C4267] 'initializing': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:2213 [C4127] conditional expression is constant
> …\mdb.c:2216 [C4127] conditional expression is constant
> …\mdb.c:2252 [C4267] '=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:2272 [C4267] '=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:2433 [C4127] conditional expression is constant
> …\mdb.c:2433 [C4267] '=': conversion from 'size_t' to 'unsigned short', possible loss of data
> …\mdb.c:2887 [C4267] '=': conversion from 'size_t' to 'int', possible loss of data
> …\mdb.c:2888 [C4267] 'function': conversion from 'size_t' to 'int', possible loss of data
> …\mdb.c:3263 [C4267] 'initializing': conversion from 'size_t' to 'int', possible loss of data
> …\mdb.c:3323 [C4267] '=': conversion from 'size_t' to 'DWORD', possible loss of data
> …\mdb.c:3324 [C4267] 'function': conversion from 'size_t' to 'DWORD', possible loss of data
> …\mdb.c:3467 [C4244] '=': conversion from 'unsigned int' to 'unsigned char', possible loss of data
> …\mdb.c:3473 [C4267] '=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:3477 [C4267] '=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:3513 [C4267] '=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:3516 [C4267] '=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:3531 [C4267] '=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:3586 [C4456] declaration of 'i' hides previous local declaration
> …\mdb.c:3838 [C4244] '+=': conversion from '__int64' to 'off_t', possible loss of data
> …\mdb.c:3951 [C4267] '=': conversion from 'size_t' to 'LONG', possible loss of data
> …\mdb.c:4193 [C4100] 'mode': unreferenced formal parameter
> …\mdb.c:4205 [C4996] 'wcscpy': This function or variable may be unsafe. Consider using wcscpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
> …\mdb.c:4297 [C4996] 'GetVersion': was declared deprecated
> …\mdb.c:4465 [C4100] 'module': unreferenced formal parameter
> …\mdb.c:4465 [C4100] 'ptr': unreferenced formal parameter
> …\mdb.c:4682 [C4244] 'function': conversion from 'mdb_hash_t' to 'unsigned long', possible loss of data
> …\mdb.c:4798 [C4996] 'sprintf': This function or variable may be unsafe. Consider using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
> …\mdb.c:4799 [C4996] 'sprintf': This function or variable may be unsafe. Consider using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
> …\mdb.c:4942 [C4996] 'strdup': The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _strdup. See online help for details.
> …\mdb.c:5189 [C4267] '=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:5192 [C4267] '=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:5197 [C4244] 'return': conversion from 'ssize_t' to 'int', possible loss of data
> …\mdb.c:5223 [C4244] 'return': conversion from 'ssize_t' to 'int', possible loss of data
> …\mdb.c:5315 [C4244] '=': conversion from 'unsigned int' to 'indx_t', possible loss of data
> …\mdb.c:5412 [C4456] declaration of 'x' hides previous local declaration
> …\mdb.c:5579 [C4457] declaration of 'flags' hides function parameter
> …\mdb.c:5671 [C4267] '=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:5679 [C4267] '=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:5691 [C4267] '=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:5692 [C4267] '=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:6044 [C4244] '=': conversion from 'unsigned int' to 'indx_t', possible loss of data
> …\mdb.c:6081 [C4244] '=': conversion from 'unsigned int' to 'indx_t', possible loss of data
> …\mdb.c:6297 [C4244] '=': conversion from 'int' to 'indx_t', possible loss of data
> …\mdb.c:6517 [C4267] '=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:6632 [C4267] '=': conversion from 'size_t' to 'uint16_t', possible loss of data
> …\mdb.c:6714 [C4267] '=': conversion from 'size_t' to 'uint16_t', possible loss of data
> …\mdb.c:6720 [C4267] '=': conversion from 'size_t' to 'indx_t', possible loss of data
> …\mdb.c:6776 [C4267] '=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\mdb.c:6785 [C4244] '=': conversion from 'unsigned int' to 'indx_t', possible loss of data
> …\mdb.c:6792 [C4244] '=': conversion from 'unsigned int' to 'indx_t', possible loss of data
> …\mdb.c:6811 [C4267] 'initializing': conversion from 'size_t' to 'int', possible loss of data
> …\mdb.c:6855 [C4146] unary minus operator applied to unsigned type, result still unsigned
> …\mdb.c:6863 [C4267] '=': conversion from 'size_t' to 'unsigned short', possible loss of data
> …\mdb.c:6910 [C4456] declaration of 'mp' hides previous local declaration
> …\mdb.c:6963 [C4456] declaration of 'mp' hides previous local declaration
> …\mdb.c:6986 [C4267] '=': conversion from 'size_t' to 'int', possible loss of data
> …\mdb.c:7146 [C4244] '=': conversion from 'uint32_t' to 'uint16_t', possible loss of data
> …\mdb.c:7148 [C4244] '=': conversion from 'unsigned int' to 'indx_t', possible loss of data
> …\mdb.c:7263 [C4267] '-=': conversion from 'size_t' to 'indx_t', possible loss of data
> …\mdb.c:7276 [C4267] 'initializing': conversion from 'size_t' to 'int', possible loss of data
> …\mdb.c:7303 [C4267] '=': conversion from 'size_t' to 'indx_t', possible loss of data
> …\mdb.c:7311 [C4267] '=': conversion from 'size_t' to 'unsigned short', possible loss of data
> …\mdb.c:7312 [C4244] '=': conversion from 'unsigned int' to 'unsigned short', possible loss of data
> …\mdb.c:7314 [C4267] '=': conversion from 'size_t' to 'unsigned short', possible loss of data
> …\mdb.c:7316 [C4267] '=': conversion from 'size_t' to 'unsigned short', possible loss of data
> …\mdb.c:7316 [C4127] conditional expression is constant
> …\mdb.c:7377 [C4267] '+=': conversion from 'size_t' to 'indx_t', possible loss of data
> …\mdb.c:7396 [C4244] '+=': conversion from 'unsigned int' to 'indx_t', possible loss of data
> …\mdb.c:7405 [C4244] '+=': conversion from 'unsigned int' to 'indx_t', possible loss of data
> …\mdb.c:7439 [C4333] '>>': right shift by too large amount, data loss
> …\mdb.c:7762 [C4244] '-=': conversion from 'int' to 'indx_t', possible loss of data
> …\mdb.c:7768 [C4244] '-=': conversion from 'int' to 'indx_t', possible loss of data
> …\mdb.c:7775 [C4267] '=': conversion from 'size_t' to 'unsigned short', possible loss of data
> …\mdb.c:7848 [C4244] '=': conversion from 'unsigned int' to 'unsigned short', possible loss of data
> …\mdb.c:7849 [C4244] '=': conversion from 'unsigned int' to 'unsigned short', possible loss of data
> …\mdb.c:7875 [C4244] '=': conversion from 'unsigned int' to 'unsigned short', possible loss of data
> …\mdb.c:7876 [C4244] '=': conversion from 'unsigned int' to 'unsigned short', possible loss of data
> …\mdb.c:7898 [C4267] 'function': conversion from 'size_t' to 'int', possible loss of data
> …\mdb.c:8060 [C4244] '=': conversion from 'unsigned int' to 'indx_t', possible loss of data
> …\mdb.c:8147 [C4244] '+=': conversion from 'unsigned int' to 'indx_t', possible loss of data
> …\mdb.c:8165 [C4244] '=': conversion from 'unsigned int' to 'unsigned short', possible loss of data
> …\mdb.c:8166 [C4244] '=': conversion from 'unsigned int' to 'unsigned short', possible loss of data
> …\mdb.c:8643 [C4244] '-=': conversion from 'unsigned int' to 'indx_t', possible loss of data
> …\mdb.c:8644 [C4244] '+=': conversion from 'unsigned int' to 'indx_t', possible loss of data
> …\mdb.c:8645 [C4244] '+=': conversion from 'unsigned int' to 'indx_t', possible loss of data
> …\mdb.c:8646 [C4244] '-=': conversion from 'unsigned int' to 'indx_t', possible loss of data
> …\mdb.c:8660 [C4267] '-=': conversion from 'size_t' to 'indx_t', possible loss of data
> …\mdb.c:8668 [C4267] '-=': conversion from 'size_t' to 'indx_t', possible loss of data
> …\mdb.c:8669 [C4244] '=': conversion from 'int' to 'indx_t', possible loss of data
> …\mdb.c:8676 [C4267] '=': conversion from 'size_t' to 'int', possible loss of data
> …\mdb.c:8678 [C4267] '=': conversion from 'size_t' to 'int', possible loss of data
> …\mdb.c:8690 [C4244] '=': conversion from 'unsigned int' to 'indx_t', possible loss of data
> …\mdb.c:8825 [C4244] '=': conversion from 'int' to 'indx_t', possible loss of data
> …\mdb.c:8844 [C4244] 'function': conversion from 'int' to 'indx_t', possible loss of data
> …\mdb.c:8941 [C4244] '-=': conversion from 'int' to 'indx_t', possible loss of data
> …\mdb.c:9168 [C4457] declaration of 'pg' hides function parameter
> …\mdb.c:9226 [C4457] declaration of 'pg' hides function parameter
> …\mdb.c:9260 [C4127] conditional expression is constant
> …\mdb.c:9260 [C4267] '=': conversion from 'size_t' to 'unsigned short', possible loss of data
> …\mdb.c:9447 [C4267] '=': conversion from 'size_t' to 'DWORD', possible loss of data
> …\mdb.c:9483 [C4267] '=': conversion from 'size_t' to 'DWORD', possible loss of data
> …\mdb.c:9762 [C4996] 'strdup': The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _strdup. See online help for details.
> …\mdb.c:9773 [C4456] declaration of 'dummy' hides previous local declaration
> …\mdb.c:9785 [C4244] '=': conversion from 'int' to 'unsigned char', possible loss of data
> …\mdb.c:9916 [C4244] '=': conversion from 'unsigned int' to 'indx_t', possible loss of data
> …\mdb.c:10033 [C4100] 'env': unreferenced formal parameter
> …\mdb.c:10056 [C4996] 'sprintf': This function or variable may be unsafe. Consider using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
> …\midl.c:44 [C4267] 'initializing': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\midl.c:146 [C4267] '+=': conversion from 'size_t' to 'unsigned int', possible loss of data
> …\midl.c:176 [C4267] 'function': conversion from 'size_t' to 'int', possible loss of data
> …
> …\mdb.c:1509 [C4172] returning address of local variable or temporary: buf
> …\mdb.c:2069 [C4706] assignment within conditional expression
> …\mdb.c:2219 [C4701] potentially uninitialized local variable 'last' used
> …\mdb.c:2254 [C4706] assignment within conditional expression
> …\mdb.c:2288 [C4706] assignment within conditional expression
> …\mdb.c:2421 [C4706] assignment within conditional expression
> …\mdb.c:2700 [C4706] assignment within conditional expression
> …\mdb.c:2727 [C4706] assignment within conditional expression
> …\mdb.c:2743 [C4706] assignment within conditional expression
> …\mdb.c:2863 [C4706] assignment within conditional expression
> …\mdb.c:3473 [C4706] assignment within conditional expression
> …\mdb.c:3619 [C4706] assignment within conditional expression
> …\mdb.c:4733 [C4706] assignment within conditional expression
> …\mdb.c:4934 [C4706] assignment within conditional expression
> …\mdb.c:4990 [C4706] assignment within conditional expression
> …\mdb.c:5495 [C4706] assignment within conditional expression
> …\mdb.c:5538 [C4706] assignment within conditional expression
> …\mdb.c:5620 [C4706] assignment within conditional expression
> …\mdb.c:6594 [C4706] assignment within conditional expression
> …\mdb.c:6602 [C4706] assignment within conditional expression
> …\mdb.c:6774 [C4706] assignment within conditional expression
> …\mdb.c:7037 [C4706] assignment within conditional expression
> …\mdb.c:7114 [C4706] assignment within conditional expression
> …\mdb.c:7142 [C4706] assignment within conditional expression
> …\mdb.c:7284 [C4706] assignment within conditional expression
> …\mdb.c:7817 [C4706] assignment within conditional expression
> …\mdb.c:8052 [C4706] assignment within conditional expression
> …\mdb.c:8578 [C4706] assignment within conditional expression
> …\mdb.c:8589 [C4706] assignment within conditional expression
> …\mdb.c:9286 [C4706] assignment within conditional expression
> …\mdb.c:9322 [C4706] assignment within conditional expression
> …\mdb.c:9435 [C4706] assignment within conditional expression
> …\mdb.c:9473 [C4706] assignment within conditional expression
> …\mdb.c:10151 [C4706] assignment within conditional expression
> …\mdb.c:10215 [C4706] assignment within conditional expression
> …\midl.c:123 [C4706] assignment within conditional expression
> …\midl.c:149 [C4706] assignment within conditional expression

Nathan: do we have a policy for how to handle such warnings?  I can work with upstream to address them, but I expect that to be a extended process (with uncertain result, depending on how amenable the upstream maintainers are to accepting fixes for such warnings).
Flags: needinfo?(myk) → needinfo?(nfroyd)
For Rust warnings, we just try to work with upstream.

I think we have to just deal with the warnings from the C code as well, though I thought Ted was making it so the compiler that Cargo thinks it should be invoking didn't warn for many things?
Flags: needinfo?(nfroyd) → needinfo?(ted)
Assignee

Comment 3

11 months ago
(In reply to Nathan Froyd [:froydnj] from comment #2)
> For Rust warnings, we just try to work with upstream.

I submitted https://github.com/mystor/synstructure/pull/17 to fix these upstream.
Assignee

Comment 4

11 months ago
It looks like third_party/rust/bzip2-sys/build.rs disables warnings when building libbz2:

https://searchfox.org/mozilla-central/source/third_party/rust/bzip2-sys/build.rs#7

Ted: would that be a reasonable solution here?
(In reply to Nathan Froyd [:froydnj] from comment #2)
> For Rust warnings, we just try to work with upstream.
> 
> I think we have to just deal with the warnings from the C code as well,
> though I thought Ted was making it so the compiler that Cargo thinks it
> should be invoking didn't warn for many things?

I disabled warnings-as-errors for C code compiled via Rust build scripts in bug 1409276, but I didn't disable warnings:
https://hg.mozilla.org/mozilla-central/rev/5f20a8a55268

(In reply to Myk Melez [:myk] [@mykmelez] from comment #4)
> It looks like third_party/rust/bzip2-sys/build.rs disables warnings when
> building libbz2:
> 
> https://searchfox.org/mozilla-central/source/third_party/rust/bzip2-sys/
> build.rs#7
> 
> Ted: would that be a reasonable solution here?

That seems fine since it's all third-party code anyway, but my patches in bug 1409276 probably make us enable warnings anyway since we'll now be passing down all our CFLAGS, which include a bunch of -Whatever.

Unfortunately I don't think it'd be straightforward to remove warning flags from our set of cflags for Rust compiles, as they're currently just jammed into CFLAGS in old-configure:
https://dxr.mozilla.org/mozilla-central/rev/4e56a2f51ad739ca52046723448f3129a58f1666/old-configure.in#4371
Flags: needinfo?(ted)
Assignee

Comment 6

11 months ago
Indeed, tracing the code, it looks like the cc crate first populates Tool::args from CFLAGS:

https://github.com/alexcrichton/cc-rs/blob/master/src/lib.rs#L1122-L1124

And then adds a warning arg if warnings are enabled (which they are by default):

https://github.com/alexcrichton/cc-rs/blob/master/src/lib.rs#L1297-L1300

But it doesn't remove a warning arg from Tool::args if warnings are explicitly disabled via `Build::warnings(false)`.  So a warning arg in CFLAGS overrides a call to `Build::warnings(false)`.


Assuming acrichto (cc:ed) is amenable, we could use the "removed_args" feature that jdm landed recently in the cc crate to make the crate strip out the warning arg if warnings are explicitly disabled:

https://github.com/alexcrichton/cc-rs/commit/d37bd811a2319fe5b4c78860cb355b047798791b

And then I could ask the maintainer of the lmdb-sys crate to explicitly disable warnings; and fork that crate to make the change myself if they decline.

Alternately, we could decide to accept the regression in warning count, since it's in third-party code (twice over) rather than our own; although I'm unsure who would make that decision.

Ted: do you have a preference or a third alternative?
Flags: needinfo?(ted)
It seems difficult to get this right from the perspective of the build script or the cc crate. It's kind of on us for passing all those flags in in CFLAGS. I'd say we either fix this by giving ourselves a way to elide warnings flags from moz.build (and then using it in moz.build files where we build Rust code), or we just take the warnings regression. The whole point of tracking warnings is just to try to keep build output readable, so it'd be unfortunate to do that of course.

After writing comment 5 I realized that we probably could move `WARNINGS_CFLAGS` out of old-configure, since the only thing that happens there is we add `-Qunused-arguments` to it when compiling with clang, then it gets inserted into `CFLAGS`. We could instead set a `WARNINGS_CFLAGS` var and plumb that through the `COMPILE_FLAGS` setup so we could do something like `COMPILE_FLAGS['WARNINGS'] = []`.
Flags: needinfo?(ted)
FWIW I'd be definitely willing to have `cc` updated to strip out any flag that looks like a warning from CFLAGS if warnings are disabled. I agree with Ted that it's sort of tricky here but it seems like it could help at least for `cc` to strip those out! (I'm not sure if categorizing with 100% fidelity is possible, but we can tweak the heuristics over time)
Assignee

Comment 9

10 months ago
MozReview-Commit-ID: 9CZgLGbWjbA
Assignee

Comment 10

10 months ago
(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #7)
> After writing comment 5 I realized that we probably could move
> `WARNINGS_CFLAGS` out of old-configure, since the only thing that happens
> there is we add `-Qunused-arguments` to it when compiling with clang, then
> it gets inserted into `CFLAGS`. We could instead set a `WARNINGS_CFLAGS` var
> and plumb that through the `COMPILE_FLAGS` setup so we could do something
> like `COMPILE_FLAGS['WARNINGS'] = []`.

I have a patch that does this more or less, I think.  It's hard to say for sure because I haven't been able to reproduce the warnings locally (and also because I'm unfamiliar with the build system code).  I've pushed it to tryserver to see how it does there:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b076febb54f128543737435183d8fcefe4639bdd

Note: the patch sets separate WARNINGS_CFLAGS and WARNINGS_CXXFLAGS vars because old-configure distinguishes between them, and Rust crates can compile both C and C++ code (although this particular regression is the result of C code compilation).

Not asking for review yet, pending results on tryserver and further attempts to reproduce locally.
Assignee: nobody → myk
Status: NEW → ASSIGNED
Assignee

Comment 11

10 months ago
(In reply to Alex Crichton [:acrichto] from comment #8)
> FWIW I'd be definitely willing to have `cc` updated to strip out any flag
> that looks like a warning from CFLAGS if warnings are disabled. I agree with
> Ted that it's sort of tricky here but it seems like it could help at least
> for `cc` to strip those out! (I'm not sure if categorizing with 100%
> fidelity is possible, but we can tweak the heuristics over time)

Would you instead be amenable to `cc` not enabling warnings by default if CFLAGS is defined?

Currently, `cc` enables warnings by default, even if CFLAGS is defined, which means that each individual crate must explicitly disable them via `Build::warnings(false)`.

That's straightforward for a crate that Mozilla maintains, like bzip2-sys, where you disabled warnings in https://github.com/alexcrichton/bzip2-rs/commit/fd2c7257ce8d15d20b25dd0b8db3868fe9b9895c.  But it's trickier for a true third-party crate like lmdb-sys, whose author may disagree about disabling warnings.

Since Mozilla's build system defines CFLAGS, it has presumably already determined the warning flags that it wishes to set.  And, if my patch here is successful, it won't set those flags when compiling Rust crates.

Thus it would be helpful if `cc` didn't override that choice by enabling warnings anyway, so we don't have to land changes to disable warnings upstream into lmdb-sys and every other third-party crate that compiles C code.
Flags: needinfo?(acrichton)
Seems reasonable to me!
Flags: needinfo?(acrichton)
Myk, are you proceeding as stated, then, in #c11?
Flags: needinfo?(myk)
Assignee

Comment 14

10 months ago
(In reply to David Durst [:ddurst] (REO for 63) from comment #13)
> Myk, are you proceeding as stated, then, in #c11?

Yes, that's right.  I requested https://github.com/alexcrichton/cc-rs/pull/342 for the upstream changes in the cc crate that are needed to support this approach, and acrichto has merged those changes.  I also updated the patch in https://phabricator.services.mozilla.com/D3939 and requested review from Ted, who redirected the request to Chris Manchester.

There's still an issue on Windows that I need to resolve, but overall the approach seems viable, so it's the one I'm pursuing.
Flags: needinfo?(myk)
Assignee

Comment 15

10 months ago
Update: I resolved the Windows issue in https://phabricator.services.mozilla.com/D3939 and requested publication of a new version of the cc crate to crates.io in https://github.com/alexcrichton/cc-rs/pull/343 so that I can then vendor it into mozilla-central.
Comment on attachment 9002975 [details]
Bug 1482810 - set COMPILE_FLAGS var to hide warnings for Rust crates

Chris Manchester (:chmanchester) has approved the revision.
Attachment #9002975 - Flags: review+
Assignee

Comment 17

10 months ago
The fix for bug 1482810 requires an upstream change to the cc crate: if CFLAGS is defined, then don't enable warnings by default.  That change was included in cc crate version 1.0.23, and this change vendors that version of the cc crate into mozilla-central.

MozReview-Commit-ID: Krfrs1dSN9d
Comment on attachment 9005260 [details]
Bug 1482810 - update cc crate from 1.0.18 to 1.0.23

Nathan Froyd [:froydnj] has approved the revision.
Attachment #9005260 - Flags: review+

Updated

10 months ago
Attachment #9005260 - Attachment description: update cc crate from 1.0.18 to 1.0.23 → Bug 1482810 - update cc crate from 1.0.18 to 1.0.23

Comment 19

10 months ago
Pushed by myk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b9197ece7955
set COMPILE_FLAGS var to hide warnings for Rust crates r=chmanchester

Comment 20

10 months ago
Pushed by myk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7eae34bda99c
update cc crate from 1.0.18 to 1.0.23 r=froydnj
Assignee

Updated

10 months ago
Blocks: 1487477

Comment 21

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b9197ece7955
https://hg.mozilla.org/mozilla-central/rev/7eae34bda99c
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.