1329798 - Use of major & minor macros without including sys/sysmacros.h

Status: RESOLVED FIXED

Description

[Anthony Ryan]

User Agent: 
Build ID: 20161228001411

Steps to reproduce:

An upcoming version of glibc intends to stop including sys/sysmacros.h automatically with sys/type.h and require that it be included explicitly for the use of the major, minor & makedev macros.

Compiling firefox against a version of glibc with this implicit inclusion removed causes a compiler error.


Actual results:

Compiler error: 
/var/tmp/portage/www-client/firefox-50.1.0-r1/work/firefox-50.1.0/xpcom/io/nsLocalFileUnix.cpp:1380:46: error: 'major' was not declared in this scope
   if (!GetDeviceName(major(mCachedStat.st_dev),
                                              ^
/var/tmp/portage/www-client/firefox-50.1.0-r1/work/firefox-50.1.0/xpcom/io/nsLocalFileUnix.cpp:1381:46: error: 'minor' was not declared in this scope
                      minor(mCachedStat.st_dev),
                                              ^



Expected results:

The file should

#include <sys/sysmacros.h>

This will also improve compatibility with alternative libc implementations.

Comment 1

[Michał Górny]

Created attachment 8832788 [details] [diff] [review]
0001-Include-sys-sysmacros.h-for-major-minor-when-availab.patch

Here's a patch adding the #include conditionally to a configure check, and adding the file to system-includes. I have confirmed that it fixes the build on Linux with patched glibc.

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8832788 [details] [diff] [review]
0001-Include-sys-sysmacros.h-for-major-minor-when-availab.patch

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

::: xpcom/io/nsLocalFileUnix.cpp
@@ +30,5 @@
>  #define BLOCK_SIZE 1024 /* kernel block size */
>  #endif
>  #endif
> +#if defined(HAVE_SYS_SYSMACROS_H)
> +#include <sys/sysmacros.h>

The macros major and minor are only used in portions of the code that require linux/quota.h. Which pretty much assumes a Linux target system.

Looking at the most popular non-glibc Linux libcs (bionic, musl, uclibc), they all provide sys/sysmacros.h. So let's just not check it exists. We can pretty much assumes it does.

Just place the include in the same #if as the sys/quota.h include.

Comment 3

[Michał Górny]

Created attachment 8837466 [details] [diff] [review]
0001-Include-sys-sysmacros.h-for-major-minor-on-Linux.patch

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9a1da5e4f89
Include sys/sysmacros.h for major(), minor() on Linux. r=glandium

Comment 5

[Jory A. Pratt]

[Tracking Requested - why for this release]:

As many distros are already moving to glibc-2.24 support would be nice to get this landed to be a part of esr and aurora, there are no downsides to uplifting to beta and aurora at this point.

Comment 6

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/b9a1da5e4f89

Comment 7

[Julien Cristau [:jcristau]]

Michal, Mike, want to request uplift to aurora/beta for this one?

Comment 8

[Michał Górny]

(In reply to Julien Cristau [:jcristau] from comment #7)
> Michal, Mike, want to request uplift to aurora/beta for this one?

Yes, please uplift it. It would certainly be helpful to have this fix around before we unmask new glibc on our users.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8837466 [details] [diff] [review]
0001-Include-sys-sysmacros.h-for-major-minor-on-Linux.patch

Approval Request Comment
[Feature/Bug causing the regression]: Header changes in newer glibc
[User impact if declined]: Build failure against newer glibc (recent distros)
[Is this code covered by automated tests?]: N/A
[Has the fix been verified in Nightly?]: N/A
[Needs manual test from QE? If yes, steps to reproduce]: N/A
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No
[Why is the change risky/not risky?]: Adds a #include of a header that has existed since the dawn of times, but happened to be #included indirectly, which is what ceases to happen with newer glibc.
[String changes made/needed]: N/A

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8837466 [details] [diff] [review]
0001-Include-sys-sysmacros.h-for-major-minor-on-Linux.patch

build fix for new glibc, aurora53+, beta52+

Let's get this in ESR52 so linux distros don't need to patch it in.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d4e8a7b1832d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5326967d9271

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/5326967d9271