Closed Bug 1738684 Opened 4 years ago Closed 2 years ago

Warning lines missing from build output

Categories

(Firefox Build System :: General, defect, P5)

Product:
Firefox Build System
Component:
General
Type:
defect

Tracking

(firefox121 fixed)

Status:
RESOLVED FIXED
Milestone:
121 Branch
Tracking Flags:
Tracking Status
firefox121 --- fixed

People

(Reporter: jesup, Assigned: glandium)

References

Details

Attachments

(2 files, 1 obsolete file)

D130598.diff
10.25 KB, patch
Details | Diff | Splinter Review
Bug 1738684 - Avoid the warnings collector from eating build output.
48 bytes, text/x-phabricator-request
Details | Review

Some warning lines are missing from build output. Example with -Wthread-safety when a member (incorrectly) has attribute(exclusive_locks_required(mLock)):

Manually running the clang command, I see this:

In file included from netwerk/cache2/CacheStorage.cpp:6:
In file included from netwerk/cache2/CacheStorage.h:9:
netwerk/cache2/CacheEntry.h:390:43: warning: 'exclusive_locks_required' attribute only applies to functions [-Wignored-attributes]
  nsCOMPtr<nsIOutputStream> mOutputStream EXCLUSIVE_LOCKS_REQUIRED(mLock) ;
                                          ^

etc

while if I run it through ./mach build -v, I see this:

 0:01.90 In file included from ../../../netwerk/cache2/CacheStorage.cpp:6:
 0:01.90 In file included from ../../../netwerk/cache2/CacheStorage.h:9:
 0:01.90   nsCOMPtr<nsIOutputStream> mOutputStream EXCLUSIVE_LOCKS_REQUIRED(mLock) ;
 0:01.90                                           ^

etc

Thanks! Can you share:

  • Which revision you're on (is this repro-able on current central/asdfec54c35cb3bd?)
  • How are you providing the -Wthread-safety option to the build system? Can you share your mozconfig?
Flags: needinfo?(rjesup)
See Also: → 1721471
Priority: -- → P3
Assignee: nobody → ahochheiden
Status: NEW → ASSIGNED
Assignee: ahochheiden → mhentges

The NI from earlier can be cleared:

  • The revision is a stack of private patches on top of d8a16efe2d2c
  • The (simplified by me) mozconfig looks like:
export TOPSRCDIR=$topsrcdir

. $TOPSRCDIR/browser/config/mozconfig

export CFLAGS="-Wthread-safety"
export CXXFLAGS="-Wthread-safety"
ac_add_options --enable-debug
ac_add_options --enable-optimize

There's been some DM-discussion outside of this bug.
To summarize, I've been pushing to reproduce this problem on <a specific stack of patches> with <a specific mozconfig>, but I can't repro the problem.

Since this is affecting very few people (that I can tell), and since there's other priorities to dig into, I'm going to defer this a little.
If there's a rock-solid repro that I can dig into, please let me know :)

Assignee: mhentges → nobody
Status: ASSIGNED → UNCONFIRMED
Ever confirmed: false
Flags: needinfo?(rjesup)
Priority: P3 → P5

This continues to be a problem, and we are planning to require resolving these warnings and enabling -Wthread-safety as a default soon.

I'll try to upload a simplified patch showing it

SImplified patch. You need to add this to .mozconfig (perhaps that was the problem reproing before?)
export CFLAGS="-Wthread-safety"
export CXXFLAGS="-Wthread-safety"

Flags: needinfo?(mhentges)

I need additional information to reproduce this. I have a log file here (from a ./mach build -v), but a cursory scroll shows that I'm seeing both the affected line of code and the warning, for each warning.

I can go through every warning to find one that matches what you're referring to, but there's 4,131 of 'em, and I don't think that that would be effective.

For me to repro this, I need:

  1. ✅ A patch which will repro the problem and a base revision to apply it to (it looks like current central, aka 3652687, works for this)
  2. ✅ Additional config needed to repro the issue
  3. ❌ A specific description of what goes wrong.

I looked for the EXCLUSIVE_LOCKS_REQUIRED warning as provided in the initial comment, but that isn't here in the current log with your provided patch.

If you can give me a specific warning text that I can find in the build output, that'd be helpful.

Flags: needinfo?(mhentges) → needinfo?(rjesup)

Update the patch to make it easier to repro

Apply this patch to 0cb5037fb92a
add

export CFLAGS="-Wthread-safety"
export CXXFLAGS="-Wthread-safety"

to .mozconfig
Do a debug build
touch netwerk/sctp/datachannel/DataChannel.cpp
do another debug build (this isolates it out from other warning-spew)

Actual results:

0:02.20 netwerk/sctp/datachannel
 0:02.22     free(data);
 0:02.22     ^
 0:02.22 ../../../../netwerk/sctp/datachannel/DataChannel.cpp:2376:13: note: mutex acquired here
 0:02.22       mLock.Lock();
 0:02.22             ^
 0:02.22 1 warning generated.

Expected result (from using the command line seen in ./mach build -v):

netwerk/sctp/datachannel/DataChannel.cpp:2393:5: warning: mutex 'mLock' is not held on every path through here [-Wthread-safety-analysis]
    free(data);
    ^
netwerk/sctp/datachannel/DataChannel.cpp:2376:13: note: mutex acquired here
      mLock.Lock();
            ^
Flags: needinfo?(rjesup) → needinfo?(mhentges)

When doing the steps listed above, I'm seeing all three elements: the file reference with line number, the source code line itself, and the caret (^). I can't reproduce the source code line being missing yet.

 0:04.43 /home/mitch/dev/firefox/netwerk/sctp/datachannel/DataChannel.cpp:2393:5: warning: mutex 'mLock' is not held on every path through here [-Wthread-safety-analysis]
 0:04.43     free(data);
 0:04.43     ^
 0:04.43 /home/mitch/dev/firefox/netwerk/sctp/datachannel/DataChannel.cpp:2376:13: note: mutex acquired here
 0:04.43       mLock.Lock();
 0:04.43             ^

Here's the full output I'm seeing:

~/d/firefox % hg log -r ". or parents(.)"
changeset:   613281:496bc60d1cfa
tag:         tip
parent:      612891:0cb5037fb92a
user:        Randell Jesup <rjesup@wgate.com>
date:        Sat Mar 05 14:51:05 2022 +0000
summary:     WIP: Bug 1738684 - missing_warning_lines

changeset:   612891:0cb5037fb92a
user:        Mozilla Releng Treescript <release+treescript@mozilla.org>
date:        Fri Mar 04 19:08:53 2022 +0000
summary:     no bug - Bumping Firefox l10n changesets r=release a=l10n-bump DONTBUILD

~/d/firefox % cat .mozconfig mozconfig
cat: .mozconfig: No such file or directory
export CFLAGS="-Wthread-safety"
export CXXFLAGS="-Wthread-safety"
~/d/firefox % touch netwerk/sctp/datachannel/DataChannel.cpp && ./mach build
 0:00.74 Clobber not needed.
 0:00.80 Adding make options from /home/mitch/dev/firefox/mozconfig
    MOZ_OBJDIR=/home/mitch/dev/firefox/obj-x86_64-pc-linux-gnu
    OBJDIR=/home/mitch/dev/firefox/obj-x86_64-pc-linux-gnu
    FOUND_MOZCONFIG=/home/mitch/dev/firefox/mozconfig
    export FOUND_MOZCONFIG
  Parallelism determined by memory: using 16 jobs for 16 cores based on 31.3 GiB RAM and estimated job size of 1.0 GiB
 0:00.80 /usr/bin/gmake -f client.mk -j16 -s
 0:00.97 Elapsed: 0.00s; From dist/private: Kept 0 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.97 Elapsed: 0.00s; From dist/public: Kept 0 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.98 Elapsed: 0.01s; From dist/xpi-stage: Kept 94 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.06 Elapsed: 0.09s; From _tests: Kept 1249 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.11 Elapsed: 0.13s; From dist/bin: Kept 2679 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.23 Elapsed: 0.27s; From dist/include: Kept 6582 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.27 ./buildid.h.stub
 0:01.27 ./source-repo.h.stub
 0:01.74 ./application.ini.stub
 0:01.74 ./IsCombiningDiacritic.h.stub
 0:01.87 ./application.ini.h.stub
 0:02.37 toolkit/library/rust/force-cargo-library-build
 0:02.37 testing/geckodriver/force-cargo-program-build
 0:02.38 browser/app
 0:02.42     Blocking waiting for file lock on package cache
 0:02.85 netwerk/sctp/datachannel
 0:03.10 toolkit/library/buildid.cpp.stub
 0:03.28 browser/app/firefox
 0:03.29 netwerk/test/http3server/force-cargo-program-build
 0:03.34 toolkit/library
 0:03.34     Blocking waiting for file lock on package cache
 0:03.92     Blocking waiting for file lock on package cache
 0:04.43 /home/mitch/dev/firefox/netwerk/sctp/datachannel/DataChannel.cpp:2393:5: warning: mutex 'mLock' is not held on every path through here [-Wthread-safety-analysis]
 0:04.43     free(data);
 0:04.43     ^
 0:04.43 /home/mitch/dev/firefox/netwerk/sctp/datachannel/DataChannel.cpp:2376:13: note: mutex acquired here
 0:04.43       mLock.Lock();
 0:04.43             ^
 0:05.11     Blocking waiting for file lock on package cache
 0:06.10 1 warning generated.
 0:06.31     Blocking waiting for file lock on package cache
 0:06.31     Blocking waiting for file lock on package cache
 0:06.32     Blocking waiting for file lock on package cache
 0:06.32     Blocking waiting for file lock on package cache
 0:06.33     Blocking waiting for file lock on package cache
 0:06.33     Finished release [optimized] target(s) in 3.03s
 0:06.34     Finished release [optimized] target(s) in 3.96s
 0:06.38 warning: using `procedural-masquerade` crate
 0:06.39   --> toolkit/components/cascade_bloom_filter/src/lib.rs:24:1
 0:06.39    |
 0:06.39 24 | / rental! {
 0:06.39 25 | |     mod rentals {
 0:06.39 26 | |         use super::Cascade;
 0:06.39 27 | |
 0:06.39 ...  |
 0:06.39 33 | |     }
 0:06.39 34 | | }
 0:06.39    | |_^
 0:06.39    |
 0:06.39    = note: `#[warn(proc_macro_back_compat)]` on by default
 0:06.39    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
 0:06.39    = note: for more information, see issue #83125 <https://github.com/rust-lang/rust/issues/83125>
 0:06.39    = note: The `procedural-masquerade` crate has been unnecessary since Rust 1.30.0. Versions of this crate below 0.1.7 will eventually stop compiling.
 0:06.39    = note: this warning originates in the macro `rental` (in Nightly builds, run with -Z macro-backtrace for more info)
 0:06.39 warning: `cascade_bloom_filter` (lib) generated 1 warning
 0:06.39 warning: using `procedural-masquerade` crate
 0:06.39    --> security/manager/ssl/cert_storage/src/lib.rs:139:1
 0:06.39     |
 0:06.39 139 | / rental! {
 0:06.39 140 | |     mod holding {
 0:06.39 141 | |         use super::{Cascade, Mmap};
 0:06.39 142 | |
 0:06.39 ...   |
 0:06.39 148 | |     }
 0:06.39 149 | | }
 0:06.39     | |_^
 0:06.39     |
 0:06.39     = note: `#[warn(proc_macro_back_compat)]` on by default
 0:06.39     = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
 0:06.39     = note: for more information, see issue #83125 <https://github.com/rust-lang/rust/issues/83125>
 0:06.39     = note: The `procedural-masquerade` crate has been unnecessary since Rust 1.30.0. Versions of this crate below 0.1.7 will eventually stop compiling.
 0:06.39     = note: this warning originates in the macro `rental` (in Nightly builds, run with -Z macro-backtrace for more info)
 0:06.39 warning: `cert_storage` (lib) generated 1 warning
 0:06.39     Finished release [optimized] target(s) in 4.01s
 0:06.39 warning: the following packages contain code that will be rejected by a future version of Rust: cascade_bloom_filter v0.1.0 (/home/mitch/dev/firefox/toolkit/components/cascade_bloom_filter), cert_storage v0.0.1 (/home/mitch/dev/firefox/security/manager/ssl/cert_storage), rental v0.5.6
 0:06.39 note: to see what the problems were, use the option `--future-incompat-report`, or run `cargo report future-incompatibilities --id 7`
 0:06.51 security/manager/ssl/ipcclientcerts/force-cargo-library-build
 0:06.51 toolkit/library/build/libxul.so
 0:07.96     Finished release [optimized] target(s) in 1.45s
 0:11.10 ./dependentlibs.list.stub
 0:13.57 Packaging specialpowers@mozilla.org.xpi...
 0:13.67 Packaging quitter@mozilla.org.xpi...
 0:13.76 Packaging mozscreenshots@mozilla.org.xpi...
 0:13.89 472 compiler warnings present.
 0:13.95 Overall system resources - Wall time: 13s; CPU: 41%; Read bytes: 5595136; Write bytes: 13012992; Read time: 53; Write time: 195
 0:13.96 warning: netwerk/sctp/datachannel/DataChannel.cpp:2393:5 [-Wthread-safety-analysis] mutex 'mLock' is not held on every path through here
To view resource usage of the build, run |mach resource-usage|.
 0:13.96 Your build was successful!
To take your build for a test drive, run: |mach run|
For more information on what to do now, see https://firefox-source-docs.mozilla.org/setup/contributing_code.html
~/d/firefox % hg diff                           
~/d/firefox %
Flags: needinfo?(mhentges) → needinfo?(rjesup)

Perhaps it's related to the OS? I'm running Fedora 34.

This is still happening; I just had a patch bounce because this hid a dozen warnings

Just apply https://phabricator.services.mozilla.com/D130598 on top of m-c to see the problem now (at least until I update it to resolve these issues). I'll attach a current copy here as well

Flags: needinfo?(rjesup)
Attached patch D130598.diff — — Splinter Review
Attachment #9265500 - Attachment is obsolete: true

I've updated D130598, so use the patch attached here

I'm also on Fedora 34.
This may be easier to resolve in a Zoom call, if you'd be interested in scheduling one.

Also, can you try reproducing this if you do the following?
0. Use a clean shell without any custom environment variables set, if possible.

  1. Have a clean checkout
  2. Go to revision badee72 (current central)
  3. Edit your mozconfig to only have the following contents
export CFLAGS="-Wthread-safety"
export CXXFLAGS="-Wthread-safety"
  1. hg import --no-commit -, paste your patch, press CTRL-D
  2. ./mach build
  3. touch dom/ipc/ProcessHangMonitor.cpp
  4. ./mach build -v 2>&1 | tee output.txt
  5. Look in output.txt for a warning that doesn't describe its source file (note that this time you didn't share what warning to look for).
Flags: needinfo?(rjesup)

Closing due to a lack of response from the reporter.

Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Flags: needinfo?(rjesup)

I found this bug while typing the summary for a bug I found by code inspection, which I think explains this bug, so I'll reopen this one.

Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INCOMPLETE → ---

If for some reason the warning collector throws an exception, we ignore
it, which is fine, but we also end up not setting the message for the
BuildOutputResult, which means the line is not displayed.

It's better for the warning collector to miss a warning than for the
output to pretend the line wasn't there to begin with.

Assignee: nobody → mh+mozilla
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/autoland/rev/44badba56678 Avoid the warnings collector from eating build output. r=firefox-build-system-reviewers,ahochheiden

https://hg.mozilla.org/mozilla-central/rev/44badba56678

Status: REOPENED → RESOLVED
Closed: 3 years ago → 2 years ago
status-firefox121: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 121 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: