| Message ID | 87muiks0ur.fsf@oldenburg2.str.redhat.com |
|---|---|
| State | Committed |
| Headers |
Received: (qmail 91865 invoked by alias); 14 Jun 2019 14:22:44 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 91853 invoked by uid 89); 14 Jun 2019 14:22:44 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-19.0 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=wonders X-HELO: mx1.redhat.com From: Florian Weimer <fweimer@redhat.com> To: Szabolcs Nagy <Szabolcs.Nagy@arm.com> Cc: nd <nd@arm.com>, Zack Weinberg <zackw@panix.com>, GNU C Library <libc-alpha@sourceware.org> Subject: Re: [glibc] <sys/stat.h>: Use Linux UAPI header for statx if available and useful References: <20190612110504.66041.qmail@sourceware.org> <87ftoe7utd.fsf@oldenburg2.str.redhat.com> <CAKCAbMj2G7_1bAuE+5X0qvNGMTMN9QenzOpgJmkwC4LK_mrGzA@mail.gmail.com> <87y3266czw.fsf@oldenburg2.str.redhat.com> <c35c19da-b775-3435-d38e-702f30bc6ac3@arm.com> <87ef3y6bpv.fsf@oldenburg2.str.redhat.com> <17fed83c-e000-ccbf-9900-104fc20a6273@arm.com> <874l4u6bad.fsf@oldenburg2.str.redhat.com> <2df99c26-7a4c-02ba-a50e-7557c5581a9b@arm.com> Date: Fri, 14 Jun 2019 16:22:36 +0200 In-Reply-To: <2df99c26-7a4c-02ba-a50e-7557c5581a9b@arm.com> (Szabolcs Nagy's message of "Wed, 12 Jun 2019 16:05:43 +0000") Message-ID: <87muiks0ur.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain |
Commit Message
Florian Weimer
June 14, 2019, 2:22 p.m. UTC
* Szabolcs Nagy: > On 12/06/2019 17:03, Florian Weimer wrote: >> * Szabolcs Nagy: >> >>> On 12/06/2019 16:54, Florian Weimer wrote: >>>> Linux: Fix __glibc_has_include use for <sys/stat.h> and statx >>>> >>>> The identifier linux is used as a predefined macro, so the actually used >>>> path is 1/stat.h or 1/stat64.h. Using the quote-based version triggers >>>> a file lookup for /usr/include/bits/linux/stat.h (or whatever directory >>>> is used to store bits/statx.h), but since bits/ is pretty much reserved >>>> by glibc, this appears to be acceptable. >>>> >>>> This is related to GCC PR 80005: incorrect macro expansion of the >>>> argument of __has_include. >>>> >>>> Suggested by Zack Weinberg. >>>> >>>> 2019-06-12 Florian Weimer <fweimer@redhat.com> >>>> >>>> * sysdeps/unix/sysv/linux/bits/statx.h: Use string literal in >>>> argument to __glibc_has_include to inhibit macro expansion. >>>> >>>> diff --git a/sysdeps/unix/sysv/linux/bits/statx.h b/sysdeps/unix/sysv/linux/bits/statx.h >>>> index d36f44efc6..3599f85a47 100644 >>>> --- a/sysdeps/unix/sysv/linux/bits/statx.h >>>> +++ b/sysdeps/unix/sysv/linux/bits/statx.h >>>> @@ -23,8 +23,8 @@ >>>> #endif >>>> >>>> /* Use the Linux kernel header if available. */ >>>> -#if __glibc_has_include (<linux/stat.h>) >>>> -# include <linux/stat.h> >>>> +#if __glibc_has_include ("linux/stat.h") >>>> +# include "linux/stat.h" >>> >>> i would add a comment here with the gcc bug number as a >>> reminder in case anybody wonders about "" >> >> What about this? >> >> /* Use "" to work around incorrect macro expansion of the __has_include >> argument (GCC PR 80005). */ > > looks good. Thanks. Updated patch below. I plan to push this soon. Florian Linux: Fix __glibc_has_include use for <sys/stat.h> and statx The identifier linux is used as a predefined macro, so the actually used path is 1/stat.h or 1/stat64.h. Using the quote-based version triggers a file lookup for /usr/include/bits/linux/stat.h (or whatever directory is used to store bits/statx.h), but since bits/ is pretty much reserved by glibc, this appears to be acceptable. This is related to GCC PR 80005: incorrect macro expansion of the argument of __has_include. Suggested by Zack Weinberg. 2019-06-14 Florian Weimer <fweimer@redhat.com> * sysdeps/unix/sysv/linux/bits/statx.h: Use string literal in argument to __glibc_has_include to inhibit macro expansion.
Comments
On 6/14/19 10:22 AM, Florian Weimer wrote: > * Szabolcs Nagy: > >> On 12/06/2019 17:03, Florian Weimer wrote: >>> * Szabolcs Nagy: >>> >>>> On 12/06/2019 16:54, Florian Weimer wrote: >>>>> Linux: Fix __glibc_has_include use for <sys/stat.h> and statx >>>>> >>>>> The identifier linux is used as a predefined macro, so the actually used >>>>> path is 1/stat.h or 1/stat64.h. Using the quote-based version triggers >>>>> a file lookup for /usr/include/bits/linux/stat.h (or whatever directory >>>>> is used to store bits/statx.h), but since bits/ is pretty much reserved >>>>> by glibc, this appears to be acceptable. >>>>> >>>>> This is related to GCC PR 80005: incorrect macro expansion of the >>>>> argument of __has_include. >>>>> >>>>> Suggested by Zack Weinberg. >>>>> >>>>> 2019-06-12 Florian Weimer <fweimer@redhat.com> >>>>> >>>>> * sysdeps/unix/sysv/linux/bits/statx.h: Use string literal in >>>>> argument to __glibc_has_include to inhibit macro expansion. >>>>> >>>>> diff --git a/sysdeps/unix/sysv/linux/bits/statx.h b/sysdeps/unix/sysv/linux/bits/statx.h >>>>> index d36f44efc6..3599f85a47 100644 >>>>> --- a/sysdeps/unix/sysv/linux/bits/statx.h >>>>> +++ b/sysdeps/unix/sysv/linux/bits/statx.h >>>>> @@ -23,8 +23,8 @@ >>>>> #endif >>>>> >>>>> /* Use the Linux kernel header if available. */ >>>>> -#if __glibc_has_include (<linux/stat.h>) >>>>> -# include <linux/stat.h> >>>>> +#if __glibc_has_include ("linux/stat.h") >>>>> +# include "linux/stat.h" >>>> >>>> i would add a comment here with the gcc bug number as a >>>> reminder in case anybody wonders about "" >>> >>> What about this? >>> >>> /* Use "" to work around incorrect macro expansion of the __has_include >>> argument (GCC PR 80005). */ >> >> looks good. > > Thanks. Updated patch below. I plan to push this soon. > > Florian > > Linux: Fix __glibc_has_include use for <sys/stat.h> and statx > > The identifier linux is used as a predefined macro, so the actually > used path is 1/stat.h or 1/stat64.h. Using the quote-based version > triggers a file lookup for /usr/include/bits/linux/stat.h (or whatever > directory is used to store bits/statx.h), but since bits/ is pretty > much reserved by glibc, this appears to be acceptable. > > This is related to GCC PR 80005: incorrect macro expansion of the > argument of __has_include. > > Suggested by Zack Weinberg. > > 2019-06-14 Florian Weimer <fweimer@redhat.com> > > * sysdeps/unix/sysv/linux/bits/statx.h: Use string literal in > argument to __glibc_has_include to inhibit macro expansion. LGTM. FYI, Rich Felker tweeted about this :-) Reviewed-by: Carlos O'Donell <carlos@redhat.com> > diff --git a/sysdeps/unix/sysv/linux/bits/statx.h b/sysdeps/unix/sysv/linux/bits/statx.h > index d36f44efc6..206878723f 100644 > --- a/sysdeps/unix/sysv/linux/bits/statx.h > +++ b/sysdeps/unix/sysv/linux/bits/statx.h > @@ -23,8 +23,11 @@ > #endif > > /* Use the Linux kernel header if available. */ > -#if __glibc_has_include (<linux/stat.h>) > -# include <linux/stat.h> > + > +/* Use "" to work around incorrect macro expansion of the > + __has_include argument (GCC PR 80005). */ > +#if __glibc_has_include ("linux/stat.h") > +# include "linux/stat.h" > # ifdef STATX_TYPE > # define __statx_timestamp_defined 1 > # define __statx_defined 1 >
It's been reported that this now breaks: #define __ASSEMBLER__ 1 #include <sys/stat.h> This is because <linux/types.h> does not define __u64 under these circumstances. I don't think defining __ASSEMBLER__ is a supported scenario. Do you agree? Thanks, Florian
On Jun 25 2019, Florian Weimer <fweimer@redhat.com> wrote: > It's been reported that this now breaks: > > #define __ASSEMBLER__ 1 > #include <sys/stat.h> > > This is because <linux/types.h> does not define __u64 under these > circumstances. > > I don't think defining __ASSEMBLER__ is a supported scenario. __ASSEMBLER__ is defined by -xassembler-with-cpp, but <sys/stat.h> is not an assembler header. Andreas.
* Andreas Schwab: > On Jun 25 2019, Florian Weimer <fweimer@redhat.com> wrote: > >> It's been reported that this now breaks: >> >> #define __ASSEMBLER__ 1 >> #include <sys/stat.h> >> >> This is because <linux/types.h> does not define __u64 under these >> circumstances. >> >> I don't think defining __ASSEMBLER__ is a supported scenario. > > __ASSEMBLER__ is defined by -xassembler-with-cpp, but <sys/stat.h> is > not an assembler header. Sorry for being unclear. Someone is using __ASSEMBLER__ to inhibit type definitions in kernel headers. There is a literal “#define __ASSEMBLER__ 1” in the source code. I assume this is to get access to kernel header constants while avoid conflicts with glibc headers. This is not about an .S file include <sys/stat.h>; it's fairly obvious that this wouldn't make sense. Thanks, Florian
On Fri, Jun 14, 2019 at 7:27 AM Carlos O'Donell <carlos@redhat.com> wrote: > > On 6/14/19 10:22 AM, Florian Weimer wrote: > > * Szabolcs Nagy: > > > >> On 12/06/2019 17:03, Florian Weimer wrote: > >>> * Szabolcs Nagy: > >>> > >>>> On 12/06/2019 16:54, Florian Weimer wrote: > >>>>> Linux: Fix __glibc_has_include use for <sys/stat.h> and statx > >>>>> > >>>>> The identifier linux is used as a predefined macro, so the actually used > >>>>> path is 1/stat.h or 1/stat64.h. Using the quote-based version triggers > >>>>> a file lookup for /usr/include/bits/linux/stat.h (or whatever directory > >>>>> is used to store bits/statx.h), but since bits/ is pretty much reserved > >>>>> by glibc, this appears to be acceptable. > >>>>> > >>>>> This is related to GCC PR 80005: incorrect macro expansion of the > >>>>> argument of __has_include. > >>>>> > >>>>> Suggested by Zack Weinberg. > >>>>> > >>>>> 2019-06-12 Florian Weimer <fweimer@redhat.com> > >>>>> > >>>>> * sysdeps/unix/sysv/linux/bits/statx.h: Use string literal in > >>>>> argument to __glibc_has_include to inhibit macro expansion. > >>>>> > >>>>> diff --git a/sysdeps/unix/sysv/linux/bits/statx.h b/sysdeps/unix/sysv/linux/bits/statx.h > >>>>> index d36f44efc6..3599f85a47 100644 > >>>>> --- a/sysdeps/unix/sysv/linux/bits/statx.h > >>>>> +++ b/sysdeps/unix/sysv/linux/bits/statx.h > >>>>> @@ -23,8 +23,8 @@ > >>>>> #endif > >>>>> > >>>>> /* Use the Linux kernel header if available. */ > >>>>> -#if __glibc_has_include (<linux/stat.h>) > >>>>> -# include <linux/stat.h> > >>>>> +#if __glibc_has_include ("linux/stat.h") > >>>>> +# include "linux/stat.h" > >>>> > >>>> i would add a comment here with the gcc bug number as a > >>>> reminder in case anybody wonders about "" > >>> > >>> What about this? > >>> > >>> /* Use "" to work around incorrect macro expansion of the __has_include > >>> argument (GCC PR 80005). */ > >> > >> looks good. > > > > Thanks. Updated patch below. I plan to push this soon. > > > > Florian > > > > Linux: Fix __glibc_has_include use for <sys/stat.h> and statx > > > > The identifier linux is used as a predefined macro, so the actually > > used path is 1/stat.h or 1/stat64.h. Using the quote-based version > > triggers a file lookup for /usr/include/bits/linux/stat.h (or whatever > > directory is used to store bits/statx.h), but since bits/ is pretty > > much reserved by glibc, this appears to be acceptable. > > > > This is related to GCC PR 80005: incorrect macro expansion of the > > argument of __has_include. > > > > Suggested by Zack Weinberg. > > > > 2019-06-14 Florian Weimer <fweimer@redhat.com> > > > > * sysdeps/unix/sysv/linux/bits/statx.h: Use string literal in > > argument to __glibc_has_include to inhibit macro expansion. > > LGTM. FYI, Rich Felker tweeted about this :-) > > Reviewed-by: Carlos O'Donell <carlos@redhat.com> > > > diff --git a/sysdeps/unix/sysv/linux/bits/statx.h b/sysdeps/unix/sysv/linux/bits/statx.h > > index d36f44efc6..206878723f 100644 > > --- a/sysdeps/unix/sysv/linux/bits/statx.h > > +++ b/sysdeps/unix/sysv/linux/bits/statx.h > > @@ -23,8 +23,11 @@ > > #endif > > > > /* Use the Linux kernel header if available. */ > > -#if __glibc_has_include (<linux/stat.h>) > > -# include <linux/stat.h> > > + > > +/* Use "" to work around incorrect macro expansion of the > > + __has_include argument (GCC PR 80005). */ > > +#if __glibc_has_include ("linux/stat.h") > > +# include "linux/stat.h" ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This may have caused: https://sourceware.org/bugzilla/show_bug.cgi?id=24752 > > # ifdef STATX_TYPE > > # define __statx_timestamp_defined 1 > > # define __statx_defined 1 > > > > > -- > Cheers, > Carlos.
* H. J. Lu: >> > diff --git a/sysdeps/unix/sysv/linux/bits/statx.h b/sysdeps/unix/sysv/linux/bits/statx.h >> > index d36f44efc6..206878723f 100644 >> > --- a/sysdeps/unix/sysv/linux/bits/statx.h >> > +++ b/sysdeps/unix/sysv/linux/bits/statx.h >> > @@ -23,8 +23,11 @@ >> > #endif >> > >> > /* Use the Linux kernel header if available. */ >> > -#if __glibc_has_include (<linux/stat.h>) >> > -# include <linux/stat.h> >> > + >> > +/* Use "" to work around incorrect macro expansion of the >> > + __has_include argument (GCC PR 80005). */ >> > +#if __glibc_has_include ("linux/stat.h") >> > +# include "linux/stat.h" > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > This may have caused: > > https://sourceware.org/bugzilla/show_bug.cgi?id=24752 This has also been reported as bug 24532, which predates the commit. Thanks, Florian
On Jun 14 2019, Florian Weimer <fweimer@redhat.com> wrote: > diff --git a/sysdeps/unix/sysv/linux/bits/statx.h b/sysdeps/unix/sysv/linux/bits/statx.h > index d36f44efc6..206878723f 100644 > --- a/sysdeps/unix/sysv/linux/bits/statx.h > +++ b/sysdeps/unix/sysv/linux/bits/statx.h > @@ -23,8 +23,11 @@ > #endif > > /* Use the Linux kernel header if available. */ > -#if __glibc_has_include (<linux/stat.h>) > -# include <linux/stat.h> > + > +/* Use "" to work around incorrect macro expansion of the > + __has_include argument (GCC PR 80005). */ > +#if __glibc_has_include ("linux/stat.h") FWIW, this gets mangled by fixincludes: @@ -26,7 +35,7 @@ /* Use "" to work around incorrect macro expansion of the __has_include argument (GCC PR 80005). */ -#if __glibc_has_include ("linux/stat.h") +#if __glibc_has_include ("__linux__/stat.h") # include "linux/stat.h" # ifdef STATX_TYPE # define __statx_timestamp_defined 1 Andreas.
* Andreas Schwab: > On Jun 14 2019, Florian Weimer <fweimer@redhat.com> wrote: > >> diff --git a/sysdeps/unix/sysv/linux/bits/statx.h b/sysdeps/unix/sysv/linux/bits/statx.h >> index d36f44efc6..206878723f 100644 >> --- a/sysdeps/unix/sysv/linux/bits/statx.h >> +++ b/sysdeps/unix/sysv/linux/bits/statx.h >> @@ -23,8 +23,11 @@ >> #endif >> >> /* Use the Linux kernel header if available. */ >> -#if __glibc_has_include (<linux/stat.h>) >> -# include <linux/stat.h> >> + >> +/* Use "" to work around incorrect macro expansion of the >> + __has_include argument (GCC PR 80005). */ >> +#if __glibc_has_include ("linux/stat.h") > > FWIW, this gets mangled by fixincludes: > > @@ -26,7 +35,7 @@ > > /* Use "" to work around incorrect macro expansion of the > __has_include argument (GCC PR 80005). */ > -#if __glibc_has_include ("linux/stat.h") > +#if __glibc_has_include ("__linux__/stat.h") > # include "linux/stat.h" > # ifdef STATX_TYPE > # define __statx_timestamp_defined 1 Yuck. Do you agree that fixincludes must be fixed? Thanks, Florian
On Jul 04 2019, Florian Weimer <fweimer@redhat.com> wrote:
> Yuck. Do you agree that fixincludes must be fixed?
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91085
Andreas.
* Andreas Schwab: > On Jul 04 2019, Florian Weimer <fweimer@redhat.com> wrote: > >> Yuck. Do you agree that fixincludes must be fixed? > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91085 Thanks, I added: <https://sourceware.org/glibc/wiki/Release/2.30#GCC.27s_fixincludes_tool_may_break_.3Cbits.2BAC8-statx.h.3E> Florian
diff --git a/sysdeps/unix/sysv/linux/bits/statx.h b/sysdeps/unix/sysv/linux/bits/statx.h index d36f44efc6..206878723f 100644 --- a/sysdeps/unix/sysv/linux/bits/statx.h +++ b/sysdeps/unix/sysv/linux/bits/statx.h @@ -23,8 +23,11 @@ #endif /* Use the Linux kernel header if available. */ -#if __glibc_has_include (<linux/stat.h>) -# include <linux/stat.h> + +/* Use "" to work around incorrect macro expansion of the + __has_include argument (GCC PR 80005). */ +#if __glibc_has_include ("linux/stat.h") +# include "linux/stat.h" # ifdef STATX_TYPE # define __statx_timestamp_defined 1 # define __statx_defined 1