825820 - mach's warnings database does not detect when all warnings for a file are fixed at once

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Nicholas Nethercote [inactive]]

Steps to reproduce:

1. Do a clean build.

2. Run |mach warnings-list|.

3. Modify one file so as to add or remove compile warning(s).  For example, the patch in bug 825818 fixes six clang-3.2 warnings.

4. Rebuild.

5. Run |mach warnings-list|.  The results are unchanged from step 2.

|mach warnings-summary| has the same problem.

Comment 1

[Nicholas Nethercote [inactive]]

Related to this... the help for |warnings-list| says this:

  usage: mach [global arguments] command [command arguments] warnings-list
         [-h] [report]

  positional arguments:
    report    Warnings report to display. If not defined, show the most recent
              report.

I don't know what a "report" is in this context.

Comment 2

[Nicholas Nethercote [inactive]]

Actually, step 3 in comment 0 should say "so as to remove compile warning(s)".  If I make a change that triggers a new warning, that gets added to the warnings database just fine.

So the problem is that mach doesn't look for warnings that have gone away.  I looked at the mach code a bit (e.g. python/mozbuild/mozbuild/compilation/warnings.py) and there doesn't seem to be any code to handle this case.

Comment 3

[Gregory Szorc [:gps]]

We do look for warnings that go away! We record the SHA-1 of the file that contains the warning. If the SHA-1 changes, we wipe out all recorded warnings for that file and replace with the new set.

This works in most scenarios. However, if things like CFLAGS change or a macro from a #include'd file changes the warning, we don't detect that. We could implement support for these, if needed.

Could you provide more specifics on the exact warning so I can triage what the root cause is?

Comment 4

[Nicholas Nethercote [inactive]]

See bug 825818 for a specific warning set and patch that fixes them.

But I've been fixing lots of clang warnings recently and this problem occurs with every warning I've fixed -- no futzing with CFLAGS or inter-file macros or anything else unusual is necessary.

(Bug 825105 is another case, with a patch, that you could try.)

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: imported patch cairo-fix-most-hmm

Here's another patch for demonstration.  Without it, on my Ubuntu box, clang
3.2 gives 93 -Wtautological-constant-out-of-range-compare warnings in cairo,
like this:

  comparison of constant 100 with expression of type 'cairo_status_t' (aka 'enum _cairo_status') is always true

Both warnings.json and the console output agree about the 93 warnings.

After applying the patch and rebuilding, the console output says there are only
6 such warnings remaining, but the warnings.json file has the exact same
content as before (well, the JSON is in different order, but the line/word/char
counts are the same, so I assume it's identical), and mach
warnings-{list,summary} still claims there are 93 such warnings.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Call WarningsDatabase.prune at the end of Build().

Aha!  I found the following comment in warnings.py, which explains things
nicely.

  During the course of development, it is common for warnings to change
  slightly as source code changes. For example, line numbers will disagree.
  The WarningsDatabase handles this by storing the hash of a file a warning
  occurred in. **At warning insert time, if the hash of the file does not match
  what is stored in the database, the existing warnings for that file are
  purged from the database.**
          
  Callers should periodically prune old, invalid warnings from the database
  by calling prune(). A good time to do this is at the end of a build.

(My emphasis.)

If the number of warnings in a file drops from, say, 2 to 1, the file's old
warnings will be purged.  However, if the number drops from 2 to 0, this check
will not occur -- because no warnings are inserted for the file! -- and the
file's old warnings will not be purged.

This patch fixes the problem by adding a call to prune(), as suggested.  I
tested this locally and it works.

However, this is something of a band-aid fix.  It would be nicer if Build()
instead cleared the warnings of every file that gets rebuilt.

(BTW, nice work with that comment.  It explained things very clearly.)

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 697253 [details] [diff] [review]
imported patch cairo-fix-most-hmm

Oh, and this cairo patch is a bad example, because it modifies some .h files which prevents warnings in .c files, and so even adding prune() doesn't fix it.  (That limitation is a real shame... but I guess computing the hash after pre-processing is difficult.)

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 697275 [details] [diff] [review]
Call WarningsDatabase.prune at the end of Build().

Review of attachment 697275 [details] [diff] [review]:
-----------------------------------------------------------------

I swear this line existed at one point and mysteriously vanished. Ugh. Thanks for catching it.

Comment 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10834aa54577

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/10834aa54577