Re: [PATCH 1/4] Add _dl_{mask,unmask}_all_signals

[Thomas Schwinge <thomas at codesourcery dot com>]

Hi!

On Tue, 10 Dec 2013 16:35:40 -0800, Andrew Hunter <ahh@google.com> wrote:
> This is patch 1/4 of the effort to make TLS access async-signal-safe.
> 
> Add useful functions to disable/enable signals (portably.)  This will
> enable us to guard against reentrant initializations.
> ---
>  sysdeps/generic/ldsodefs.h          |  5 ++++
>  sysdeps/mach/hurd/dl-sysdep.h       |  7 ++++++
>  sysdeps/unix/sysv/linux/dl-sysdep.c | 46 +++++++++++++++++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/dl-sysdep.h |  4 ++++

This got pushed as commit 69a17d9d245dc3551792e95e1823cc2d877592f3; the
ChangeLog shows Andrew as the author, but the Git author entry shows
Paul?


> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -233,6 +233,11 @@ extern int _dl_name_match_p (const char *__name, const struct link_map *__map)
>  extern unsigned long int _dl_higher_prime_number (unsigned long int n)
>       internal_function;
>  
> +/* Mask every signal, returning the previous sigmask in OLD.  */
> +extern void _dl_mask_all_signals (sigset_t *old) internal_function;
> +/* Undo _dl_mask_all_signals.  */
> +extern void _dl_unmask_signals (sigset_t *old) internal_function;

Why are these declarations needed both here and in the dl-sysdep.h files?

In ldsodefs.h, the unmasking declaration is named _dl_unmask_signals
(matching the implementation in the Linux dl-sysdep.c), and in the
dl-sysdep.h files it's named _dl_unmask_all_signals.


> --- a/sysdeps/mach/hurd/dl-sysdep.h
> +++ b/sysdeps/mach/hurd/dl-sysdep.h
> @@ -29,3 +29,10 @@
>  # define DL_ARGV_NOT_RELRO 1
>  # define LIBC_STACK_END_NOT_RELRO 1
>  #endif
> +
> +#include <signal.h>
> +inline void _dl_mask_all_signals (sigset_t *) internal_function;
> +inline void _dl_mask_all_signals (sigset_t *) { }
> +
> +inline void _dl_unmask_all_signals (sigset_t *) internal_function;
> +inline void _dl_unmask_all_signals (sigset_t *) { }

Thanks for caring about the Hurd, too!  (I'm always happy to be CCed; I
don't manage to read mailing lists regularely and in detail.)  However, I
found about four errors in these seven lines.  ;-P

As this is also used in assembler contexts, the C stuff needs to be
guarded by Â#ifndef __ASSEMBLER__Â.  The inline function definitions are
missing a name for their parameter.  They're incompatible to their inline
declarations the line before: missing internal_function.  The ldsodefs.h
external declarations are incompatible to these inline declarations.


I suggest to fix this up as follows, or should this be done differently?
Testing for Hurd is running; could you please test the Linux case?

	* sysdeps/mach/hurd/dl-sysdep.c (_dl_mask_all_signals)
	(_dl_unmask_signals): New internal functions.
	* sysdeps/mach/hurd/dl-sysdep.h (_dl_mask_all_signals)
	(_dl_unmask_all_signals): Remove.
	* sysdeps/unix/sysv/linux/dl-sysdep.h (_dl_mask_all_signals)
	(_dl_unmask_all_signals): Likewise.

diff --git sysdeps/mach/hurd/dl-sysdep.c sysdeps/mach/hurd/dl-sysdep.c
index f162ff0..4312766 100644
--- sysdeps/mach/hurd/dl-sysdep.c
+++ sysdeps/mach/hurd/dl-sysdep.c
@@ -16,10 +16,6 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-/* In the static library, this is all handled by dl-support.c
-   or by the vanilla definitions in the rest of the C library.  */
-#ifdef SHARED
-
 #include <hurd.h>
 #include <link.h>
 #include <unistd.h>
@@ -44,6 +40,10 @@
 #include <dl-machine.h>
 #include <dl-procinfo.h>
 
+/* In the static library, this is all handled by dl-support.c
+   or by the vanilla definitions in the rest of the C library.  */
+#ifdef SHARED
+
 extern void __mach_init (void);
 
 extern int _dl_argc;
@@ -670,3 +670,17 @@ _dl_init_first (int argc, ...)
 }
 
 #endif /* SHARED */
+
+/* Mask every signal, returning the previous sigmask in OLD.  */
+void
+internal_function
+_dl_mask_all_signals (sigset_t *old)
+{
+}
+
+/* Return sigmask to what it was before a call to _dl_mask_all_signals.  */
+void
+internal_function
+_dl_unmask_signals (sigset_t *old)
+{
+}
diff --git sysdeps/mach/hurd/dl-sysdep.h sysdeps/mach/hurd/dl-sysdep.h
index 0e7cac4..52563b0 100644
--- sysdeps/mach/hurd/dl-sysdep.h
+++ sysdeps/mach/hurd/dl-sysdep.h
@@ -29,10 +29,3 @@
 # define DL_ARGV_NOT_RELRO 1
 # define LIBC_STACK_END_NOT_RELRO 1
 #endif
-
-#include <signal.h>
-inline void _dl_mask_all_signals (sigset_t *) internal_function;
-inline void _dl_mask_all_signals (sigset_t *) { }
-
-inline void _dl_unmask_all_signals (sigset_t *) internal_function;
-inline void _dl_unmask_all_signals (sigset_t *) { }
diff --git sysdeps/unix/sysv/linux/dl-sysdep.h sysdeps/unix/sysv/linux/dl-sysdep.h
index 0fe1e1c..e1eab09 100644
--- sysdeps/unix/sysv/linux/dl-sysdep.h
+++ sysdeps/unix/sysv/linux/dl-sysdep.h
@@ -30,8 +30,4 @@
 /* Get version of the OS.  */
 extern int _dl_discover_osversion (void) attribute_hidden;
 # define HAVE_DL_DISCOVER_OSVERSION	1
-
-#include <signal.h>
-void _dl_mask_all_signals (sigset_t *) internal_function;
-void _dl_unmask_all_signals (sigset_t *) internal_function;
 #endif


GrÃÃe,
 Thomas

Attachment: pgpCU5QfoKYn3.pgp
Description: PGP signature