1225428 - Address some compilation maybe-uninitialized warnings

Status: RESOLVED WONTFIX

(Core :: General, defect)

Description

[Paul Bignier]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20151104125556

Steps to reproduce:

'make' in the gecko repo


Actual results:

Got ~160 [-Wmaybe-uninitialized] warnings


Expected results:

No warnings

Comment 1

[Paul Bignier]

Attachment: Patch for commit #1

Comment 2

[Paul Bignier]

Attachment: Patch for commit #2

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Poiru, can you review these patches or redirect them to someone who does? Thank you.

Comment 4

[Paul Bignier]

Mind you these patches only fix some of the warnings at the moment.
Reason is indicated in the commit message (my editor removing trailing whitespaces makes a lot of modifications...)

Comment 5

[Biru [:poiru]]

(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #3)
> Poiru, can you review these patches or redirect them to someone who does?
> Thank you.

Yeah, I can do that.

(In reply to Paul Bignier from comment #4)
> Mind you these patches only fix some of the warnings at the moment.
> Reason is indicated in the commit message (my editor removing trailing
> whitespaces makes a lot of modifications...)

Thanks for your patches! I think the whitespace changes would be better saved for separate patches. Could you perhaps configure your editor to leave whitespace alone or, alternatively, generate patches without the whitespace changes (`-w` flag)?

Comment 6

[Paul Bignier]

Attachment: Patch for commit #1 - no trailing whitespaces

Patch containing the two previous ones created with 'git format-patch -w' to ignore trailing whitespaces modifications

Comment 7

[Paul Bignier]

(In reply to Paul Bignier from comment #6)
> Created attachment 8688424 [details] [diff] [review]
> Patch for commit #1 - no trailing whitespaces
> 
> Patch containing the two previous ones created with 'git format-patch -w' to
> ignore trailing whitespaces modifications

Hold on, that gives me a compilation error in nsEditor.cpp :S

Comment 8

[Nathan Froyd [:froydnj]]

Changes to nsprpub/ and security/nss/ will each require their own separate bugs (one for nsprpub/, one for security/nss/), and those changes will need to be done against the NSPR and NSS repos, rather than mozilla-central.

http://hg.mozilla.org/projects/nspr/
http://hg.mozilla.org/projects/nss/

Comment 9

[Paul Bignier]

Attachment: Patch for commit #1 - no trailing whitespaces

Comment 10

[Paul Bignier]

Attachment: Patch for commit #1 - no trailing whitespaces - no nss no nspr

(In reply to Nathan Froyd [:froydnj] from comment #8)
> Changes to nsprpub/ and security/nss/ will each require their own separate
> bugs (one for nsprpub/, one for security/nss/), and those changes will need
> to be done against the NSPR and NSS repos, rather than mozilla-central.
> 
> http://hg.mozilla.org/projects/nspr/
> http://hg.mozilla.org/projects/nss/

Removed the concerned files from the patch and created separate bugs:

 - NSPR
 https://bugzilla.mozilla.org/show_bug.cgi?id=1225479
 - NSS:
 https://bugzilla.mozilla.org/show_bug.cgi?id=1225469

Comment 11

[Chris Peterson [:cpeterson]]

Note that your patch may receive some pushback because some Gecko module owners do not like unnecessarily initializing variables to suppress false positives warnings because that may mask legitimate warnings. In my experience, gcc's -Wmaybe-uninitialized warnings are mostly false positives (like 90% of them).

Comment 12

[Paul Bignier]

(In reply to Chris Peterson [:cpeterson] from comment #11)
> Note that your patch may receive some pushback because some Gecko module
> owners do not like unnecessarily initializing variables to suppress false
> positives warnings because that may mask legitimate warnings. In my
> experience, gcc's -Wmaybe-uninitialized warnings are mostly false positives
> (like 90% of them).

I'm not emotionally attached to my patch, you can discard it without me minding :)

It was made for gecko to output cleaner compilation logs and in some cases to give a vague idea of the type represented ("nscoord color = 0;" shows 'nscoord' is some kind of number (maybe address), not a struct or class)

Regards,
Paul

Comment 13

[Paul Bignier]

Attachment: Patch for commit #2 - no trailing whitespaces

Patch #2, now outing ~40 warnings (not counting the ones in nss/nsprpub).

BR,
Paul

Comment 14

[Biru [:poiru]]

Paul, could you split the patches by module (e.g. one patch for layout/ changes) and request review from the module owners or peers[0]? I suggest you start with just a single directory in case people don't want this.

I also noticed that you touched various pieces of 3rd party code[1]. Could you remove those hunks? If you want to get rid of warnings in 3rd party code, you should instead add `-Wnomaybe-uninitialized` to the relevant moz.build files (see examples[2]).

[0]: https://wiki.mozilla.org/Modules/Core
[1]: http://mxr.mozilla.org/mozilla-central/source/tools/rewriting/ThirdPartyPaths.txt?force=1
[2]: http://mxr.mozilla.org/mozilla-central/search?string=-Wno&find=moz.build

Comment 15

[Sylvestre Ledru [:Sylvestre]]

(In reply to Birunthan Mohanathas [:poiru] from comment #14)ple don't want this.
> 
> I also noticed that you touched various pieces of 3rd party code[1]. Could
> you remove those hunks? If you want to get rid of warnings in 3rd party
> code, you should instead add `-Wnomaybe-uninitialized` to the relevant
> moz.build files (see examples[2]).
Or fix them upstream directly.

Comment 16

[Paul Bignier]

After successful landing of two modules, I split the rest into modules & requested reviews:

 DOM: https://bugzilla.mozilla.org/show_bug.cgi?id=1245111
 Editor: https://bugzilla.mozilla.org/show_bug.cgi?id=1245113
 GFX: https://bugzilla.mozilla.org/show_bug.cgi?id=1245115
 JS: https://bugzilla.mozilla.org/show_bug.cgi?id=1245108
 Layout: https://bugzilla.mozilla.org/show_bug.cgi?id=1245104
 Netwerk: https://bugzilla.mozilla.org/show_bug.cgi?id=1245106
 Toolkit: https://bugzilla.mozilla.org/show_bug.cgi?id=1245103
 XPCOM: https://bugzilla.mozilla.org/show_bug.cgi?id=1245099

Sill have a dozen in gfx/cairo/, media/libnestegg/, media/mtransport/ & toolkit/components/protobuf/ but those are listed in the thirdparties so I won't commit them.

Best regards,
Paul

Comment 17

[Chris Peterson [:cpeterson]]

Awesome! Thanks, Paul.

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Paul Bignier from comment #12)
> (In reply to Chris Peterson [:cpeterson] from comment #11)
> > Note that your patch may receive some pushback because some Gecko module
> > owners do not like unnecessarily initializing variables to suppress false
> > positives warnings because that may mask legitimate warnings.
> 
> I'm not emotionally attached to my patch, you can discard it without me
> minding :)

I'm glad you said this. :)

Skimming the attached patch here, I am weakly against taking most of the changes, for the reasons outlined in bug 1245104 -- the cost/benefit calculus doesn't add up.

BENEFIT:
 * Cleaner compiler output for compilers that spam -Wmaybe-uninitialized.
 * Theoretical benefit that we might fix some subtle uninitialized-usage bugs (but maybe not; these could also be 100% false positives).

COST:
 * Reduced performance.
 * Makes it impossible for tools to help us discover *real* uninitialized usages (current or future) of these variables. See bug 1245104 comment 1 for more details.

The second "COST" in particular is very worrisome.

If these compiler warnings are really causing noise, a possibly better solution would be to simply disable them, or perhaps to add a way to suppress them in particular places, since they're known false-positivey.  That would give us the primary BENEFIT here with none of the COSTs.

> It was made for gecko to output cleaner compilation logs

(per above, simply turning off this warning is an arguably-better way to achieve that)

> and in some cases
> to give a vague idea of the type represented ("nscoord color = 0;" shows
> 'nscoord' is some kind of number (maybe address), not a struct or class)

To the extent that our types are confusing/opaque to code readers, we should solve it with better documentation and better discoverability of the type definition -- not by adding dummy initializations all over the place.

Comment 19

[Daniel Holbert [:dholbert]]

So, to avoid each separate reviewer on each helper-bug having to go through the process of thinking through the issues brought up in comment 18 (or worse, not think through those implications and simply rubberstamping): I tend to think that we should just close out all of the helper-bugs, as well as this bug here, as WONTFIX, except for any chunks that seem likely to be *really* uninitialized usage bugs (not false positives).

Paul, is this OK with you? Are there any pieces that you found that you think we really should take, despite my concerns?  (and: sorry that we prompted you to do a bunch of potentially-unnecessary patch-churn. :-/)

Comment 20

[Daniel Holbert [:dholbert]]

(Also: for future patches, you should add "unified = 8" to the [diff] section of your $HOME/.hgrc file, so that your patches have 8 lines of context instead of 3 -- which makes it a bit easier to see how your changes fit into the surrounding code. See this MDN page with a sample .hgrc, for reference:
  https://developer.mozilla.org/en-US/docs/Installing_Mercurial#Basic_configuration
)

Comment 21

[Paul Bignier]

(In reply to Daniel Holbert [:dholbert] from comment #19)

Well I wouldn't have done these patches if I weren't convinced with their utilities, but your experience prevails.

(In reply to Daniel Holbert [:dholbert] from comment #20)
> (Also: for future patches, you should add "unified = 8" to the [diff]

Thanks for the tip :)

Comment 22

[Daniel Holbert [:dholbert]]

I guess we should call this WONTFIX, like bug 1245111.

If there happen to be any legitimately uninitialized (not false-positive) issues that remain here, it'd probably be best to start fresh with a focused & rebased patch, and a fresh bug.