Closed Bug 1406687 Opened 2 years ago Closed 2 years ago

Resolve warnings exposed by FORTIFY_SOURCE

Categories

(Core :: General, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

FORTIFY_SOURCE enables some warnings we previously didn't have, so we will need to resolve them before we can land this compiler flag.
Comment on attachment 8916360 [details]
Bug 1406687 Pass return values from fwrite to Unused to silence the warn-unused-result warning

https://reviewboard.mozilla.org/r/187530/#review192588

r=me with the comments below addressed.

::: media/webrtc/signaling/gtest/mediaconduit_unittests.cpp:230
(Diff revision 1)
>     WriteWaveHeader(PLAYOUT_SAMPLE_FREQUENCY, 1, inFile);
>     GenerateMusic(inbuf, SAMPLES);
> -   fwrite(inbuf,1,SAMPLES*sizeof(inbuf[0])*CHANNELS,inFile);
> +   size_t written = fwrite(inbuf,1,SAMPLES*sizeof(inbuf[0])*CHANNELS,inFile);
> +   if(written != SAMPLES*sizeof(inbuf[0])*CHANNELS) {
> +     cerr << "Couldn't write entire buffer " << endl;
> +   }

Please add appropriate whitespace on the modified line and the added lines: after commas, around operators like '*', and after `if`.

Also, why are you adding a console message here when you are ignoring the result in most other places? This is test code, I don't think the message is useful and I suggest removing it.

::: testing/gtest/gtest/src/gtest.cc:3872
(Diff revision 1)
>        // don't want to fail the test because of this.
>        FILE* pfile = posix::FOpen(premature_exit_filepath, "w");
> -      fwrite("0", 1, 1, pfile);
> +      int written = fwrite("0", 1, 1, pfile);
> +      if(written != 1) {
> +        printf("WARNING: did not write byte.\n");
> +      }

Again: space after `if`, and again, I don't think this message needs to be printed.
Attachment #8916360 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #2)
> Comment on attachment 8916360 [details]
> Bug 1406687 Pass return values from fwrite to Unused to silence the
> warn-unused-result warning
> 
> https://reviewboard.mozilla.org/r/187530/#review192588
> 
> r=me with the comments below addressed.
> 
> ::: media/webrtc/signaling/gtest/mediaconduit_unittests.cpp:230
> (Diff revision 1)
> >     WriteWaveHeader(PLAYOUT_SAMPLE_FREQUENCY, 1, inFile);
> >     GenerateMusic(inbuf, SAMPLES);
> > -   fwrite(inbuf,1,SAMPLES*sizeof(inbuf[0])*CHANNELS,inFile);
> > +   size_t written = fwrite(inbuf,1,SAMPLES*sizeof(inbuf[0])*CHANNELS,inFile);
> > +   if(written != SAMPLES*sizeof(inbuf[0])*CHANNELS) {
> > +     cerr << "Couldn't write entire buffer " << endl;
> > +   }
> 
> Please add appropriate whitespace on the modified line and the added lines:
> after commas, around operators like '*', and after `if`.
> 
> Also, why are you adding a console message here when you are ignoring the
> result in most other places? This is test code, I don't think the message is
> useful and I suggest removing it.

My initial attempt at adding Unused.h to each of these two fails did not succeed, so I copied whatever error-handling technique I found in the file. If you want I can try harder to make Unused.h work. (And if I assigned the variable but didn't use it, I got an unused variable warning that also broke the build.)
Flags: needinfo?(n.nethercote)
> My initial attempt at adding Unused.h to each of these two fails did not
> succeed

In what way did it fail?

> (And if I assigned the
> variable but didn't use it, I got an unused variable warning that also broke
> the build.)

I have seen this work for GCC, it might also work for MSVC:

> int dummy = fwrite(...);
> (void)dummy;
Flags: needinfo?(n.nethercote)
(In reply to Nicholas Nethercote [:njn] from comment #4)
> > My initial attempt at adding Unused.h to each of these two fails did not
> > succeed
> 
> In what way did it fail?

You're right, fiddling with a bit more got it to work. I think it's set now.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0317bcff40bc
Pass return values from fwrite to Unused to silence the warn-unused-result warning r=njn
Keywords: checkin-needed
Backed out for build bustage at testing/gtest/gtest/src/gtest.cc:3871: 'Unused' was not declared in this scope:

https://hg.mozilla.org/integration/autoland/rev/ffccd2307ac197fab5dc4ea1876d57b0cf8544fe

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0317bcff40bc7f1e73422c4b6f8f22bd73b981ba&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=135727488&repo=autoland

 /builds/worker/workspace/build/src/testing/gtest/gtest/src/gtest.cc:3871:7: error: 'Unused' was not declared in this scope
gmake[5]: *** [gtest-all.o] Error 1
gmake[4]: *** [testing/gtest/target] Error 2
gmake[4]: *** Waiting for unfinished jobs....
/builds/worker/workspace/build/src/media/webrtc/signaling/gtest/mediaconduit_unittests.cpp:228:4: error: 'Unused' was not declared in this scope
Flags: needinfo?(tom)
I think gtests don't get built with a normal build (e.g. `./mach build`). If you run `./mach gtest` it'll build and run the gtests.
Bug looks to be pretty straightforward: MinGW doesn't need the mozilla:: namespace... everything else does.

Here's a try run fixing it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18c25cfd2c6c9b5028ff88c2c99adcd13dec6319

I'll mark it as checkin-needed tomorrow morning instead of right before leaving for the day though.
Status: NEW → ASSIGNED
Priority: -- → P1
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4cce79b99e13
Pass return values from fwrite to Unused to silence the warn-unused-result warning r=njn
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4cce79b99e13
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
I just got notified that this got backed out for a build bustage?
https://phabricator.services.mozilla.com/rNSS7a5d74db770b

That's fine if so, but why did it show up a year later?
Flags: needinfo?(aryx.bugmail)

I believe this was a typo.

Flags: needinfo?(aryx.bugmail)
You need to log in before you can comment on or make changes to this bug.