Closed Bug 1329798 Opened 3 years ago Closed 3 years ago
Use of major & minor macros without including sys/sysmacros
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.
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 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.
Attachment #8837466 - Flags: review?(mh+mozilla) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/b9a1da5e4f89 Include sys/sysmacros.h for major(), minor() on Linux. r=glandium
[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.
Michal, Mike, want to request uplift to aurora/beta for this one?
Assignee: nobody → mgorny
(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 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 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.
You need to log in before you can comment on or make changes to this bug.