Open Bug 187528 (buildwarning) Opened 18 years ago Updated 4 months ago

[META] Fix compiler 'Build Warnings'

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

People

(Reporter: mozilla-bugs, Unassigned)

References

(Depends on 43 open bugs)

Details

(Keywords: meta) 

There were some duplicated efforts (see bug 179775) with different people
creating conflicting patches aiming at reducing the number of warning in
Mozilla, so a general anti-warning tracking bug might be useful.

Right now brad TBox lists 1269 warning. If we count duplicates (a header file
warning replicated for every C/C++ file that include the header) only once,
there are about 688 warnings. The current distribution by "kind" is roughly
following:

    184 Unused variable `...'
    125 `...' might be used uninitialized in this function
     62 `...' defined but not used
     58 Choosing `...' over `...' For conversion from `...' to `...' Because
conversion sequence for the argument is better
     54 Comparison between signed and unsigned
     44 ...was hidden by `...'
     32 ANSI C forbids braced-groups within expressions
     14 Return of negative value `...' to `...' Negative integer implicitly
converted to unsigned type
     10 Multi-character character constant
      8 Value computed is not used
      8 Suggest parentheses around assignment used as truth value
      8 `...' is usually a function
      6 ANSI does not permit the keyword `...'
      5 Will be re-ordered to match declaration order
      5 Using synthesized `...' for copy assignment
      5 Missing initializer (near initialization for `...')
      5 Member initializers for `...'
      5 Enumeration value `...' not handled in switch
      5 Converting NULL to non-pointer type
      5 Assignment of negative value `...' to `...' Negative integer implicitly
converted to unsigned type
      5 And `...'
      4 `...' has virtual functions but non-virtual destructor
      3 Assignment to `...' from `...'
      2 `...' within comment
      2 Where cfront would use `...'
      2 Left shift count >= width of type
      2 Label `...' defined but not used
      2 Converting of negative value `...' to `...' Negative integer implicitly
converted to unsigned type
      1 ...was hidden by `...' ...was hidden by `...'
      1 String constant runs past end of line
      1 Precision used with `...' format
      1 Passing arg 1 of `...' discards qualifiers from pointer target type
      1 Negative integer implicitly converted to unsigned type
      1 Long unsigned int format, PRUint32 arg (arg 3)
      1 Initialization to `...' from `...'
      1 Initialization of negative value `...' to `...' Negative integer
implicitly converted to unsigned type
      1 Ignoring pragma:
      1 `...' declared `...' but never defined
      1 Decimal integer constant is so large that it is unsigned
      1 Control reaches end of non-void function `...'
      1 Control reaches end of non-void function
      1 Char format, void arg (arg 2)
      1 Argument `...' might be clobbered by `...' or `...'
      1 ANSI C does not support the `...' length modifier
      1 ANSI C does not allow extra `...' outside of a function
      1 Aggregate has a partly bracketed initializer

P.S. To simplify things, for those classes of warnings that have their own
tracking bugs (such as bug 59652 for "uninitialized" warnings and 179028 for
"hides" warnings), I only list the tracking bug as a dependency (see the
dependency tree if you want the complete list).
Depends on: 187530
*** Bug 228464 has been marked as a duplicate of this bug. ***
No longer depends on: 211231
No longer depends on: 114937
Severity: normal → trivial
Depends on: 90906, 114937, 211231, 217089
Summary: Get rid of compilation warnings in Mozilla → [META] Fix compiler 'Build Warnings'
Depends on: 205358
No longer depends on: 189712
Depends on: 219982
Depends on: 221128
Depends on: 214199
No longer depends on: 211231
No longer depends on: 114937
Depends on: 82151
Depends on: 228780
No longer depends on: 228780
No longer depends on: 90906
No longer depends on: 219982, 221128
No longer depends on: 214199
Blocks: 187015
No longer depends on: 187015
Depends on: 132141
No longer depends on: 132141
Depends on: 194240
No longer depends on: 195731
Depends on: 49640, 49641
Depends on: 229182
Depends on: 229456
Depends on: 229540
Depends on: 229631
Depends on: 229730
Depends on: 229874
Depends on: 229866
Depends on: 229897
Depends on: 229969
Depends on: 230001
Depends on: 230030
Depends on: 230397
No longer depends on: 230397
Depends on: 231716
No longer depends on: 230001
Depends on: 287540
Depends on: 299560, 299575, 299582, 299589, 299590, 299940
Depends on: 300068
Depends on: 305715
No longer depends on: 138188
Depends on: 468781
Depends on: 508531
Depends on: 499995
Back in 2003 it was written:
> Right now brad TBox lists 1269 warning. If we count duplicates (a header file
> warning replicated for every C/C++ file that include the header) only once,
> there are about 688 warnings. The current distribution by "kind" is roughly
> following:

After fixing about 100 warnings on my own, I was guided to this bug entry.
Today there are 1676 on my own compilation of TB3 using comm-central
 under linux, and unique warnings are 847.
There were about 50-60 warnings of the nature as follows. These are newcomers
which were not in the list above.
$MOZSRC/mozilla/editor/libeditor/html/nsHTMLEditor.cpp:5878:25: warning: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98

$MOZSRC/mozilla/editor/libeditor/html/nsHTMLEditor.cpp:5879:32: warning: invoking macro NS_ENSURE_TRUE argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98

These have been fixed and be posted to either Bug 609210 or
by creating additional bug entry. (I added 609210 in the Depends list.)
Depends on: 609210
Depends on: 665359
Depends on: 665387
Depends on: 665388
Depends on: 665389
Depends on: 665390
Depends on: 665391
Depends on: 659234
Depends on: 665532
Depends on: 665534
Depends on: 665541
Depends on: 665546
Depends on: 665549
Depends on: 665582
Depends on: 665595
Depends on: 665610
Depends on: 665622
Depends on: 665686
Depends on: 665723
Depends on: 458491
Depends on: 513503
Depends on: 604850
Apologies for the bugspam to all the newly added dependant bugs, but I feel that the current level of build warnings (eg 3100+ lines on win debug m-c tip, let alone other platforms) is beyond belief, so am starting on a crusade to eliminate them. 

Not only do the sheer number of existing warnings make it hard for contributors to work out if a patch has added more, but a proportion of them are potentially hiding real bugs (eg bug 659234).

Once the count is down to a more sensible number, I'm hoping that bugs like bug 187015, bug 604850 or bug 513503 will be able to keep them more under control - and mean that people are no longer able to unknowingly add a bunch of warnings with each commit, instead having to fix them as they go.
URL: http://tinderbox.mozilla.org/SeaMonke...
Severity: trivial → normal
Depends on: 35544, 215544, 221126, 230001, 239445, 243018, 243763, 301985, 301988, 302152, 323916, 326171, 326315, 330154, 331301, 331316, 362799, 368159, 370415, 388110, 389320, 395786, 399700, 413981, 433515, 439144, 445414, 447838, 449291, 450163, 450918, 452331, 454489, 454761, 456150, 457803, 461099, 463199, 467092, 469749, 474612, 481282, 488991, 491190, 491191, 494686, 495156, 501614, 502904, 503862, 514150, 514152, 514153, 514154, 521329, 527006, 551082, 552788, 552793, 552821, 557952, 558163, 560266, 562403, 562503, 563191, 563226, 567862, 567868, 568692, 568870, 571769, 572412, 577471, 579392, 581467, 581468, 581473, 581474, 581476, 581477, 581478, 581481, 582427, 582443, 583607, 585414, 587496, 589022, 589032, 592207, 592425, 595995, 598049, 598051, 598058, 602044, 605174, 605180, 609188, 609431, 609710, 610228, 610809, 612777, 613984, 615608, 616788, 616792, 618427, 620510, 630290, 637179, 637605, 637607, 639865, 643173, 646115, 646160, 652398, 653295, 654287, 654618, 654891, 654893, 658081, 660922, 661962, 666605, 666610, 666612, 666614, 666615, 666617, 666621, 666627, 666629, 666632, 666638, 666641, 666646, 666651, 666655, 666656, 666659, 666661, 666664, 666670, 666672, 666676, 666677, 666679, 666688, 669236
Alias: buildwarning
In lieu of an automatic build warnings count on tinderbox/TBPL (a la bug 187015); for now I'm manually downloading build logs from various platforms (from try to ensure they get a clobber) and running through a script (based on one sent to me by Chiaki Ishikawa) to scan/summarise the warnings.

The warning breakdown counts in the script are still a WIP (the compilers on each platform seem to like outputting the warnings in slightly different formats, so I'll need to work out the least false-positive regex), so for now, here's the summary. 

First figure is total build warning count, with the unique line count in parentheses.

linux-debug:    1106  (700)
linux-opt:      2785 (1023)
linux64-debug:  1228  (785)
linux64-opt:    3027 (1151)
osx-debug:       647  (540)
osx64-debug:     587  (483)
osx64-opt:      2462 (1482)
win-debug:      3149 (1660)
win-opt:        3312 (1752)

[Taken from a try job based on m-c 72331:821b5076d2c0 ~2011-07-05]

I'll update this every now and again, to show the progress made by this meta.
Depends on: 586113
Depends on: 670111
Depends on: 670338
Depends on: 554348
Depends on: 350380
Depends on: 440619
Depends on: 265368
Depends on: 488993
Depends on: 505181
Depends on: 431354
Depends on: 413226
Depends on: 297146
Depends on: 549766
Depends on: 301775
Depends on: 535861
Depends on: 263952
Depends on: 556886
Depends on: 331299
Depends on: 564324
Depends on: 219688
Depends on: 545972
Depends on: 343456
Depends on: 486774
Depends on: 670461
Depends on: 631155
Depends on: 672444
Depends on: 90906
Depends on: 228780
Depends on: 682139
Depends on: 677952
Depends on: 687070
Depends on: 687342
Depends on: 687121
Depends on: 687166
Depends on: 686601
Depends on: 687389
Depends on: 458726
Depends on: 458728
Depends on: 691041
Depends on: 693155
Depends on: 698933
Depends on: 131390
Depends on: 699228
Depends on: 699731
Depends on: 700712
Depends on: 700992
Depends on: 703178
Depends on: 708430
Depends on: 709603
Depends on: 711727
Depends on: 713632
Depends on: 714258
Depends on: 617819
Depends on: 726416
Depends on: 726417
Depends on: 726961
Depends on: 726964
Depends on: 726968
Depends on: 727163
Depends on: 727156
Depends on: 727212
Depends on: 734306
Depends on: 716278, 733448
Depends on: 739632
Depends on: 739635
Depends on: 739958
Depends on: 739962
Depends on: 740122
Depends on: 741223
Depends on: 743573
Depends on: 745266
Depends on: 745287
Depends on: 745291
Depends on: 745296
Depends on: 745568
Depends on: 729759
Depends on: 751314
Depends on: 754643
Depends on: 755048
Depends on: 756397
Depends on: 756523
Depends on: 763451
Depends on: 763580
Depends on: 764367
Depends on: 765799
Depends on: 773626
Depends on: 759434
Depends on: 774066
Depends on: 774071
Depends on: 774344
Depends on: 776879
Depends on: 778980
Depends on: 688055
Depends on: 785422
Depends on: 786372
Depends on: 786375
Depends on: 787040
Depends on: 787971
Depends on: 791565
Depends on: 791566
Depends on: 796085
Depends on: 798828
Depends on: 697810
Depends on: 808036
Depends on: 810668
Depends on: 813830
Depends on: 817913
Depends on: 818817
Depends on: 820791
Depends on: 820864
Depends on: 821396
Depends on: 826983
Depends on: 827032
Depends on: 819559
Depends on: 829288
Depends on: 843288
Depends on: 849866
Depends on: 851954
No longer depends on: 849980
Depends on: 849980
Depends on: 854096
Depends on: 857076
I submitted a following comment 
https://bugzilla.mozilla.org/show_bug.cgi?id=563195#c43

--- begin quote ---
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #18)
> 
> > At this point, I think I agree that it's not worth anybody's time to get
> > this fully working.  Something that I've wanted for a long time, though, is
> > enabling the build system to tell you which warnings you have introduced
> > compared to the previous build.  I think that goes a long way to help using
> > compiler warnings locally when writing code.
> 
> Can someone introduce this to feature to TryBuilder for FF and TB?

I submitted a bugzilla entry 
Bug 858543 - Creating "Warning Line Police" to reduce compile-time warning
to discuss this.

I think it is important for a long time health of the source base.

TIA
--- end quote ---

Please check out 
Bug 858543 - Creating "Warning Line Police" to reduce compile-time warning

I wonder if I should make 858543 block this bug entry?

TIA
Depends on: 862933
Depends on: 872557
Depends on: 872558
Depends on: 877706
Depends on: 881358
Depends on: 882616
Depends on: 883729
Depends on: 755049
Depends on: 900839
Depends on: 905542
Depends on: 911425
Depends on: 911428
Depends on: 883727
Depends on: 918176
Depends on: 919403
Depends on: 919844
Depends on: 920033
Depends on: 923486
Depends on: 923894
Depends on: 924608
No longer depends on: 666614
Depends on: 786118
Depends on: 925541
Depends on: 926583
Depends on: 927209
Depends on: 925195
Depends on: 921871
Depends on: 931664
Depends on: 931669
Depends on: 933124
Depends on: 933597
Depends on: 934262
Depends on: 935395
Depends on: 936336
Depends on: 936989
Depends on: 937459
Depends on: 940329
Depends on: 940330
Depends on: 940332
Depends on: 940336
Depends on: 940337
Depends on: 942399
Depends on: 942400
Depends on: 943655
Depends on: 944257
Depends on: 945077
Depends on: 945613
Depends on: 946116
Depends on: 946998
Depends on: 947626
Depends on: 948686
Depends on: 949353
Depends on: 949324
Depends on: 950499
Depends on: 951547
Depends on: 953145
Depends on: 956161
Depends on: 956126
Depends on: 956470
Depends on: 957075
Depends on: 961295
Depends on: 962077
Depends on: 962080
Depends on: 964852
Depends on: 965994
Depends on: 967006
Depends on: 967927
Depends on: 970159
Depends on: 972693
Depends on: 973795
Depends on: 973996
Depends on: 974785
Depends on: 976143
Depends on: 977723
Depends on: 977977
Depends on: 980169
Depends on: 980170
Depends on: 980211
Depends on: 980810
Depends on: 982334
Depends on: 983959
Depends on: 984071
Depends on: 984073
Depends on: 984074
Depends on: 984076
Depends on: 984079
Depends on: 984080
Depends on: 984081
Depends on: 984242
Depends on: 984250
Depends on: 986787
Depends on: 986788
Depends on: 986789
Depends on: 986793
Depends on: 986794
Depends on: 986795
Depends on: 989199
Depends on: 989203
Depends on: 768968
Depends on: 991451
Depends on: 995654
Depends on: 995655
Depends on: 997026
Depends on: 998179
Depends on: 999717
Depends on: 777515
Depends on: 1000712
Depends on: 1000758
Depends on: 1002891
Depends on: 1000465
Depends on: 1003702
Depends on: 1003917
Depends on: 1003921
Depends on: 1005784
Depends on: 1006307
Depends on: 1006982
Depends on: 1006998
Depends on: 1007741
Depends on: 1010706
Depends on: 1012218
Depends on: 1013263
Depends on: 1017110
Depends on: 1018270
Depends on: 1018288
Depends on: 1018554
Depends on: 1018555
Depends on: 1018680
Depends on: 1019381
Depends on: 1020414
Depends on: 1023075
Depends on: 1024318
Depends on: 1026336
Depends on: 992670
Depends on: 1027486
Depends on: 1028021
Depends on: 1028420
Depends on: 1031978
Depends on: 1031982
Depends on: 1032635
Depends on: 1032639
Depends on: 1032640
Depends on: 1032641
Depends on: 1032644
Depends on: 624205
Depends on: 1033188
Depends on: 1033192
Depends on: 1034466
Depends on: 1035607
Depends on: 1039917
Depends on: 1039923
Depends on: 1047301
Depends on: 1024795
Depends on: 1052014
Depends on: 1052033
Depends on: 1057066
Depends on: 1066935
Depends on: 1068417
Depends on: 1069110
Depends on: 1072071
Depends on: 1072296
Depends on: 1072317
Depends on: 1078430
Depends on: 1079672
Depends on: 1081010
Depends on: 1081561
Depends on: 1089370
Depends on: 1091979
Depends on: 1092001
Depends on: 1092028
Depends on: 1092110
Depends on: 1092710
Depends on: 1092711
Depends on: 1092923
Depends on: 1095882
Depends on: 1095878
Depends on: 1095926
Depends on: 1095990
Depends on: 1098134
Depends on: 1103859
Depends on: 1105867
Depends on: 1105870
Depends on: 1105974
Depends on: 1108932
Depends on: 1108934
Depends on: 1108938
Depends on: 1110641
Depends on: 1111190
Depends on: 1113031
No longer depends on: 1113031
Depends on: 1113229
Depends on: 1114898
Depends on: 1115264
Depends on: 1115477
Depends on: 1117267
Depends on: 1117466
Depends on: 989303
Depends on: 1117635
Depends on: 1117636
Depends on: 1119499
Depends on: 1120312
Depends on: 1121451
Depends on: 1123553
Depends on: 1124029
Depends on: 1125592
Depends on: 1125665
Depends on: 1125690
Depends on: 1125693
Depends on: 1125698
Depends on: 1125701
Depends on: 1125947
Depends on: 1130244
Depends on: 1130828
Depends on: 1132679
Depends on: 1132818
Depends on: 1135535
Depends on: 1136004
Depends on: 1138198
Depends on: 1141917
Depends on: 1142860
Depends on: 1142863
Depends on: 1143336
Depends on: 1143512
Depends on: 1143994
Depends on: 1150011
Depends on: 1152122
Depends on: 1153114
Depends on: 1153122
Depends on: 1153373
Depends on: 1153378
Depends on: 1153579
Depends on: 1159124
Depends on: 1163364
Depends on: 1167834
Depends on: 1170059
Depends on: 1170066
Depends on: 1171358
Depends on: 1171361
Depends on: 1171368
Depends on: 1185422
Depends on: 1190148
Depends on: 1190149
Depends on: 1191283
Depends on: 1192070
Depends on: 1192586
Depends on: 1193470
Depends on: 1194806
Depends on: 1194951
Depends on: 1194954
Depends on: 1194955
Depends on: 1194985
Depends on: 1198128
Depends on: 1202568
Depends on: 1204400
Depends on: 1209164
Depends on: 1215774
Depends on: 1217835
Depends on: 1226907
Depends on: 1229189
Depends on: 1229196
We're now in a *much* better situation w.r.t. compiler warnings. The test machines run with --enable-warnings-as-errors and we use ALLOW_COMPILER_WARNINGS to opt out of warnings for third-party code on a directory basis, and we can also disable warnings for specific source files in moz.build files as necessary.

So I suspect that most of the warnings cited in bugs blocking this bug either no longer occur, or occur in ways that we've explicitly allowed for (either third-party code, or one-off warnings in our code that we've decided to accept).

The other bugs blocking this one that are about ways to reduce the warnings count (e.g. bug 858543) are likely no longer relevant.

So I think there's a good chance that this bug can be closed. It might be good to have someone go through the still-open blocking bugs here and close them out (if appropriate). It's possible that this process might find evidence against my hypothesis that this bug can be closed.

cpeterson, would you be interested in going through the open blockers? dholbert, do you have any opinions here?
Flags: needinfo?(dholbert)
Flags: needinfo?(cpeterson)
(In reply to Nicholas Nethercote [:njn] from comment #6)
> cpeterson, would you be interested in going through the open blockers?

Sounds good. I'll triage the open blockers soon. I agree that this particular bug is no longer useful.
Flags: needinfo?(cpeterson)
(In reply to Nicholas Nethercote [:njn] from comment #6)
> dholbert, do you have any opinions here?

tl;dr: I'm fine with this being closed, but I think we need a way to track these sorts of bugs going forward. (and for now, that's this bug)

I agree that we've *largely* solved build warnings in our codebase (the main exception being imported 3rd-party code, which we can't manageably take spot-fixes for). However, I expect we will still have compile-warning bugs in our code in the future[1], and I think there's value in having a way of tracking them as a category. This metabug is not the best way of tracking them; perhaps we should add a new bugzilla keyword for build-warnings, and just use that instead of this metabug going forward? (and maybe someone can automate an update to all dependencies of this metabug to have that keyword)

So: until we have a better alternative, in the rare case that I need to file a new build-warning bug, I'd probably still make it block this metabug.

[1] While we've basically prevented developers from checking in new code that introduces build warnings (yay!), we could still have accidents where someone copypastes a moz.build file that happens to have ALLOW_COMPILER_WARNINGS into a new directory, and some build warnings slip through as a result.  More legitimately, I fully expect that new gcc/clang/MSVC versions will create new categories of warnings (either enabled by default or useful-enough-that-we-want-to-enable-them), which will likely give us new build-warning-bugs that we'll want to file & fix (& track).

(In reply to Chris Peterson [:cpeterson] from comment #7)
> Sounds good. I'll triage the open blockers soon.

Nice, thanks! You are a hero.
Flags: needinfo?(dholbert)
Depends on: 1234130
Depends on: 1239539
Depends on: 1245099
Depends on: 1225428
Depends on: 1256452
Depends on: 1266614
Depends on: 1266615
Depends on: 1275475
Depends on: 1277428
Component: Tracking → General
QA Contact: chofmann
Assignee: chofmann → nobody
Depends on: 1304305
Depends on: 1313903
Depends on: 1313905
Depends on: 1317177
Depends on: 1320738
Depends on: 1330202
Depends on: 1333686
Depends on: 1340835
Depends on: 1341162
Depends on: 1355699
Depends on: Wcomma
Depends on: 1363555
Depends on: 1371483
No longer blocks: 1214953
Depends on: Wunused-member-function, 1214953
Depends on: 1378412
Depends on: 1379517
Depends on: 1379520
Depends on: 1379521
Depends on: 1330240, 1346305, 1378312, 1347661, 1330359
No longer depends on: 1378312
Depends on: 1379523
Depends on: 1387158
Depends on: 1400374
Depends on: 1298144
Depends on: 1415470
Depends on: 1522203
Depends on: 1564298
Depends on: 1564216
Depends on: 1588921
Depends on: 1613074
No longer depends on: 1613074
Product: Core → Firefox Build System
Depends on: 1613074
Depends on: 1655367
You need to log in before you can comment on or make changes to this bug.