Closed Bug 1332167 Opened 3 years ago Closed 3 years ago

xpcom/io/nsLocalFileUnix.cpp doesn't build with warnings-as-errors

Categories

(Core :: XPCOM, defect)

45 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jesup, Assigned: jesup)

Details

Attachments

(1 file)

xpcom/io/nsLocalFileUnix.cpp won't build if warnings-as-errors is on, because the GetDeviceName function takes "int, int, nsACString&".  It is called later using "major(...)" and "minor(...)", which are "unsigned int", not int, so it doesn't see the reference to the static local function, thus a warning/error.

Simple solution: change the inputs to unsigned int.
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
How does the file compile with warnings-as-errors in automation, then?  Is this a newer/different compiler?  The calls to major() and minor() depend on the type of st_dev, dev_t:

http://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileUnix.cpp#1381

which has been unsigned on Linux for...a long time, I suspect.  POSIX makes no reference to the signedness of dev_t:

http://pubs.opengroup.org/onlinepubs/009604499/basedefs/sys/types.h.html

so I'm not sure the fix is correct in general.  The fix probably would be correct for our tier-1 platforms, though I'm not sure offhand how Mac defines dev_t here.
Flags: needinfo?(rjesup)
I don't know how it compiles in automation; yesterday it started failing for me after a pull (I don't think there were any system updates on my Fedora 22).  Failure was function-defined-but-not-used (as it's a static).  

Note that dev_t is an input to major() and minor(), not an output.

While I found one legacy Unix that had major() and minor() return int (HPUX circa 2007), inherently they almost have to be some type of unsigned value given how device IDs work.

And %d:%d for the deviceNum if either is negative would be ... a problem.  So instead of casting all to (int) before calling GetDeviceName(), I think this is better - if you're worried about major() or minor() returning int, then we cast them to unsigned int before calling GetDeviceName.  Cool?
Flags: needinfo?(rjesup)
(In reply to Randell Jesup [:jesup] from comment #3)
> I don't know how it compiles in automation; yesterday it started failing for
> me after a pull (I don't think there were any system updates on my Fedora
> 22).  Failure was function-defined-but-not-used (as it's a static).

That sounds a little bit more different than a simple type mismatch.  What does the preprocessed source look like, in particular nsLocalFile::GetDiskSpaceAvailable?
Flags: needinfo?(rjesup)
(In reply to Nathan Froyd [:froydnj] from comment #4)
> (In reply to Randell Jesup [:jesup] from comment #3)
> > I don't know how it compiles in automation; yesterday it started failing for
> > me after a pull (I don't think there were any system updates on my Fedora
> > 22).  Failure was function-defined-but-not-used (as it's a static).
> 
> That sounds a little bit more different than a simple type mismatch.  What
> does the preprocessed source look like, in particular
> nsLocalFile::GetDiskSpaceAvailable?

It sounded like GetDeviceName(unsigned, unsigned, nsACString&) was matching "something else" (not sure what), making the static one unused.  I tried updating from inbound (no change).  I checked #defines since that code is littered with them, and verified the call to it was in fact being compiled (syntax-errored the code there to ensure it was compiled, it was).  Then I tried a simple cast to (int) for major() and minor(); that worked.  Later to put the patch up I revised the function instead of casting the call.

Just tried now without the patch - and it compiles. (Don't know everything I did inbetween; I may have clobbered, since I was mucking with build infrastructure stuff for the VP9 patches when I hit this (old-configure.in/gyp.mozbuild, though nothing anywhere near stuff that affects this.) I'm clobbering and trying again (which was ok), and I'm trying one temporary tweak I made to with-system-libvpx while working on those patches.

Weird.
Flags: needinfo?(rjesup)
It works with the one line added (to set MOZ_LIBVPX_CFLAGS to -Isomething) i was playing with, no surprise.  Poking at it some more... Sounds like some sort of compiler weirdness.  Maybe even weird choices for unified builds?

All that said: int is a bad choice here to begin with; negatives in deviceNum ("-1:2") would be pretty bad/useless.
If it happens again.... I'll dig out any details.  My bet now is on unified builds.  But I can't see how that would break it either....

I think the patch above (with, if you want, casts on the call to GetDeviceName to unsigned) still makes sense.  But up to you.
Comment on attachment 8828345 [details] [diff] [review]
Unix major() and minor() return unsigned

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

Yeah, I think you're right.
Attachment #8828345 - Flags: review?(nfroyd) → review+
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25e81b94c153
Unix major() and minor() return unsigned r=froydnj
https://hg.mozilla.org/mozilla-central/rev/25e81b94c153
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.