diff mbox

<sys/stat.h>: Use Linux UAPI header for statx if available and useful

Message ID 87muiks0ur.fsf@oldenburg2.str.redhat.com
State Committed
Headers show

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

Carlos O'Donell June 14, 2019, 2:27 p.m. UTC | #1
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
>
Florian Weimer June 25, 2019, 5:24 p.m. UTC | #2
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
Andreas Schwab June 25, 2019, 5:46 p.m. UTC | #3
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.
Florian Weimer June 25, 2019, 5:49 p.m. UTC | #4
* 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
H.J. Lu July 1, 2019, 8:56 p.m. UTC | #5
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.
Florian Weimer July 2, 2019, 6:54 a.m. UTC | #6
* 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
Andreas Schwab July 4, 2019, 10:09 a.m. UTC | #7
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.
Florian Weimer July 4, 2019, 10:10 a.m. UTC | #8
* 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
Andreas Schwab July 4, 2019, 10:17 a.m. UTC | #9
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.
Florian Weimer July 4, 2019, 12:48 p.m. UTC | #10
* 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 mbox

Patch

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