time: Define _STRUCT_TIMESPEC in <bits/types/struct_timespec.h>

Message ID 20180621064654.2D7DA4289B0D0@oldenburg.str.redhat.com
State New
Headers show
Series
  • time: Define _STRUCT_TIMESPEC in <bits/types/struct_timespec.h>
Related show

Commit Message

Florian Weimer June 21, 2018, 6:46 a.m.
After commit d76d3703551a362b472c866b5b6089f66f8daa8e ("Fix missing
timespec definition for sys/stat.h (BZ #21371)"), sanitizer builds start
to fail due to a conflicting definition of struct timespec in
<linux/time.h>.  Define _STRUCT_TIMESPEC, which is already checked in
the kernel header, to support including <linux/time.h> after
<sys/stat.h>.

2018-06-21  Florian Weimer  <fweimer@redhat.com>

	* time/bits/types/struct_timespec.h (_STRUCT_TIMESPEC): Define.

Comments

Zack Weinberg June 21, 2018, 11 a.m. | #1
On Thu, Jun 21, 2018 at 2:46 AM, Florian Weimer <fweimer@redhat.com> wrote:
> After commit d76d3703551a362b472c866b5b6089f66f8daa8e ("Fix missing

> timespec definition for sys/stat.h (BZ #21371)"), sanitizer builds start

> to fail due to a conflicting definition of struct timespec in

> <linux/time.h>.  Define _STRUCT_TIMESPEC, which is already checked in

> the kernel header, to support including <linux/time.h> after

> <sys/stat.h>.


Should it go the other way around as well?  That is, if
_STRUCT_TIMESPEC is already defined, should we suppress our
definition?

Either way I think there should be a comment saying that linux/time.h
checks this macro.

zw
Florian Weimer June 21, 2018, 11:24 a.m. | #2
On 06/21/2018 01:00 PM, Zack Weinberg wrote:
> On Thu, Jun 21, 2018 at 2:46 AM, Florian Weimer <fweimer@redhat.com> wrote:

>> After commit d76d3703551a362b472c866b5b6089f66f8daa8e ("Fix missing

>> timespec definition for sys/stat.h (BZ #21371)"), sanitizer builds start

>> to fail due to a conflicting definition of struct timespec in

>> <linux/time.h>.  Define _STRUCT_TIMESPEC, which is already checked in

>> the kernel header, to support including <linux/time.h> after

>> <sys/stat.h>.

> 

> Should it go the other way around as well?  That is, if

> _STRUCT_TIMESPEC is already defined, should we suppress our

> definition?


Hmm, sure, that would be possible.

> Either way I think there should be a comment saying that linux/time.h

> checks this macro.


It's in generic code, so I wasn't sure if it was okay to refer to 
<linux/time.h>.  But I can certainly add that.

What about the attached patch?

Thanks,
Florian
Subject: [PATCH] time: Define _STRUCT_TIMESPEC in <bits/types/struct_timespec.h>
To: libc-alpha@sourceware.org

After commit d76d3703551a362b472c866b5b6089f66f8daa8e ("Fix missing
timespec definition for sys/stat.h (BZ #21371)"), sanitizer builds
start to fail due to a conflicting definition of struct timespec in
<linux/time.h>.  Use _STRUCT_TIMESPEC as the header file inclusion
guard, which is already checked in the kernel header, to support
including <linux/time.h> and <sys/stat.h> in the same translation
unit.

2018-06-21  Florian Weimer  <fweimer@redhat.com>

	* time/bits/types/struct_timespec.h (_STRUCT_TIMESPEC): Define.

diff --git a/time/bits/types/struct_timespec.h b/time/bits/types/struct_timespec.h
index 644db9fdb6..5b77c52b4f 100644
--- a/time/bits/types/struct_timespec.h
+++ b/time/bits/types/struct_timespec.h
@@ -1,5 +1,6 @@
-#ifndef __timespec_defined
-#define __timespec_defined 1
+/* NB: Include guard matches what <linux/time.h> uses.  */
+#ifndef _STRUCT_TIMESPEC
+#define _STRUCT_TIMESPEC 1
 
 #include <bits/types.h>
Florian Weimer June 26, 2018, 5:03 p.m. | #3
On 06/21/2018 01:24 PM, Florian Weimer wrote:
> 2018-06-21  Florian Weimer<fweimer@redhat.com>

> 

> 	* time/bits/types/struct_timespec.h (_STRUCT_TIMESPEC): Define.

> 

> diff --git a/time/bits/types/struct_timespec.h b/time/bits/types/struct_timespec.h

> index 644db9fdb6..5b77c52b4f 100644

> --- a/time/bits/types/struct_timespec.h

> +++ b/time/bits/types/struct_timespec.h

> @@ -1,5 +1,6 @@

> -#ifndef __timespec_defined

> -#define __timespec_defined 1

> +/* NB: Include guard matches what <linux/time.h> uses.  */

> +#ifndef _STRUCT_TIMESPEC

> +#define _STRUCT_TIMESPEC 1

>   

>   #include <bits/types.h>


Ping?

Thanks,
Florian
Florian Weimer June 28, 2018, 8:27 a.m. | #4
On 06/21/2018 01:24 PM, Florian Weimer wrote:
> diff --git a/time/bits/types/struct_timespec.h b/time/bits/types/struct_timespec.h

> index 644db9fdb6..5b77c52b4f 100644

> --- a/time/bits/types/struct_timespec.h

> +++ b/time/bits/types/struct_timespec.h

> @@ -1,5 +1,6 @@

> -#ifndef __timespec_defined

> -#define __timespec_defined 1

> +/* NB: Include guard matches what <linux/time.h> uses.  */

> +#ifndef _STRUCT_TIMESPEC

> +#define _STRUCT_TIMESPEC 1


Are there any objections to this change?  It's required to fix a GCC 
build failure, so I'm going to commit this soon (probably on Friday).

Thanks,
Florian
Dmitry V. Levin June 28, 2018, 9:52 a.m. | #5
On Thu, Jun 28, 2018 at 10:27:53AM +0200, Florian Weimer wrote:
> On 06/21/2018 01:24 PM, Florian Weimer wrote:

> > diff --git a/time/bits/types/struct_timespec.h b/time/bits/types/struct_timespec.h

> > index 644db9fdb6..5b77c52b4f 100644

> > --- a/time/bits/types/struct_timespec.h

> > +++ b/time/bits/types/struct_timespec.h

> > @@ -1,5 +1,6 @@

> > -#ifndef __timespec_defined

> > -#define __timespec_defined 1

> > +/* NB: Include guard matches what <linux/time.h> uses.  */

> > +#ifndef _STRUCT_TIMESPEC

> > +#define _STRUCT_TIMESPEC 1

> 

> Are there any objections to this change?  It's required to fix a GCC 

> build failure, so I'm going to commit this soon (probably on Friday).


The change looks fine, thanks.


-- 
ldv
Florian Weimer June 28, 2018, 10:52 a.m. | #6
On 06/28/2018 11:52 AM, Dmitry V. Levin wrote:
> On Thu, Jun 28, 2018 at 10:27:53AM +0200, Florian Weimer wrote:

>> On 06/21/2018 01:24 PM, Florian Weimer wrote:

>>> diff --git a/time/bits/types/struct_timespec.h b/time/bits/types/struct_timespec.h

>>> index 644db9fdb6..5b77c52b4f 100644

>>> --- a/time/bits/types/struct_timespec.h

>>> +++ b/time/bits/types/struct_timespec.h

>>> @@ -1,5 +1,6 @@

>>> -#ifndef __timespec_defined

>>> -#define __timespec_defined 1

>>> +/* NB: Include guard matches what <linux/time.h> uses.  */

>>> +#ifndef _STRUCT_TIMESPEC

>>> +#define _STRUCT_TIMESPEC 1

>>

>> Are there any objections to this change?  It's required to fix a GCC

>> build failure, so I'm going to commit this soon (probably on Friday).

> 

> The change looks fine, thanks.


Thanks.  I filed bug 23349 to track this and will reference it in the 
commit.

Florian

Patch

diff --git a/time/bits/types/struct_timespec.h b/time/bits/types/struct_timespec.h
index 644db9fdb6..bde7e2826d 100644
--- a/time/bits/types/struct_timespec.h
+++ b/time/bits/types/struct_timespec.h
@@ -1,6 +1,10 @@ 
 #ifndef __timespec_defined
 #define __timespec_defined 1
 
+#ifndef _STRUCT_TIMESPEC
+# define _STRUCT_TIMESPEC 1
+#endif
+
 #include <bits/types.h>
 
 /* POSIX.1b structure for a time value.  This is like a `struct timeval' but