Re: Improving visibility of compiler warnings

[Joshua Cranmer ]

On 5/25/17 6:11 PM, Eric Rahm wrote:

I think we disable it for local builds because we don't control what
versions of tools folks use. So clang vFOO might spit out errors we don't
see in clang vBAR and it would be a huge pain if those failed locally even
though they'd be fine in automation.


It should be possible to check the compiler and version and enable it by 
default if it's the same version as the ones on our check-in infrastructure.


--
Joshua Cranmer
Thunderbird and DXR developer
Source code archæologist

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Ehsan Akhgari]

On 05/25/2017 02:14 PM, Gregory Szorc wrote:
On Thu, May 25, 2017 at 5:31 AM, Ehsan Akhgari 
<ehsan.akhg...@gmail.com <mailto:ehsan.akhg...@gmail.com>> wrote:


What was the motivation behind this change?  Was there a complaint
from a significant number of developers about it being difficult
fixing compiler warnings grepping for things like "warning:" or
using ./mach warnings-list? Was the feedback from developers who
have the use case of writing code and fix compiler errors in such
code taken into account?


The motification was that relevant compiler warnings are flying under 
the radar and are detected too late in the development process - such 
as when pushing to Try. We want to catch problems as early in the 
development cycle as possible to reduce the end-to-end times for patch 
development. There have been complaints from developers over the years 
about this. What specifically motivated me to make this change 
recently was finding a pattern of feature requests during bug triage 
that all seemed to point to improving visibility of compiler warnings 
as a potential solution.


I fully acknowledge that the spew of warnings at the end of the build 
is annoying. This was expected to cause temporary pain. The hope was 
that this would spur people into fixing the warnings.


FWIW, IMO this is not OK.  I don't think there is any consensus that 
fixing compiler warnings is suddenly of such a great importance that we 
need to interrupt everyone's different workflows without them having 
opted into doing the work to fix the warnings.  And it may very well 
turn out that a project with a different format that isn't structured 
around the tools forcing people into fixing things may be more suitable 
to efforts like this, as years of work on 
https://bugzilla.mozilla.org/show_bug.cgi?id=buildwarning shows.


There has been some movement there. ICU warnings were suppressed, for 
example. And, there has been talk in this thread (and presumably 
elsewhere) about fixing other large offenders. So on that front, I'd 
say this change was successful.


I hope in the future you would consider discussing disruptive changes 
like this first instead of announcing them post-facto. Maybe at least we 
would have been able to think of a better initial implementation 
including not emitting the list for third-party library warnings or 
failed builds.  Or maybe we should have allowed people to opt in first.  
As the bug above shows, there are people who are interested to help in 
this area.


But the pace of progress hasn't been terrific and the status quo is 
annoying. This thread is also sapping up people's time. I've prepared 
a patch to tweak the behavior that I think strikes a compromise. I'll 
file a bug to track the review shortly.
Thank you.  I think most of the immediate productivity drain issues are 
fixed for now.


Cheers,
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Gregory Szorc]

On Thu, May 25, 2017 at 4:11 PM, Eric Rahm  wrote:

> I think we disable it for local builds because we don't control what
> versions of tools folks use. So clang vFOO might spit out errors we don't
> see in clang vBAR and it would be a huge pain if those failed locally even
> though they'd be fine in automation.
>

Exactly.

When we offer a build mode that uses the exact toolchain that automation is
using and can guarantee that the local environment reasonably approximates
CI, we can enable --enable-warnings-as-errors by default in that
configuration. We'll likely need a flag on `mach build` to disable that
warnings as errors per invocation, as sometimes developers just want to get
code to compile, even if there are warnings. But the default should be to
match CI so there are fewer surprises and errors are caught quicker. Of
course, the more we write Rust the more warnings as errors becomes the
explicit behavior because Rust just flat out refuses to compile problems
that C++ compilers treat as warnings.


>
> On Thu, May 25, 2017 at 4:07 PM, Andrew McCreight 
> wrote:
>
> > On Thu, May 25, 2017 at 4:03 PM,  wrote:
> >
> > > On Friday, May 26, 2017 at 6:03:02 AM UTC+12, Chris Peterson wrote:
> > > > On 2017-05-25 5:31 AM, Ehsan Akhgari wrote:
> > > > > On 05/19/2017 02:44 PM, Gregory Szorc wrote:
> > > > >> `mach build` attempts to parse compiler warnings to a persisted
> > > > >> "database."
> > > > >> You can view a list of compiler warnings post build by running
> `mach
> > > > >> warnings-list`. The intent behind this feature was to make it
> easier
> > > to
> > > > >> find and fix compiler warnings. After all, something out of sight
> is
> > > > >> out of
> > > > >> mind.
> > > >
> > > > Given that we treat warnings as errors on CI and most directories
> that
> > > > opt out of warnings-as-errors are third-party code
> > > > (ALLOW_COMPILER_WARNINGS in moz.build [1]), I don't think we need to
> > > > make the warning list any more visible. A warning regression in
> nearly
> > > > all first-party code is already treated as an error.
> > > >
> > > >
> > > > [1]
> > > > https://searchfox.org/mozilla-central/search?q=ALLOW_
> > > COMPILER_WARNINGS=true=moz.build
> > >
> > > Tangentially: "we treat warnings as errors on CI" -- It'd be great if
> > > local builds could have that behavior by default, so we (or just I?)
> > don't
> > > get caught by surprise when warnings-as-errors appear after landing.
> > >
> > > Or at least: Can I opt-in locally? (I tried 'ac_add_options
> > > --enable-warnings-ar-errors' in mozconfig, but that was not
> recognized.)
> > >
> >
> > This has worked for me before:
> > ac_add_options --enable-warnings-as-errors
> >
> >
> > ___
> > > dev-platform mailing list
> > > dev-platform@lists.mozilla.org
> > > https://lists.mozilla.org/listinfo/dev-platform
> > >
> > ___
> > dev-platform mailing list
> > dev-platform@lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
> >
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[gsquelart]

On Friday, May 26, 2017 at 11:08:09 AM UTC+12, Andrew McCreight wrote:
> On Thu, May 25, 2017 at 4:03 PM,  wrote:
> 
> > On Friday, May 26, 2017 at 6:03:02 AM UTC+12, Chris Peterson wrote:
> > > On 2017-05-25 5:31 AM, Ehsan Akhgari wrote:
> > > > On 05/19/2017 02:44 PM, Gregory Szorc wrote:
> > > >> `mach build` attempts to parse compiler warnings to a persisted
> > > >> "database."
> > > >> You can view a list of compiler warnings post build by running `mach
> > > >> warnings-list`. The intent behind this feature was to make it easier
> > to
> > > >> find and fix compiler warnings. After all, something out of sight is
> > > >> out of
> > > >> mind.
> > >
> > > Given that we treat warnings as errors on CI and most directories that
> > > opt out of warnings-as-errors are third-party code
> > > (ALLOW_COMPILER_WARNINGS in moz.build [1]), I don't think we need to
> > > make the warning list any more visible. A warning regression in nearly
> > > all first-party code is already treated as an error.
> > >
> > >
> > > [1]
> > > https://searchfox.org/mozilla-central/search?q=ALLOW_
> > COMPILER_WARNINGS=true=moz.build
> >
> > Tangentially: "we treat warnings as errors on CI" -- It'd be great if
> > local builds could have that behavior by default, so we (or just I?) don't
> > get caught by surprise when warnings-as-errors appear after landing.
> >
> > Or at least: Can I opt-in locally? (I tried 'ac_add_options
> > --enable-warnings-ar-errors' in mozconfig, but that was not recognized.)
> >
> 
> This has worked for me before:
> ac_add_options --enable-warnings-as-errors

Haha, I've just noticed that I had typed '--enable-warnings-ar-errors' (Notice 
the 'ar' instead of 'as' -- must have been pirate day!)

Thanks Andrew.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Eric Rahm]

I think we disable it for local builds because we don't control what
versions of tools folks use. So clang vFOO might spit out errors we don't
see in clang vBAR and it would be a huge pain if those failed locally even
though they'd be fine in automation.

-e

On Thu, May 25, 2017 at 4:07 PM, Andrew McCreight 
wrote:

> On Thu, May 25, 2017 at 4:03 PM,  wrote:
>
> > On Friday, May 26, 2017 at 6:03:02 AM UTC+12, Chris Peterson wrote:
> > > On 2017-05-25 5:31 AM, Ehsan Akhgari wrote:
> > > > On 05/19/2017 02:44 PM, Gregory Szorc wrote:
> > > >> `mach build` attempts to parse compiler warnings to a persisted
> > > >> "database."
> > > >> You can view a list of compiler warnings post build by running `mach
> > > >> warnings-list`. The intent behind this feature was to make it easier
> > to
> > > >> find and fix compiler warnings. After all, something out of sight is
> > > >> out of
> > > >> mind.
> > >
> > > Given that we treat warnings as errors on CI and most directories that
> > > opt out of warnings-as-errors are third-party code
> > > (ALLOW_COMPILER_WARNINGS in moz.build [1]), I don't think we need to
> > > make the warning list any more visible. A warning regression in nearly
> > > all first-party code is already treated as an error.
> > >
> > >
> > > [1]
> > > https://searchfox.org/mozilla-central/search?q=ALLOW_
> > COMPILER_WARNINGS=true=moz.build
> >
> > Tangentially: "we treat warnings as errors on CI" -- It'd be great if
> > local builds could have that behavior by default, so we (or just I?)
> don't
> > get caught by surprise when warnings-as-errors appear after landing.
> >
> > Or at least: Can I opt-in locally? (I tried 'ac_add_options
> > --enable-warnings-ar-errors' in mozconfig, but that was not recognized.)
> >
>
> This has worked for me before:
> ac_add_options --enable-warnings-as-errors
>
>
> ___
> > dev-platform mailing list
> > dev-platform@lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
> >
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Andrew McCreight]

On Thu, May 25, 2017 at 4:03 PM,  wrote:

> On Friday, May 26, 2017 at 6:03:02 AM UTC+12, Chris Peterson wrote:
> > On 2017-05-25 5:31 AM, Ehsan Akhgari wrote:
> > > On 05/19/2017 02:44 PM, Gregory Szorc wrote:
> > >> `mach build` attempts to parse compiler warnings to a persisted
> > >> "database."
> > >> You can view a list of compiler warnings post build by running `mach
> > >> warnings-list`. The intent behind this feature was to make it easier
> to
> > >> find and fix compiler warnings. After all, something out of sight is
> > >> out of
> > >> mind.
> >
> > Given that we treat warnings as errors on CI and most directories that
> > opt out of warnings-as-errors are third-party code
> > (ALLOW_COMPILER_WARNINGS in moz.build [1]), I don't think we need to
> > make the warning list any more visible. A warning regression in nearly
> > all first-party code is already treated as an error.
> >
> >
> > [1]
> > https://searchfox.org/mozilla-central/search?q=ALLOW_
> COMPILER_WARNINGS=true=moz.build
>
> Tangentially: "we treat warnings as errors on CI" -- It'd be great if
> local builds could have that behavior by default, so we (or just I?) don't
> get caught by surprise when warnings-as-errors appear after landing.
>
> Or at least: Can I opt-in locally? (I tried 'ac_add_options
> --enable-warnings-ar-errors' in mozconfig, but that was not recognized.)
>

This has worked for me before:
ac_add_options --enable-warnings-as-errors


___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[gsquelart]

On Friday, May 26, 2017 at 6:03:02 AM UTC+12, Chris Peterson wrote:
> On 2017-05-25 5:31 AM, Ehsan Akhgari wrote:
> > On 05/19/2017 02:44 PM, Gregory Szorc wrote:
> >> `mach build` attempts to parse compiler warnings to a persisted
> >> "database."
> >> You can view a list of compiler warnings post build by running `mach
> >> warnings-list`. The intent behind this feature was to make it easier to
> >> find and fix compiler warnings. After all, something out of sight is
> >> out of
> >> mind.
> 
> Given that we treat warnings as errors on CI and most directories that 
> opt out of warnings-as-errors are third-party code 
> (ALLOW_COMPILER_WARNINGS in moz.build [1]), I don't think we need to 
> make the warning list any more visible. A warning regression in nearly 
> all first-party code is already treated as an error.
> 
> 
> [1] 
> https://searchfox.org/mozilla-central/search?q=ALLOW_COMPILER_WARNINGS=true=moz.build

Tangentially: "we treat warnings as errors on CI" -- It'd be great if local 
builds could have that behavior by default, so we (or just I?) don't get caught 
by surprise when warnings-as-errors appear after landing.

Or at least: Can I opt-in locally? (I tried 'ac_add_options 
--enable-warnings-ar-errors' in mozconfig, but that was not recognized.)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Kris Maglione]

On Thu, May 25, 2017 at 08:31:24AM -0400, Ehsan Akhgari wrote:
What was the motivation behind this change?  Was there a complaint 
from a significant number of developers about it being difficult 
fixing compiler warnings grepping for things like "warning:" or using 
./mach warnings-list? Was the feedback from developers who have the 
use case of writing code and fix compiler errors in such code taken 
into account?


For what it's worth, I've often wished for something like this 
when watching huge numbers of warnings flow by in NSS and media 
code, if only to motivate people to fix them. But the fact that 
we compile with -Werror and generally only get warnings in 
third-party code (ignoring NSS and NSPR) means that they're 
effectively pretty useless, and we'd be better off just 
disabling warnings in that code.


What I really want at the end of a compile is a list of warnings 
and errors that matter, rather than a lot of useless warning 
spew that makes it harder to find the ones that do among the 
ones that don't.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Chris Peterson]

On 2017-05-25 5:31 AM, Ehsan Akhgari wrote:

On 05/19/2017 02:44 PM, Gregory Szorc wrote:

`mach build` attempts to parse compiler warnings to a persisted
"database."
You can view a list of compiler warnings post build by running `mach
warnings-list`. The intent behind this feature was to make it easier to
find and fix compiler warnings. After all, something out of sight is
out of
mind.


Given that we treat warnings as errors on CI and most directories that 
opt out of warnings-as-errors are third-party code 
(ALLOW_COMPILER_WARNINGS in moz.build [1]), I don't think we need to 
make the warning list any more visible. A warning regression in nearly 
all first-party code is already treated as an error.



[1] 
https://searchfox.org/mozilla-central/search?q=ALLOW_COMPILER_WARNINGS=true=moz.build

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Eric Rescorla]

I'd like to second Ehsan's point, but also expand upon it into a more
general observation.

As it becomes progressively more difficult to build Firefox without mach,
it becomes
increasingly important that mach respect people's workflows. For those of
us who
were comfortable with make and the behavior of having relatively unfiltered
access
to what the build system is doing, it would be great if mach could have
some set of
flags to preserve that view (cf. the flags to remove the timestamps so that
emacs
compiler mode works).

-Ekr
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Boris Zbarsky]

On 5/25/17 8:31 AM, Ehsan Akhgari wrote:

This currently only
serves to make it more difficult to find compiler errors when they
occur.


Fwiw (and not to distract from your main point), 
https://bugzilla.mozilla.org/show_bug.cgi?id=1367405 just got fixed so 
we should no longer get the warning spew when there are compiler errors.


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Ehsan Akhgari]

On 05/19/2017 02:44 PM, Gregory Szorc wrote:

`mach build` attempts to parse compiler warnings to a persisted "database."
You can view a list of compiler warnings post build by running `mach
warnings-list`. The intent behind this feature was to make it easier to
find and fix compiler warnings. After all, something out of sight is out of
mind.

There have been a few recent changes to increase the visibility of compiler
warnings with the expectation being that raising visibility will increase
the chances of someone addressing them. After all, a compiler warning is
either a) valid and should be fixed or b) invalid and should be ignored.
Either way, a compiler warning shouldn't exist.


Since mystor and billm's objection seems to have gone unaddressed in 
this thread, let me try once again.  There is a (c) scenario that you 
are ignoring here:


c) the warning is valid and should be fixed but it's not more important 
than other things than the developer may want to do at the moment.  I 
would like to posit that this is often the case.  Let's look at what I 
get these days at the end of a normal debug build on Linux.  This output 
is long and I am intentionally pasting the full thing here in order to 
make a point on how much useless information we are presenting at the 
end of each build:


18:39.34 warning: db/sqlite3/src/sqlite3.c:131236:39 
[-Wunreachable-code] code will never be executed
18:39.34 warning: db/sqlite3/src/sqlite3.c:131292:39 
[-Wunreachable-code] code will never be executed
18:39.34 warning: db/sqlite3/src/sqlite3.c:137670:9 [-Wunreachable-code] 
code will never be executed
18:39.34 warning: 
gfx/angle/src/compiler/translator/ASTMetadataHLSL.cpp:93:15 
[-Wimplicit-fallthrough] unannotated fall-through between switch labels
18:39.34 warning: 
gfx/angle/src/compiler/translator/ParseContext.cpp:1123:9 
[-Wimplicit-fallthrough] unannotated fall-through between switch labels
18:39.34 warning: 
gfx/angle/src/compiler/translator/ParseContext.cpp:3640:9 
[-Wimplicit-fallthrough] unannotated fall-through between switch labels
18:39.34 warning: 
gfx/angle/src/compiler/translator/ParseContext.cpp:3808:9 
[-Wimplicit-fallthrough] unannotated fall-through between switch labels
18:39.34 warning: gfx/cairo/libpixman/src/pixman-bits-image.c:268:32 
[-Wshift-negative-value] shifting a negative signed value is undefined
18:39.34 warning: gfx/cairo/libpixman/src/pixman-linear-gradient.c:395:6 
[-Wunreachable-code] code will never be executed
18:39.34 warning: gfx/cairo/libpixman/src/pixman-x86.c:80:5 
[-Wexpansion-to-defined] macro expansion producing 'defined' has 
undefined behavior
18:39.34 warning: gfx/cairo/libpixman/src/pixman-x86.c:119:5 
[-Wexpansion-to-defined] macro expansion producing 'defined' has 
undefined behavior
18:39.34 warning: intl/hyphenation/hyphen/hyphen.c:332:27 
[-Wsign-compare] comparison of integers of different signs: 'int' and 
'unsigned long'
18:39.34 warning: intl/icu/source/common/locdspnm.cpp:286:14 
[-Wunused-private-field] private field 'capitalizationBrkIter' is not used
18:39.34 warning: intl/icu/source/common/udataswp.c:438:29 
[-Wsign-compare] comparison of integers of different signs: 'int32_t' 
(aka 'int') and 'unsigned long'
18:39.34 warning: intl/icu/source/common/ulist.c:161:24 [-Wsign-compare] 
comparison of integers of different signs: 'int32_t' (aka 'int') and 
'unsigned long'
18:39.34 warning: intl/icu/source/common/uloc_tag.c:1374:31 
[-Wsign-compare] comparison of integers of different signs: 'int32_t' 
(aka 'int') and 'unsigned long'
18:39.34 warning: intl/icu/source/common/uloc_tag.c:1409:36 
[-Wsign-compare] comparison of integers of different signs: 'int32_t' 
(aka 'int') and 'unsigned long'
18:39.34 warning: intl/icu/source/common/ures_cnv.c:46:18 
[-Wsign-compare] comparison of integers of different signs: 'int32_t' 
(aka 'int') and 'unsigned long'
18:39.34 warning: intl/icu/source/common/ures_cnv.c:64:22 
[-Wsign-compare] comparison of integers of different signs: 'int32_t' 
(aka 'int') and 'unsigned long'
18:39.34 warning: intl/icu/source/common/ushape.cpp:1247:12 [-Wcomma] 
possible misuse of comma operator here
18:39.34 warning: intl/icu/source/common/ustring.cpp:860:57 [-Wcomma] 
possible misuse of comma operator here
18:39.34 warning: intl/icu/source/common/ustring.cpp:870:57 [-Wcomma] 
possible misuse of comma operator here
18:39.34 warning: intl/icu/source/common/ustrtrns.cpp:488:47 [-Wcomma] 
possible misuse of comma operator here
18:39.34 warning: intl/icu/source/common/ustrtrns.cpp:535:47 [-Wcomma] 
possible misuse of comma operator here
18:39.34 warning: intl/icu/source/common/ustrtrns.cpp:609:51 [-Wcomma] 
possible misuse of comma operator here
18:39.34 warning: intl/icu/source/common/ustrtrns.cpp:655:47 [-Wcomma] 
possible misuse of comma operator here
18:39.34 warning: intl/icu/source/common/ustrtrns.cpp:704:47 [-Wcomma] 
possible misuse of comma operator here
18:39.34 warning: intl/icu/source/common/utrace.c:149:16 
[-Wsign-compare] 

Re: Improving visibility of compiler warnings

[Martin Thomson]

On Thu, May 25, 2017 at 6:03 AM, Nathan Froyd  wrote:
> Where does NSS do this?  Cloning the NSS tree and grepping for
> sign-compare turns up no disabling of -Wsign-compare, except perhaps
> in XCode for gtest itself.

My bad, -Wsign-compare is in -Wextra, and we don't enable anything
extra.  We disable errors for C4018 on MSVC, which is what I was
thinking of.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Michael Layzell]

I had it pointed out to me on IRC by jcristau that `chronic` already exists
as a Unix utility from moreutils:
https://manpages.debian.org/jessie/moreutils/chronic.1.en.html

It does pretty much the exact same thing as nowarn, but is probably better
written ^.^.


On Tue, May 23, 2017 at 11:56 AM, Michael Layzell 
wrote:

> I don't have enough time to work on a proper solution, but I wrote a
> simple rust wrapper which just consumes stderr from the wrapped process,
> and doesn't write it out unless the internal command exited with a non-zero
> exit code. I figured I'd post it in case anyone else finds it useful.
> https://github.com/mystor/nowarn
>
> Very hacky, but it helps clean up the spam of compiler warnings to let me
> actually find my build errors.
>
> On Mon, May 22, 2017 at 7:16 PM, Karl Tomlinson  wrote:
>
>> On Sat, 20 May 2017 14:59:11 -0400, Eric Rescorla wrote:
>>
>> > On Sat, May 20, 2017 at 1:16 PM, Kris Maglione 
>> > wrote:
>> >
>> >> On Sat, May 20, 2017 at 08:36:13PM +1000, Martin Thomson wrote:
>> >>
>> >>> Hmm, these are all -Wsign-compare issues bar one, which is fixed
>> >>> upstream.  We have an open bug tracking the warnings
>> >>> (https://bugzilla.mozilla.org/show_bug.cgi?id=1307958 and
>> specifically
>> >>> https://bugzilla.mozilla.org/show_bug.cgi?id=1212199 for NSS).
>> >>>
>> >>> NSS is built with -Werror separately, but disables errors on
>> >>> -Wsign-compare.  Disabling those warnings for a Firefox build of NSS
>> >>> wouldn't be so bad now that we share gyp config.  Based on a recent
>> >>> build, that's 139 messages (add 36 if you want to add nspr).
>> >>>
>> >>
>> >> Is there a particularly good reason that NSS needs to disable
>> >> -Wsign-compare? That seems like a footgun waiting to happen,
>> especially in
>> >> crypto code.
>> >
>> >
>> > Like many other pieces of old code, there are a lot of things that
>> trigger
>> > compiler warnings which we have been progressively removing, but we
>> > haven't removed these yet. Ideally we would have some tooling that
>> > would distinguish new warnings from old warnings, but absent that,
>> > until we fix all the existing warnings it's either disable -Werror or
>> > disable this particular warning. It's not something we're particularly
>> > happy about.
>>
>> -Wno-error=sign-compare is also an option,
>> but a wall of sign-compare warnings may not be helpful in general.
>>
>> ___
>> dev-platform mailing list
>> dev-platform@lists.mozilla.org
>> https://lists.mozilla.org/listinfo/dev-platform
>>
>
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Michael Layzell]

I don't have enough time to work on a proper solution, but I wrote a simple
rust wrapper which just consumes stderr from the wrapped process, and
doesn't write it out unless the internal command exited with a non-zero
exit code. I figured I'd post it in case anyone else finds it useful.
https://github.com/mystor/nowarn

Very hacky, but it helps clean up the spam of compiler warnings to let me
actually find my build errors.

On Mon, May 22, 2017 at 7:16 PM, Karl Tomlinson  wrote:

> On Sat, 20 May 2017 14:59:11 -0400, Eric Rescorla wrote:
>
> > On Sat, May 20, 2017 at 1:16 PM, Kris Maglione 
> > wrote:
> >
> >> On Sat, May 20, 2017 at 08:36:13PM +1000, Martin Thomson wrote:
> >>
> >>> Hmm, these are all -Wsign-compare issues bar one, which is fixed
> >>> upstream.  We have an open bug tracking the warnings
> >>> (https://bugzilla.mozilla.org/show_bug.cgi?id=1307958 and specifically
> >>> https://bugzilla.mozilla.org/show_bug.cgi?id=1212199 for NSS).
> >>>
> >>> NSS is built with -Werror separately, but disables errors on
> >>> -Wsign-compare.  Disabling those warnings for a Firefox build of NSS
> >>> wouldn't be so bad now that we share gyp config.  Based on a recent
> >>> build, that's 139 messages (add 36 if you want to add nspr).
> >>>
> >>
> >> Is there a particularly good reason that NSS needs to disable
> >> -Wsign-compare? That seems like a footgun waiting to happen, especially
> in
> >> crypto code.
> >
> >
> > Like many other pieces of old code, there are a lot of things that
> trigger
> > compiler warnings which we have been progressively removing, but we
> > haven't removed these yet. Ideally we would have some tooling that
> > would distinguish new warnings from old warnings, but absent that,
> > until we fix all the existing warnings it's either disable -Werror or
> > disable this particular warning. It's not something we're particularly
> > happy about.
>
> -Wno-error=sign-compare is also an option,
> but a wall of sign-compare warnings may not be helpful in general.
>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Karl Tomlinson]

On Sat, 20 May 2017 14:59:11 -0400, Eric Rescorla wrote:

> On Sat, May 20, 2017 at 1:16 PM, Kris Maglione 
> wrote:
>
>> On Sat, May 20, 2017 at 08:36:13PM +1000, Martin Thomson wrote:
>>
>>> Hmm, these are all -Wsign-compare issues bar one, which is fixed
>>> upstream.  We have an open bug tracking the warnings
>>> (https://bugzilla.mozilla.org/show_bug.cgi?id=1307958 and specifically
>>> https://bugzilla.mozilla.org/show_bug.cgi?id=1212199 for NSS).
>>>
>>> NSS is built with -Werror separately, but disables errors on
>>> -Wsign-compare.  Disabling those warnings for a Firefox build of NSS
>>> wouldn't be so bad now that we share gyp config.  Based on a recent
>>> build, that's 139 messages (add 36 if you want to add nspr).
>>>
>>
>> Is there a particularly good reason that NSS needs to disable
>> -Wsign-compare? That seems like a footgun waiting to happen, especially in
>> crypto code.
>
>
> Like many other pieces of old code, there are a lot of things that trigger
> compiler warnings which we have been progressively removing, but we
> haven't removed these yet. Ideally we would have some tooling that
> would distinguish new warnings from old warnings, but absent that,
> until we fix all the existing warnings it's either disable -Werror or
> disable this particular warning. It's not something we're particularly
> happy about.

-Wno-error=sign-compare is also an option,
but a wall of sign-compare warnings may not be helpful in general. 

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[ISHIKAWA,chiaki]

Hi,

On 2017/05/20 19:36, Martin Thomson wrote:

On Sat, May 20, 2017 at 4:55 AM, Kris Maglione  wrote:

Can we make some effort to get clean warnings output at the end of standard
builds? A huge chunk of the warnings come from NSS and NSPR, and should be
easily fixable.


Hmm, these are all -Wsign-compare issues bar one, which is fixed
upstream.  We have an open bug tracking the warnings
(https://bugzilla.mozilla.org/show_bug.cgi?id=1307958 and specifically
https://bugzilla.mozilla.org/show_bug.cgi?id=1212199 for NSS).


I am surprised that
https://bugzilla.mozilla.org/show_bug.cgi?id=1307958
was mentioned and when I looked at it, it was filed by me. :-)

The patches there were filed about four months ago, and recently I 
noticed that I need to add a few more changes presumably due to code 
changes, AND I found that I must have disabled a few modules from 
compiling to build C-C TB, and a full M-C FF build with -Wsign-compare 
and warning as error setting needs still a few more changes (not that many).


I will upload the latest patches later.

I agree that disabling this check is a little uncomfortable for crypto 
code.

My patch is pasting over the mismatched comparison by
casting one type to the other. In most of the cases, I see that the 
value should not go that high for wraparound near 2^31, etc. but still
I would have inserted an assert() to detect a problematic situation if I 
have infinite amount of time to spend reading the code.
My patches would alert astute coders to look at the places where the 
problem may arise and insert assert() appropriately.


TIA






NSS is built with -Werror separately, but disables errors on
-Wsign-compare.  Disabling those warnings for a Firefox build of NSS
wouldn't be so bad now that we share gyp config.  Based on a recent
build, that's 139 messages (add 36 if you want to add nspr).

I've spent a little time looking into the real issues.  Fixing
requires some care, since it touches ABI compat in many places.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform




___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Eric Rescorla]

On Sat, May 20, 2017 at 1:16 PM, Kris Maglione 
wrote:

> On Sat, May 20, 2017 at 08:36:13PM +1000, Martin Thomson wrote:
>
>> On Sat, May 20, 2017 at 4:55 AM, Kris Maglione 
>> wrote:
>>
>>> Can we make some effort to get clean warnings output at the end of
>>> standard
>>> builds? A huge chunk of the warnings come from NSS and NSPR, and should
>>> be
>>> easily fixable.
>>>
>>
>> Hmm, these are all -Wsign-compare issues bar one, which is fixed
>> upstream.  We have an open bug tracking the warnings
>> (https://bugzilla.mozilla.org/show_bug.cgi?id=1307958 and specifically
>> https://bugzilla.mozilla.org/show_bug.cgi?id=1212199 for NSS).
>>
>> NSS is built with -Werror separately, but disables errors on
>> -Wsign-compare.  Disabling those warnings for a Firefox build of NSS
>> wouldn't be so bad now that we share gyp config.  Based on a recent
>> build, that's 139 messages (add 36 if you want to add nspr).
>>
>
> Is there a particularly good reason that NSS needs to disable
> -Wsign-compare? That seems like a footgun waiting to happen, especially in
> crypto code.


Like many other pieces of old code, there are a lot of things that trigger
compiler warnings which we have been progressively removing, but we
haven't removed these yet. Ideally we would have some tooling that
would distinguish new warnings from old warnings, but absent that,
until we fix all the existing warnings it's either disable -Werror or
disable this particular warning. It's not something we're particularly
happy about.

-Ekr

>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Kris Maglione]

On Sat, May 20, 2017 at 08:36:13PM +1000, Martin Thomson wrote:

On Sat, May 20, 2017 at 4:55 AM, Kris Maglione  wrote:

Can we make some effort to get clean warnings output at the end of standard
builds? A huge chunk of the warnings come from NSS and NSPR, and should be
easily fixable.


Hmm, these are all -Wsign-compare issues bar one, which is fixed
upstream.  We have an open bug tracking the warnings
(https://bugzilla.mozilla.org/show_bug.cgi?id=1307958 and specifically
https://bugzilla.mozilla.org/show_bug.cgi?id=1212199 for NSS).

NSS is built with -Werror separately, but disables errors on
-Wsign-compare.  Disabling those warnings for a Firefox build of NSS
wouldn't be so bad now that we share gyp config.  Based on a recent
build, that's 139 messages (add 36 if you want to add nspr).


Is there a particularly good reason that NSS needs to disable 
-Wsign-compare? That seems like a footgun waiting to happen, 
especially in crypto code.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Martin Thomson]

On Sat, May 20, 2017 at 4:55 AM, Kris Maglione  wrote:
> Can we make some effort to get clean warnings output at the end of standard
> builds? A huge chunk of the warnings come from NSS and NSPR, and should be
> easily fixable.

Hmm, these are all -Wsign-compare issues bar one, which is fixed
upstream.  We have an open bug tracking the warnings
(https://bugzilla.mozilla.org/show_bug.cgi?id=1307958 and specifically
https://bugzilla.mozilla.org/show_bug.cgi?id=1212199 for NSS).

NSS is built with -Werror separately, but disables errors on
-Wsign-compare.  Disabling those warnings for a Firefox build of NSS
wouldn't be so bad now that we share gyp config.  Based on a recent
build, that's 139 messages (add 36 if you want to add nspr).

I've spent a little time looking into the real issues.  Fixing
requires some care, since it touches ABI compat in many places.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Gregory Szorc]

On Fri, May 19, 2017 at 1:27 PM, Michael Layzell 
wrote:

> With regard to ALLOW_COMPILER_WARNINGS, I'm strongly of the opinion that we
> should be providing an option to hide all warnings from modules marked as
> ALLOW_COMPILER_WARNINGS now, as they are only in "external" libraries which
> most of us are not working on, and they really clog up my compiler output
> and make me have to scroll up many many pages in order to see the build
> errors which are actually my fault. (see this bug:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1301688). Unfortunately, I
> don't know enough about the build system to write a patch to do this.
>

I like to teach others how to fish:

https://dxr.mozilla.org/mozilla-central/search?q=path%3Amoz.build+C=false

Code to call into external build systems is typically in config/external/.
See e.g. config/external/icu/defs.mozbuild for where compiler flags for ICU
are defined.



>
> On Fri, May 19, 2017 at 2:59 PM, David Major  wrote:
>
> > I'm not sure if this is exactly the same as the ALLOW_COMPILER_WARNINGS
> > that we use for warnings-on-errors, but FWIW as of a couple of months
> > ago I cleaned out the last warning-allowance in our "own" code.
> > ALLOW_COMPILER_WARNINGS usage is now only in external libraries (I'm
> > counting NSS and NSPR as "external" for this purpose since I can't land
> > code to m-c directly).
> >
> > On Fri, May 19, 2017, at 02:55 PM, Kris Maglione wrote:
> > > I've actually been meaning to post about this, with some questions.
> > >
> > > I definitely like that we now print warnings at the end of builds,
> since
> > > it
> > > makes them harder to ignore. But the current amount of warnings spew at
> > > the
> > > end of every build is problematic, especially when a build fails and I
> > > need
> > > to scroll up several pages to find out why.
> > >
> > > Can we make some effort to get clean warnings output at the end of
> > > standard
> > > builds? A huge chunk of the warnings come from NSS and NSPR, and should
> > > be
> > > easily fixable. Most of the rest seem to come from vendored libraries,
> > > and
> > > should probably just be whitelisted if we can't get fixes upstreamed.
> > >
> > > On Fri, May 19, 2017 at 11:44:08AM -0700, Gregory Szorc wrote:
> > > >`mach build` attempts to parse compiler warnings to a persisted
> > "database."
> > > >You can view a list of compiler warnings post build by running `mach
> > > >warnings-list`. The intent behind this feature was to make it easier
> to
> > > >find and fix compiler warnings. After all, something out of sight is
> > out of
> > > >mind.
> > > >
> > > >There have been a few recent changes to increase the visibility of
> > compiler
> > > >warnings with the expectation being that raising visibility will
> > increase
> > > >the chances of someone addressing them. After all, a compiler warning
> is
> > > >either a) valid and should be fixed or b) invalid and should be
> ignored.
> > > >Either way, a compiler warning shouldn't exist.
> > > >
> > > >First, `mach build` now prints a list of all parsed compiler warnings
> at
> > > >the end of the build. More details are in the commit message:
> > > >https://hg.mozilla.org/mozilla-central/rev/4abe611696ab
> > > >
> > > >Second, Perfherder now records the compiler warning count as a metric.
> > This
> > > >means we now have alerts when compiler warnings are added or removed,
> > like
> > > >[1]. And, we can easily see graphs of compiler warnings over time,
> like
> > > >[2]. The compiler warnings are also surfaced in the "performance" tab
> of
> > > >build jobs on Treeherder, like [3].
> > > >
> > > >The Perfherder alerts and tracking are informational only: nobody will
> > be
> > > >backing you out merely because you added a compiler warning. While the
> > > >possibility of doing that now exists, I view that decision as up to
> the
> > C++
> > > >module. I'm not going to advocate for it. So if you feel passionately,
> > take
> > > >it up with them :)
> > > >
> > > >Astute link clickers will note that the count metrics in CI are often
> > noisy
> > > >- commonly fluctuating by a value of 1-2. I suspect there are race
> > > >conditions with interleaved process output and/or bugs in the compiler
> > > >warnings parser/aggregator. Since the values are currently advisory
> > only,
> > > >I'm very much taking a "perfect is the enemy of good" attitude and
> have
> > no
> > > >plans to improve the robustness.
> > > >
> > > >[1] https://treeherder.mozilla.org/perf.html#/alerts?id=6700
> > > >[2] https://mzl.la/2qFN0me and https://mzl.la/2rAkWR5
> > > >[3]
> > > >https://treeherder.mozilla.org/#/jobs?repo=autoland;
> > selectedJob=100509284
> > > >___
> > > >dev-platform mailing list
> > > >dev-platform@lists.mozilla.org
> > > >https://lists.mozilla.org/listinfo/dev-platform
> > >
> > > --
> > > Kris Maglione
> > > Senior Firefox Add-ons Engineer
> > > Mozilla Corporation
> 

Re: Improving visibility of compiler warnings

[Bill McCloskey]

I strongly agree with you, Michael! Especially now that I'm using IceCC, I
pretty much always have to use search-and-replace to find compiler errors.
It's a pointless drain on productivity.

On Fri, May 19, 2017 at 1:27 PM, Michael Layzell 
wrote:

> With regard to ALLOW_COMPILER_WARNINGS, I'm strongly of the opinion that we
> should be providing an option to hide all warnings from modules marked as
> ALLOW_COMPILER_WARNINGS now, as they are only in "external" libraries which
> most of us are not working on, and they really clog up my compiler output
> and make me have to scroll up many many pages in order to see the build
> errors which are actually my fault. (see this bug:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1301688). Unfortunately, I
> don't know enough about the build system to write a patch to do this.
>
> On Fri, May 19, 2017 at 2:59 PM, David Major  wrote:
>
> > I'm not sure if this is exactly the same as the ALLOW_COMPILER_WARNINGS
> > that we use for warnings-on-errors, but FWIW as of a couple of months
> > ago I cleaned out the last warning-allowance in our "own" code.
> > ALLOW_COMPILER_WARNINGS usage is now only in external libraries (I'm
> > counting NSS and NSPR as "external" for this purpose since I can't land
> > code to m-c directly).
> >
> > On Fri, May 19, 2017, at 02:55 PM, Kris Maglione wrote:
> > > I've actually been meaning to post about this, with some questions.
> > >
> > > I definitely like that we now print warnings at the end of builds,
> since
> > > it
> > > makes them harder to ignore. But the current amount of warnings spew at
> > > the
> > > end of every build is problematic, especially when a build fails and I
> > > need
> > > to scroll up several pages to find out why.
> > >
> > > Can we make some effort to get clean warnings output at the end of
> > > standard
> > > builds? A huge chunk of the warnings come from NSS and NSPR, and should
> > > be
> > > easily fixable. Most of the rest seem to come from vendored libraries,
> > > and
> > > should probably just be whitelisted if we can't get fixes upstreamed.
> > >
> > > On Fri, May 19, 2017 at 11:44:08AM -0700, Gregory Szorc wrote:
> > > >`mach build` attempts to parse compiler warnings to a persisted
> > "database."
> > > >You can view a list of compiler warnings post build by running `mach
> > > >warnings-list`. The intent behind this feature was to make it easier
> to
> > > >find and fix compiler warnings. After all, something out of sight is
> > out of
> > > >mind.
> > > >
> > > >There have been a few recent changes to increase the visibility of
> > compiler
> > > >warnings with the expectation being that raising visibility will
> > increase
> > > >the chances of someone addressing them. After all, a compiler warning
> is
> > > >either a) valid and should be fixed or b) invalid and should be
> ignored.
> > > >Either way, a compiler warning shouldn't exist.
> > > >
> > > >First, `mach build` now prints a list of all parsed compiler warnings
> at
> > > >the end of the build. More details are in the commit message:
> > > >https://hg.mozilla.org/mozilla-central/rev/4abe611696ab
> > > >
> > > >Second, Perfherder now records the compiler warning count as a metric.
> > This
> > > >means we now have alerts when compiler warnings are added or removed,
> > like
> > > >[1]. And, we can easily see graphs of compiler warnings over time,
> like
> > > >[2]. The compiler warnings are also surfaced in the "performance" tab
> of
> > > >build jobs on Treeherder, like [3].
> > > >
> > > >The Perfherder alerts and tracking are informational only: nobody will
> > be
> > > >backing you out merely because you added a compiler warning. While the
> > > >possibility of doing that now exists, I view that decision as up to
> the
> > C++
> > > >module. I'm not going to advocate for it. So if you feel passionately,
> > take
> > > >it up with them :)
> > > >
> > > >Astute link clickers will note that the count metrics in CI are often
> > noisy
> > > >- commonly fluctuating by a value of 1-2. I suspect there are race
> > > >conditions with interleaved process output and/or bugs in the compiler
> > > >warnings parser/aggregator. Since the values are currently advisory
> > only,
> > > >I'm very much taking a "perfect is the enemy of good" attitude and
> have
> > no
> > > >plans to improve the robustness.
> > > >
> > > >[1] https://treeherder.mozilla.org/perf.html#/alerts?id=6700
> > > >[2] https://mzl.la/2qFN0me and https://mzl.la/2rAkWR5
> > > >[3]
> > > >https://treeherder.mozilla.org/#/jobs?repo=autoland;
> > selectedJob=100509284
> > > >___
> > > >dev-platform mailing list
> > > >dev-platform@lists.mozilla.org
> > > >https://lists.mozilla.org/listinfo/dev-platform
> > >
> > > --
> > > Kris Maglione
> > > Senior Firefox Add-ons Engineer
> > > Mozilla Corporation
> > >
> > > The X server has to be the biggest program I've ever seen that doesn't
> > > do 

Re: Improving visibility of compiler warnings

[Kris Maglione]

While I agree with that in general, it seems like the warnings in NSS and 
NSPR, at least, are our responsibility, and should be fixed. I notice the 
huge number of warnings scrolling by from NSS, in particular, every time I 
compile, and they make me worry.


On Fri, May 19, 2017 at 04:27:47PM -0400, Michael Layzell wrote:

With regard to ALLOW_COMPILER_WARNINGS, I'm strongly of the opinion that we
should be providing an option to hide all warnings from modules marked as
ALLOW_COMPILER_WARNINGS now, as they are only in "external" libraries which
most of us are not working on, and they really clog up my compiler output
and make me have to scroll up many many pages in order to see the build
errors which are actually my fault. (see this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1301688). Unfortunately, I
don't know enough about the build system to write a patch to do this.

On Fri, May 19, 2017 at 2:59 PM, David Major  wrote:


I'm not sure if this is exactly the same as the ALLOW_COMPILER_WARNINGS
that we use for warnings-on-errors, but FWIW as of a couple of months
ago I cleaned out the last warning-allowance in our "own" code.
ALLOW_COMPILER_WARNINGS usage is now only in external libraries (I'm
counting NSS and NSPR as "external" for this purpose since I can't land
code to m-c directly).

On Fri, May 19, 2017, at 02:55 PM, Kris Maglione wrote:
> I've actually been meaning to post about this, with some questions.
>
> I definitely like that we now print warnings at the end of builds, since
> it
> makes them harder to ignore. But the current amount of warnings spew at
> the
> end of every build is problematic, especially when a build fails and I
> need
> to scroll up several pages to find out why.
>
> Can we make some effort to get clean warnings output at the end of
> standard
> builds? A huge chunk of the warnings come from NSS and NSPR, and should
> be
> easily fixable. Most of the rest seem to come from vendored libraries,
> and
> should probably just be whitelisted if we can't get fixes upstreamed.
>
> On Fri, May 19, 2017 at 11:44:08AM -0700, Gregory Szorc wrote:
> >`mach build` attempts to parse compiler warnings to a persisted
"database."
> >You can view a list of compiler warnings post build by running `mach
> >warnings-list`. The intent behind this feature was to make it easier to
> >find and fix compiler warnings. After all, something out of sight is
out of
> >mind.
> >
> >There have been a few recent changes to increase the visibility of
compiler
> >warnings with the expectation being that raising visibility will
increase
> >the chances of someone addressing them. After all, a compiler warning is
> >either a) valid and should be fixed or b) invalid and should be ignored.
> >Either way, a compiler warning shouldn't exist.
> >
> >First, `mach build` now prints a list of all parsed compiler warnings at
> >the end of the build. More details are in the commit message:
> >https://hg.mozilla.org/mozilla-central/rev/4abe611696ab
> >
> >Second, Perfherder now records the compiler warning count as a metric.
This
> >means we now have alerts when compiler warnings are added or removed,
like
> >[1]. And, we can easily see graphs of compiler warnings over time, like
> >[2]. The compiler warnings are also surfaced in the "performance" tab of
> >build jobs on Treeherder, like [3].
> >
> >The Perfherder alerts and tracking are informational only: nobody will
be
> >backing you out merely because you added a compiler warning. While the
> >possibility of doing that now exists, I view that decision as up to the
C++
> >module. I'm not going to advocate for it. So if you feel passionately,
take
> >it up with them :)
> >
> >Astute link clickers will note that the count metrics in CI are often
noisy
> >- commonly fluctuating by a value of 1-2. I suspect there are race
> >conditions with interleaved process output and/or bugs in the compiler
> >warnings parser/aggregator. Since the values are currently advisory
only,
> >I'm very much taking a "perfect is the enemy of good" attitude and have
no
> >plans to improve the robustness.
> >
> >[1] https://treeherder.mozilla.org/perf.html#/alerts?id=6700
> >[2] https://mzl.la/2qFN0me and https://mzl.la/2rAkWR5
> >[3]
> >https://treeherder.mozilla.org/#/jobs?repo=autoland;
selectedJob=100509284
> >___
> >dev-platform mailing list
> >dev-platform@lists.mozilla.org
> >https://lists.mozilla.org/listinfo/dev-platform
>
> --
> Kris Maglione
> Senior Firefox Add-ons Engineer
> Mozilla Corporation
>
> The X server has to be the biggest program I've ever seen that doesn't
> do anything for you.
>   --Ken Thompson
>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org

Re: Improving visibility of compiler warnings

[Michael Layzell]

With regard to ALLOW_COMPILER_WARNINGS, I'm strongly of the opinion that we
should be providing an option to hide all warnings from modules marked as
ALLOW_COMPILER_WARNINGS now, as they are only in "external" libraries which
most of us are not working on, and they really clog up my compiler output
and make me have to scroll up many many pages in order to see the build
errors which are actually my fault. (see this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1301688). Unfortunately, I
don't know enough about the build system to write a patch to do this.

On Fri, May 19, 2017 at 2:59 PM, David Major  wrote:

> I'm not sure if this is exactly the same as the ALLOW_COMPILER_WARNINGS
> that we use for warnings-on-errors, but FWIW as of a couple of months
> ago I cleaned out the last warning-allowance in our "own" code.
> ALLOW_COMPILER_WARNINGS usage is now only in external libraries (I'm
> counting NSS and NSPR as "external" for this purpose since I can't land
> code to m-c directly).
>
> On Fri, May 19, 2017, at 02:55 PM, Kris Maglione wrote:
> > I've actually been meaning to post about this, with some questions.
> >
> > I definitely like that we now print warnings at the end of builds, since
> > it
> > makes them harder to ignore. But the current amount of warnings spew at
> > the
> > end of every build is problematic, especially when a build fails and I
> > need
> > to scroll up several pages to find out why.
> >
> > Can we make some effort to get clean warnings output at the end of
> > standard
> > builds? A huge chunk of the warnings come from NSS and NSPR, and should
> > be
> > easily fixable. Most of the rest seem to come from vendored libraries,
> > and
> > should probably just be whitelisted if we can't get fixes upstreamed.
> >
> > On Fri, May 19, 2017 at 11:44:08AM -0700, Gregory Szorc wrote:
> > >`mach build` attempts to parse compiler warnings to a persisted
> "database."
> > >You can view a list of compiler warnings post build by running `mach
> > >warnings-list`. The intent behind this feature was to make it easier to
> > >find and fix compiler warnings. After all, something out of sight is
> out of
> > >mind.
> > >
> > >There have been a few recent changes to increase the visibility of
> compiler
> > >warnings with the expectation being that raising visibility will
> increase
> > >the chances of someone addressing them. After all, a compiler warning is
> > >either a) valid and should be fixed or b) invalid and should be ignored.
> > >Either way, a compiler warning shouldn't exist.
> > >
> > >First, `mach build` now prints a list of all parsed compiler warnings at
> > >the end of the build. More details are in the commit message:
> > >https://hg.mozilla.org/mozilla-central/rev/4abe611696ab
> > >
> > >Second, Perfherder now records the compiler warning count as a metric.
> This
> > >means we now have alerts when compiler warnings are added or removed,
> like
> > >[1]. And, we can easily see graphs of compiler warnings over time, like
> > >[2]. The compiler warnings are also surfaced in the "performance" tab of
> > >build jobs on Treeherder, like [3].
> > >
> > >The Perfherder alerts and tracking are informational only: nobody will
> be
> > >backing you out merely because you added a compiler warning. While the
> > >possibility of doing that now exists, I view that decision as up to the
> C++
> > >module. I'm not going to advocate for it. So if you feel passionately,
> take
> > >it up with them :)
> > >
> > >Astute link clickers will note that the count metrics in CI are often
> noisy
> > >- commonly fluctuating by a value of 1-2. I suspect there are race
> > >conditions with interleaved process output and/or bugs in the compiler
> > >warnings parser/aggregator. Since the values are currently advisory
> only,
> > >I'm very much taking a "perfect is the enemy of good" attitude and have
> no
> > >plans to improve the robustness.
> > >
> > >[1] https://treeherder.mozilla.org/perf.html#/alerts?id=6700
> > >[2] https://mzl.la/2qFN0me and https://mzl.la/2rAkWR5
> > >[3]
> > >https://treeherder.mozilla.org/#/jobs?repo=autoland;
> selectedJob=100509284
> > >___
> > >dev-platform mailing list
> > >dev-platform@lists.mozilla.org
> > >https://lists.mozilla.org/listinfo/dev-platform
> >
> > --
> > Kris Maglione
> > Senior Firefox Add-ons Engineer
> > Mozilla Corporation
> >
> > The X server has to be the biggest program I've ever seen that doesn't
> > do anything for you.
> >   --Ken Thompson
> >
> > ___
> > dev-platform mailing list
> > dev-platform@lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org

Re: Improving visibility of compiler warnings

[jmaher]

It is great to see a good use for compiler warnings and alerts.  We have added 
a lot of data to perfherder and the build metrics are not covered by any 
sheriffs by default.  For these if it is clear who introduced them, we will 
comment in the bug as a note, but there are no intentions to file bugs for any 
regressions here.

Joel
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[William Lachance]

On 2017-05-19 2:44 PM, Gregory Szorc wrote:

The Perfherder alerts and tracking are informational only: nobody will be
backing you out merely because you added a compiler warning. While the
possibility of doing that now exists, I view that decision as up to the C++
module. I'm not going to advocate for it. So if you feel passionately, take
it up with them :)


Always happy to see people find new and unanticipated uses for Perfherder.

Should performance sheriffs file bugs when they encounter regressions?

e.g. this "alert" https://treeherder.mozilla.org/perf.html#/alerts?id=6673

has the underlying root cause: 
https://bugzilla.mozilla.org/show_bug.cgi?id=1345368


I worry that filing a bug for every regression is going to be too much.

Will
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[David Major]

I'm not sure if this is exactly the same as the ALLOW_COMPILER_WARNINGS
that we use for warnings-on-errors, but FWIW as of a couple of months
ago I cleaned out the last warning-allowance in our "own" code.
ALLOW_COMPILER_WARNINGS usage is now only in external libraries (I'm
counting NSS and NSPR as "external" for this purpose since I can't land
code to m-c directly).

On Fri, May 19, 2017, at 02:55 PM, Kris Maglione wrote:
> I've actually been meaning to post about this, with some questions.
> 
> I definitely like that we now print warnings at the end of builds, since
> it 
> makes them harder to ignore. But the current amount of warnings spew at
> the 
> end of every build is problematic, especially when a build fails and I
> need 
> to scroll up several pages to find out why.
> 
> Can we make some effort to get clean warnings output at the end of
> standard 
> builds? A huge chunk of the warnings come from NSS and NSPR, and should
> be 
> easily fixable. Most of the rest seem to come from vendored libraries,
> and 
> should probably just be whitelisted if we can't get fixes upstreamed.
> 
> On Fri, May 19, 2017 at 11:44:08AM -0700, Gregory Szorc wrote:
> >`mach build` attempts to parse compiler warnings to a persisted "database."
> >You can view a list of compiler warnings post build by running `mach
> >warnings-list`. The intent behind this feature was to make it easier to
> >find and fix compiler warnings. After all, something out of sight is out of
> >mind.
> >
> >There have been a few recent changes to increase the visibility of compiler
> >warnings with the expectation being that raising visibility will increase
> >the chances of someone addressing them. After all, a compiler warning is
> >either a) valid and should be fixed or b) invalid and should be ignored.
> >Either way, a compiler warning shouldn't exist.
> >
> >First, `mach build` now prints a list of all parsed compiler warnings at
> >the end of the build. More details are in the commit message:
> >https://hg.mozilla.org/mozilla-central/rev/4abe611696ab
> >
> >Second, Perfherder now records the compiler warning count as a metric. This
> >means we now have alerts when compiler warnings are added or removed, like
> >[1]. And, we can easily see graphs of compiler warnings over time, like
> >[2]. The compiler warnings are also surfaced in the "performance" tab of
> >build jobs on Treeherder, like [3].
> >
> >The Perfherder alerts and tracking are informational only: nobody will be
> >backing you out merely because you added a compiler warning. While the
> >possibility of doing that now exists, I view that decision as up to the C++
> >module. I'm not going to advocate for it. So if you feel passionately, take
> >it up with them :)
> >
> >Astute link clickers will note that the count metrics in CI are often noisy
> >- commonly fluctuating by a value of 1-2. I suspect there are race
> >conditions with interleaved process output and/or bugs in the compiler
> >warnings parser/aggregator. Since the values are currently advisory only,
> >I'm very much taking a "perfect is the enemy of good" attitude and have no
> >plans to improve the robustness.
> >
> >[1] https://treeherder.mozilla.org/perf.html#/alerts?id=6700
> >[2] https://mzl.la/2qFN0me and https://mzl.la/2rAkWR5
> >[3]
> >https://treeherder.mozilla.org/#/jobs?repo=autoland=100509284
> >___
> >dev-platform mailing list
> >dev-platform@lists.mozilla.org
> >https://lists.mozilla.org/listinfo/dev-platform
> 
> -- 
> Kris Maglione
> Senior Firefox Add-ons Engineer
> Mozilla Corporation
> 
> The X server has to be the biggest program I've ever seen that doesn't
> do anything for you.
>   --Ken Thompson
> 
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Improving visibility of compiler warnings

[Kris Maglione]

I've actually been meaning to post about this, with some questions.

I definitely like that we now print warnings at the end of builds, since it 
makes them harder to ignore. But the current amount of warnings spew at the 
end of every build is problematic, especially when a build fails and I need 
to scroll up several pages to find out why.


Can we make some effort to get clean warnings output at the end of standard 
builds? A huge chunk of the warnings come from NSS and NSPR, and should be 
easily fixable. Most of the rest seem to come from vendored libraries, and 
should probably just be whitelisted if we can't get fixes upstreamed.


On Fri, May 19, 2017 at 11:44:08AM -0700, Gregory Szorc wrote:

`mach build` attempts to parse compiler warnings to a persisted "database."
You can view a list of compiler warnings post build by running `mach
warnings-list`. The intent behind this feature was to make it easier to
find and fix compiler warnings. After all, something out of sight is out of
mind.

There have been a few recent changes to increase the visibility of compiler
warnings with the expectation being that raising visibility will increase
the chances of someone addressing them. After all, a compiler warning is
either a) valid and should be fixed or b) invalid and should be ignored.
Either way, a compiler warning shouldn't exist.

First, `mach build` now prints a list of all parsed compiler warnings at
the end of the build. More details are in the commit message:
https://hg.mozilla.org/mozilla-central/rev/4abe611696ab

Second, Perfherder now records the compiler warning count as a metric. This
means we now have alerts when compiler warnings are added or removed, like
[1]. And, we can easily see graphs of compiler warnings over time, like
[2]. The compiler warnings are also surfaced in the "performance" tab of
build jobs on Treeherder, like [3].

The Perfherder alerts and tracking are informational only: nobody will be
backing you out merely because you added a compiler warning. While the
possibility of doing that now exists, I view that decision as up to the C++
module. I'm not going to advocate for it. So if you feel passionately, take
it up with them :)

Astute link clickers will note that the count metrics in CI are often noisy
- commonly fluctuating by a value of 1-2. I suspect there are race
conditions with interleaved process output and/or bugs in the compiler
warnings parser/aggregator. Since the values are currently advisory only,
I'm very much taking a "perfect is the enemy of good" attitude and have no
plans to improve the robustness.

[1] https://treeherder.mozilla.org/perf.html#/alerts?id=6700
[2] https://mzl.la/2qFN0me and https://mzl.la/2rAkWR5
[3]
https://treeherder.mozilla.org/#/jobs?repo=autoland=100509284
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation

The X server has to be the biggest program I've ever seen that doesn't
do anything for you.
--Ken Thompson

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Improving visibility of compiler warnings

[Gregory Szorc]

`mach build` attempts to parse compiler warnings to a persisted "database."
You can view a list of compiler warnings post build by running `mach
warnings-list`. The intent behind this feature was to make it easier to
find and fix compiler warnings. After all, something out of sight is out of
mind.

There have been a few recent changes to increase the visibility of compiler
warnings with the expectation being that raising visibility will increase
the chances of someone addressing them. After all, a compiler warning is
either a) valid and should be fixed or b) invalid and should be ignored.
Either way, a compiler warning shouldn't exist.

First, `mach build` now prints a list of all parsed compiler warnings at
the end of the build. More details are in the commit message:
https://hg.mozilla.org/mozilla-central/rev/4abe611696ab

Second, Perfherder now records the compiler warning count as a metric. This
means we now have alerts when compiler warnings are added or removed, like
[1]. And, we can easily see graphs of compiler warnings over time, like
[2]. The compiler warnings are also surfaced in the "performance" tab of
build jobs on Treeherder, like [3].

The Perfherder alerts and tracking are informational only: nobody will be
backing you out merely because you added a compiler warning. While the
possibility of doing that now exists, I view that decision as up to the C++
module. I'm not going to advocate for it. So if you feel passionately, take
it up with them :)

Astute link clickers will note that the count metrics in CI are often noisy
- commonly fluctuating by a value of 1-2. I suspect there are race
conditions with interleaved process output and/or bugs in the compiler
warnings parser/aggregator. Since the values are currently advisory only,
I'm very much taking a "perfect is the enemy of good" attitude and have no
plans to improve the robustness.

[1] https://treeherder.mozilla.org/perf.html#/alerts?id=6700
[2] https://mzl.la/2qFN0me and https://mzl.la/2rAkWR5
[3]
https://treeherder.mozilla.org/#/jobs?repo=autoland=100509284
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform