diff mbox series

alpha: Remove anonymous union in struct stat [BZ #27042]

Message ID 20201209162008.2064091-1-mattst88@gmail.com
State Superseded
Headers
Series alpha: Remove anonymous union in struct stat [BZ #27042] |

Commit Message

Matt Turner Dec. 9, 2020, 4:20 p.m. UTC 
  This is clever, but it confuses downstream detection in at least zstd
and GNOME's glib. zstd has preprocessor tests for the 'st_mtime' macro,
which is not provided by the path using the anonymous union; glib checks
for the presence of 'st_mtimensec' in struct stat but then tries to
access that field in struct statx (which might be a bug on its own).
---
 .../unix/sysv/linux/alpha/bits/struct_stat.h  | 23 ++++---------------
 1 file changed, 5 insertions(+), 18 deletions(-)

Comments

Florian Weimer Dec. 9, 2020, 5:11 p.m. UTC | #1 
* Matt Turner via Libc-alpha:

> This is clever, but it confuses downstream detection in at least zstd
> and GNOME's glib. zstd has preprocessor tests for the 'st_mtime' macro,
> which is not provided by the path using the anonymous union; glib checks
> for the presence of 'st_mtimensec' in struct stat but then tries to
> access that field in struct statx (which might be a bug on its own).

A more conservative approach would be adding

#define st_atime st_atime
#define st_ctime st_ctime
#define st_mtime st_mtime

to the anonymous union case.

Thanks,
Florian
Matt Turner Dec. 10, 2020, 1:25 a.m. UTC | #2 
On Wed, Dec 9, 2020 at 12:11 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Matt Turner via Libc-alpha:
>
> > This is clever, but it confuses downstream detection in at least zstd
> > and GNOME's glib. zstd has preprocessor tests for the 'st_mtime' macro,
> > which is not provided by the path using the anonymous union; glib checks
> > for the presence of 'st_mtimensec' in struct stat but then tries to
> > access that field in struct statx (which might be a bug on its own).
>
> A more conservative approach would be adding
>
> #define st_atime st_atime
> #define st_ctime st_ctime
> #define st_mtime st_mtime
>
> to the anonymous union case.

Thanks. That works for zstd, but not for glib—which might be a bug in
glib, like I mentioned. I'll investigate that independently.

Andreas immediately closed the bug I filed with the message

> The existence of the st_?time macros is an implementation detail.

and I have no doubt that he's correct, but it appears that all or
basically all of the architectures' headers provide these macros so
I'd prefer to bring alpha in line rather than convince multiple
projects that their code is technically wrong in a way that only
manifests on an architecture they've never heard of.

v2 sent that deals with the kernel_stat.h interactions.
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/alpha/bits/struct_stat.h b/sysdeps/unix/sysv/linux/alpha/bits/struct_stat.h
index 1c9b4248b8..dddc5fb5a0 100644
--- a/sysdeps/unix/sysv/linux/alpha/bits/struct_stat.h
+++ b/sysdeps/unix/sysv/linux/alpha/bits/struct_stat.h
@@ -27,26 +27,13 @@ 
    'struct timespec'.  This is the type used whenever possible but the
    Unix namespace rules do not allow the identifier 'timespec' to appear
    in the <sys/stat.h> header.  Therefore we have to handle the use of
-   this header in strictly standard-compliant sources special.
-
-   Use neat tidy anonymous unions and structures when possible.  */
+   this header in strictly standard-compliant sources special.  */
 
 #ifdef __USE_XOPEN2K8
-# if __GNUC_PREREQ(3,3)
-#  define __ST_TIME(X)				\
-	__extension__ union {			\
-	    struct timespec st_##X##tim;	\
-	    struct {				\
-		__time_t st_##X##time;		\
-		unsigned long st_##X##timensec;	\
-	    };					\
-	}
-# else
-#  define __ST_TIME(X) struct timespec st_##X##tim
-#  define st_atime st_atim.tv_sec
-#  define st_mtime st_mtim.tv_sec
-#  define st_ctime st_ctim.tv_sec
-# endif
+# define __ST_TIME(X) struct timespec st_##X##tim
+# define st_atime st_atim.tv_sec
+# define st_mtime st_mtim.tv_sec
+# define st_ctime st_ctim.tv_sec
 #else
 # define __ST_TIME(X)				\
 	__time_t st_##X##time;			\