Patchwork Fix -Os gnu_dev_* linknamespace, localplt issues (bug 15105, bug 19463)

login
register
mail settings
Submitter Joseph Myers
Date Feb. 2, 2018, 5:43 p.m.
Message ID <alpine.DEB.2.20.1802021743260.17793@digraph.polyomino.org.uk>
Download mbox | patch
Permalink /patch/25776/
State New
Headers show

Comments

Joseph Myers - Feb. 2, 2018, 5:43 p.m.
Building with -Os produces linknamespace and localplt failures for,
among other functions, gnu_dev_major, gnu_dev_minor and
gnu_dev_makedev.

The issue is that those functions are not inlined when building with
-Os.  While one could force them to be inlined in that case, it seems
more natural to fix this issue similarly to other namespace issues.
Thus, this patch makes gnu_dev_* into weak aliases for hidden symbols
__gnu_dev_*; __gnu_dev_* are then defined as inlines in the internal
include/sys/sysmacros.h, and uses of gnu_dev_* (often via the macros
major, minor and makedev) for which there are namespace issues are
changed to use __gnu_dev_*; where there are no namespace issues, use
of libc_hidden_proto serves to avoid unnecessary local PLT entry use.

Tested for x86_64, (a) without -Os, to verify the testsuite continues
to pass without problems and that the functions called under their new
names continue to be inlined as expected in that case; (b) with -Os,
together with
<https://sourceware.org/ml/libc-alpha/2018-02/msg00038.html> (pending
review) to fix the build, to verify that the linknamespace and
localplt failures in question go away (but because of other such
failures present, neither of the relevant bugs can yet be closed).

2018-02-02  Joseph Myers  <joseph@codesourcery.com>

	[BZ #15105]
	[BZ #19463]
	* include/sys/sysmacros.h [!_ISOMAC]
	(__SYSMACROS_NEED_IMPLEMENTATION): Define macro.
	[!_SYS_SYSMACROS_H_WRAPPER && !_ISOMAC]
	(_SYS_SYSMACROS_H_WRAPPER): Likewise.
	[!_SYS_SYSMACROS_H_WRAPPER && !_ISOMAC] (gnu_dev_major): Use
	libc_hidden_proto.
	[!_SYS_SYSMACROS_H_WRAPPER && !_ISOMAC] (gnu_dev_minor): Likewise.
	[!_SYS_SYSMACROS_H_WRAPPER && !_ISOMAC] (gnu_dev_makedev):
	Likewise.
	[!_SYS_SYSMACROS_H_WRAPPER && !_ISOMAC] (__SYSMACROS_DECL_TEMPL):
	Undefine and redefine to add use __gnu_dev_ prefix.
	[!_SYS_SYSMACROS_H_WRAPPER && !_ISOMAC] (__SYSMACROS_IMPL_TEMPL):
	Likewise.
	[!_SYS_SYSMACROS_H_WRAPPER && !_ISOMAC] (__gnu_dev_major): Declare
	and define as hidden inline function.
	[!_SYS_SYSMACROS_H_WRAPPER && !_ISOMAC] (__gnu_dev_minor):
	Likewise.
	[!_SYS_SYSMACROS_H_WRAPPER && !_ISOMAC] (__gnu_dev_makedev):
	Likewise.
	* misc/makedev.c (OUT_OF_LINE_IMPL_TEMPL): Use __gnu_dev_ prefix.
	(gnu_dev_major): Use weak_alias and libc_hidden_weak.
	(gnu_dev_minor): Likewise.
	(gnu_dev_makedev): Likewise.
	* csu/check_fds.c (check_one_fd): Use __gnu_dev_makedev instead of
	makedev.
	* posix/wordexp.c (exec_comm_child): Likewise.
	* sysdeps/mach/hurd/xmknodat.c (__xmknodat): Use __gnu_dev_minor
	instead of minor and __gnu_dev_major instead of major.
	* sysdeps/unix/sysv/linux/device-nrs.h (DEV_TTY_P): Use
	__gnu_dev_major instead of major.
	* sysdeps/unix/sysv/linux/pathconf.c (distinguish_extX): Use
	__gnu_dev_major instead of gnu_dev_major and __gnu_dev_minor
	instead of gnu_dev_minor.
	* sysdeps/unix/sysv/linux/ptsname.c (MASTER_P): Likewise.
	(SLAVE_P): Likewise.
	(__ptsname_internal): Use __gnu_dev_minor instead of minor.
	* sysdeps/unix/sysv/linux/ttyname.h (is_pty): Use __gnu_dev_major
	instead of major.
Joseph Myers - Feb. 7, 2018, 1:35 p.m.
Ping.  This patch 
<https://sourceware.org/ml/libc-alpha/2018-02/msg00076.html> is pending 
review.
Florian Weimer - Feb. 7, 2018, 2:10 p.m.
On 02/02/2018 06:43 PM, Joseph Myers wrote:
> Building with -Os produces linknamespace and localplt failures for,
> among other functions, gnu_dev_major, gnu_dev_minor and
> gnu_dev_makedev.
> 
> The issue is that those functions are not inlined when building with
> -Os.  While one could force them to be inlined in that case, it seems
> more natural to fix this issue similarly to other namespace issues.
> Thus, this patch makes gnu_dev_* into weak aliases for hidden symbols
> __gnu_dev_*; __gnu_dev_* are then defined as inlines in the internal
> include/sys/sysmacros.h, and uses of gnu_dev_* (often via the macros
> major, minor and makedev) for which there are namespace issues are
> changed to use __gnu_dev_*; where there are no namespace issues, use
> of libc_hidden_proto serves to avoid unnecessary local PLT entry use.

Patch looks reasonable.  Thanks.

Florian
Zack Weinberg - Feb. 7, 2018, 3:15 p.m.
This isn't directly related, but what do you think of completing the
removal of sys/sysmacros.h from sys/types.h in this release cycle?
Florian Weimer - Feb. 7, 2018, 3:27 p.m.
On 02/07/2018 04:15 PM, Zack Weinberg wrote:
> This isn't directly related, but what do you think of completing the
> removal of sys/sysmacros.h from sys/types.h in this release cycle?

I'm in favor.

Thanks,
Florian
Joseph Myers - Feb. 7, 2018, 4:30 p.m.
On Wed, 7 Feb 2018, Zack Weinberg wrote:

> This isn't directly related, but what do you think of completing the
> removal of sys/sysmacros.h from sys/types.h in this release cycle?

I support that.

Patch

diff --git a/csu/check_fds.c b/csu/check_fds.c
index abcdfc0..2b181d4 100644
--- a/csu/check_fds.c
+++ b/csu/check_fds.c
@@ -50,12 +50,12 @@  check_one_fd (int fd, int mode)
       if ((mode & O_ACCMODE) == O_WRONLY)
 	{
 	  name = _PATH_DEV "full";
-	  dev = makedev (DEV_FULL_MAJOR, DEV_FULL_MINOR);
+	  dev = __gnu_dev_makedev (DEV_FULL_MAJOR, DEV_FULL_MINOR);
 	}
       else
 	{
 	  name = _PATH_DEVNULL;
-	  dev = makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR);
+	  dev = __gnu_dev_makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR);
 	}
 
       /* Something is wrong with this descriptor, it's probably not
diff --git a/include/sys/sysmacros.h b/include/sys/sysmacros.h
index 87813c5..3c2b9c2 100644
--- a/include/sys/sysmacros.h
+++ b/include/sys/sysmacros.h
@@ -1 +1,52 @@ 
+/* Definitions of macros to access 'dev_t' values.  Internal header.
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _ISOMAC
+# define __SYSMACROS_NEED_IMPLEMENTATION
+#endif
+
 #include <misc/sys/sysmacros.h>
+
+#if !defined _SYS_SYSMACROS_H_WRAPPER && !defined _ISOMAC
+# define _SYS_SYSMACROS_H_WRAPPER 1
+
+libc_hidden_proto (gnu_dev_major)
+libc_hidden_proto (gnu_dev_minor)
+libc_hidden_proto (gnu_dev_makedev)
+
+# undef __SYSMACROS_DECL_TEMPL
+# define __SYSMACROS_DECL_TEMPL(rtype, name, proto)	\
+  extern rtype __gnu_dev_##name proto			\
+  __THROW __attribute_const__ attribute_hidden;
+
+# undef __SYSMACROS_IMPL_TEMPL
+# define __SYSMACROS_IMPL_TEMPL(rtype, name, proto)		\
+  __extension__ __extern_inline __attribute_const__ rtype	\
+  __NTH (__gnu_dev_##name proto)
+
+__SYSMACROS_DECLARE_MAJOR (__SYSMACROS_DECL_TEMPL)
+__SYSMACROS_DECLARE_MINOR (__SYSMACROS_DECL_TEMPL)
+__SYSMACROS_DECLARE_MAKEDEV (__SYSMACROS_DECL_TEMPL)
+
+# ifdef __USE_EXTERN_INLINES
+__SYSMACROS_DEFINE_MAJOR (__SYSMACROS_IMPL_TEMPL)
+__SYSMACROS_DEFINE_MINOR (__SYSMACROS_IMPL_TEMPL)
+__SYSMACROS_DEFINE_MAKEDEV (__SYSMACROS_IMPL_TEMPL)
+# endif
+
+#endif
diff --git a/misc/makedev.c b/misc/makedev.c
index 5da14ce..bc5bfc5 100644
--- a/misc/makedev.c
+++ b/misc/makedev.c
@@ -23,8 +23,14 @@ 
 #include <sys/sysmacros.h>
 
 #define OUT_OF_LINE_IMPL_TEMPL(rtype, name, proto) \
-  rtype gnu_dev_##name proto
+  rtype __gnu_dev_##name proto
 
 __SYSMACROS_DEFINE_MAJOR(OUT_OF_LINE_IMPL_TEMPL)
+weak_alias (__gnu_dev_major, gnu_dev_major)
+libc_hidden_weak (gnu_dev_major)
 __SYSMACROS_DEFINE_MINOR(OUT_OF_LINE_IMPL_TEMPL)
+weak_alias (__gnu_dev_minor, gnu_dev_minor)
+libc_hidden_weak (gnu_dev_minor)
 __SYSMACROS_DEFINE_MAKEDEV(OUT_OF_LINE_IMPL_TEMPL)
+weak_alias (__gnu_dev_makedev, gnu_dev_makedev)
+libc_hidden_weak (gnu_dev_makedev)
diff --git a/posix/wordexp.c b/posix/wordexp.c
index 8e33ad9..0b669a8 100644
--- a/posix/wordexp.c
+++ b/posix/wordexp.c
@@ -837,7 +837,7 @@  exec_comm_child (char *comm, int *fildes, int showerr, int noexec)
       if (__builtin_expect (__fxstat64 (_STAT_VER, STDERR_FILENO, &st), 0) != 0
 	  || __builtin_expect (S_ISCHR (st.st_mode), 1) == 0
 #if defined DEV_NULL_MAJOR && defined DEV_NULL_MINOR
-	  || st.st_rdev != makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR)
+	  || st.st_rdev != __gnu_dev_makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR)
 #endif
 	  )
 	/* It's not the /dev/null device.  Stop right here.  The
diff --git a/sysdeps/mach/hurd/xmknodat.c b/sysdeps/mach/hurd/xmknodat.c
index 40d86aa..e4e866c 100644
--- a/sysdeps/mach/hurd/xmknodat.c
+++ b/sysdeps/mach/hurd/xmknodat.c
@@ -81,9 +81,9 @@  __xmknodat (int vers, int fd, const char *path, mode_t mode, dev_t *dev)
 
       bp = buf + sizeof (buf);
       *--bp = '\0';
-      bp = _itoa (minor (*dev), bp, 10, 0);
+      bp = _itoa (__gnu_dev_minor (*dev), bp, 10, 0);
       *--bp = '\0';
-      bp = _itoa (major (*dev), bp, 10, 0);
+      bp = _itoa (__gnu_dev_major (*dev), bp, 10, 0);
       memcpy (bp - len, translator, len);
       translator = bp - len;
       len = buf + sizeof (buf) - translator;
diff --git a/sysdeps/unix/sysv/linux/device-nrs.h b/sysdeps/unix/sysv/linux/device-nrs.h
index 8d5558d..6fd7ad0 100644
--- a/sysdeps/unix/sysv/linux/device-nrs.h
+++ b/sysdeps/unix/sysv/linux/device-nrs.h
@@ -39,7 +39,7 @@ 
 
 /* Test whether given device is a tty.  */
 #define DEV_TTY_P(statp) \
-  ({ int __dev_major = major ((statp)->st_rdev);			      \
+  ({ int __dev_major = __gnu_dev_major ((statp)->st_rdev);		      \
      __dev_major >= DEV_TTY_LOW_MAJOR && __dev_major <= DEV_TTY_HIGH_MAJOR; })
 
 #endif	/* device-nrs.h */
diff --git a/sysdeps/unix/sysv/linux/pathconf.c b/sysdeps/unix/sysv/linux/pathconf.c
index 3b8f480..59a50ce 100644
--- a/sysdeps/unix/sysv/linux/pathconf.c
+++ b/sysdeps/unix/sysv/linux/pathconf.c
@@ -73,7 +73,7 @@  distinguish_extX (const struct statfs *fsbuf, const char *file, int fd)
     return EXT2_LINK_MAX;
 
   __snprintf (buf, sizeof (buf), "/sys/dev/block/%u:%u",
-	      gnu_dev_major (st.st_dev), gnu_dev_minor (st.st_dev));
+	      __gnu_dev_major (st.st_dev), __gnu_dev_minor (st.st_dev));
 
   ssize_t n = __readlink (buf, path, sizeof (path));
   if (n != -1 && n < sizeof (path))
diff --git a/sysdeps/unix/sysv/linux/ptsname.c b/sysdeps/unix/sysv/linux/ptsname.c
index 6798dfb..a4c2336 100644
--- a/sysdeps/unix/sysv/linux/ptsname.c
+++ b/sysdeps/unix/sysv/linux/ptsname.c
@@ -29,16 +29,18 @@ 
 #include <_itoa.h>
 
 /* Check if DEV corresponds to a master pseudo terminal device.  */
-#define MASTER_P(Dev)                                                         \
-  (major ((Dev)) == 2                                                         \
-   || (major ((Dev)) == 4 && minor ((Dev)) >= 128 && minor ((Dev)) < 192)     \
-   || (major ((Dev)) >= 128 && major ((Dev)) < 136))
+#define MASTER_P(Dev)							\
+  (__gnu_dev_major ((Dev)) == 2						\
+   || (__gnu_dev_major ((Dev)) == 4					\
+       && __gnu_dev_minor ((Dev)) >= 128 && __gnu_dev_minor ((Dev)) < 192) \
+   || (__gnu_dev_major ((Dev)) >= 128 && __gnu_dev_major ((Dev)) < 136))
 
 /* Check if DEV corresponds to a slave pseudo terminal device.  */
-#define SLAVE_P(Dev)                                                          \
-  (major ((Dev)) == 3                                                         \
-   || (major ((Dev)) == 4 && minor ((Dev)) >= 192 && minor ((Dev)) < 256)     \
-   || (major ((Dev)) >= 136 && major ((Dev)) < 144))
+#define SLAVE_P(Dev)							\
+  (__gnu_dev_major ((Dev)) == 3						\
+   || (__gnu_dev_major ((Dev)) == 4					\
+       && __gnu_dev_minor ((Dev)) >= 192 && __gnu_dev_minor ((Dev)) < 256) \
+   || (__gnu_dev_major ((Dev)) >= 136 && __gnu_dev_major ((Dev)) < 144))
 
 /* Note that major number 4 corresponds to the old BSD style pseudo
    terminal devices.  As of Linux 2.1.115 these are no longer
@@ -122,7 +124,7 @@  __ptsname_internal (int fd, char *buf, size_t buflen, struct stat64 *stp)
 	  return ENOTTY;
 	}
 
-      ptyno = minor (stp->st_rdev);
+      ptyno = __gnu_dev_minor (stp->st_rdev);
 
       if (ptyno / 16 >= strlen (__libc_ptyname1))
 	{
diff --git a/sysdeps/unix/sysv/linux/ttyname.h b/sysdeps/unix/sysv/linux/ttyname.h
index 74332d2..8ef0a16 100644
--- a/sysdeps/unix/sysv/linux/ttyname.h
+++ b/sysdeps/unix/sysv/linux/ttyname.h
@@ -28,7 +28,7 @@  static inline bool
 is_pty (struct stat64 *sb)
 {
 #ifdef _STATBUF_ST_RDEV
-  int m = major (sb->st_rdev);
+  int m = __gnu_dev_major (sb->st_rdev);
   return (136 <= m && m <= 143);
 #else
   return false;