Patchwork hurd: add gscope support

login
register
mail settings
Submitter Samuel Thibault
Date March 1, 2018, 12:50 a.m.
Message ID <20180301005014.10220-1-samuel.thibault@ens-lyon.org>
Download mbox | patch
Permalink /patch/26125/
State Committed, archived
Headers show

Comments

Samuel Thibault - March 1, 2018, 12:50 a.m.
* elf/dl-support.c [THREAD_GSCOPE_GLOBAL] (_dl_thread_gscope_count):
	Define variable.
	* sysdeps/generic/ldsodefs.h [THREAD_GSCOPE_GLOBAL] (struct
	rtld_global): Add _dl_thread_gscope_count member.
	* sysdeps/mach/hurd/tls.h: Include <atomic.h>.
	[!defined __ASSEMBLER__] (THREAD_GSCOPE_GLOBAL, THREAD_GSCOPE_SET_FLAG,
	THREAD_GSCOPE_RESET_FLAG, THREAD_GSCOPE_WAIT): Define macros.
---
 ChangeLog                  | 10 ++++++++++
 elf/dl-support.c           |  3 +++
 sysdeps/generic/ldsodefs.h |  3 +++
 sysdeps/mach/hurd/tls.h    | 16 ++++++++++++++++
 4 files changed, 32 insertions(+)
Joseph Myers - March 1, 2018, 1:17 a.m.
On Thu, 1 Mar 2018, Samuel Thibault wrote:

> diff --git a/elf/dl-support.c b/elf/dl-support.c
> index 114f77a8e5..be564306f2 100644
> --- a/elf/dl-support.c
> +++ b/elf/dl-support.c
> @@ -188,6 +188,9 @@ int (*_dl_make_stack_executable_hook) (void **) = _dl_make_stack_executable;
>  /* Function in libpthread to wait for termination of lookups.  */
>  void (*_dl_wait_lookup_done) (void);
>  
> +#ifdef THREAD_GSCOPE_GLOBAL
> +int _dl_thread_gscope_count;
> +#endif

Current preferred practice is always-defined macros, tested with #if 
rather than #ifdef.

There might be exceptions for a new macro that's part of an existing group 
of closely-related macros using the #if convention.  But this doesn't seem 
to apply in this case, and the patch writeup has no explanation of why 
#ifdef might be needed here.

I would hope that the new macro would have a comment somewhere (I suppose 
on its default definition) explaining its semantics (and that this 
variable would also have a comment documenting its semantics, as is 
generally appropriate for any global variable or function or macro).

> +/* Temporary poor-man's global scope switch support */

Comments should end with ".  ".

> +#define THREAD_GSCOPE_GLOBAL
> +#define THREAD_GSCOPE_SET_FLAG()					      \
> +	atomic_increment(&GL(dl_thread_gscope_count))
> +#define THREAD_GSCOPE_RESET_FLAG() do {					      \
> +	if (atomic_decrement_val(&GL(dl_thread_gscope_count)) == 0)	      \
> +		lll_wake(&GL(dl_thread_gscope_count), 0);		      \
> +} while (0)
> +#define THREAD_GSCOPE_WAIT() do {					      \
> +	int count;							      \
> +	while ((count = GL(dl_thread_gscope_count))) {			      \
> +		lll_wait(&GL(dl_thread_gscope_count), count, 0);	      \
> +	}								      \
> +} while (0)

Missing spaces before '(' in the calls to atomic_increment, 
atomic_decrement_val, lll_wake, lll_wait.  (It's correct for the spaces to 
be omitted in calls to GL, as that logically expands to a single 
identifier.)

do {} while (0) should be written with the normal GNU Coding Standards 
formatting, so

#define something
  do
    {
      ...;
    }
  while (0)

(with appropriate backslashes).  (Other formatting in these macro 
definitions seems to be off as well - two column indentation should be 
used, '{' should be on its own line in the 'while' if the {} are needed 
there at all, which I don't think they are.)

Patch

diff --git a/elf/dl-support.c b/elf/dl-support.c
index 114f77a8e5..be564306f2 100644
--- a/elf/dl-support.c
+++ b/elf/dl-support.c
@@ -188,6 +188,9 @@  int (*_dl_make_stack_executable_hook) (void **) = _dl_make_stack_executable;
 /* Function in libpthread to wait for termination of lookups.  */
 void (*_dl_wait_lookup_done) (void);
 
+#ifdef THREAD_GSCOPE_GLOBAL
+int _dl_thread_gscope_count;
+#endif
 struct dl_scope_free_list *_dl_scope_free_list;
 
 #ifdef NEED_DL_SYSINFO
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index 5e1b24ecb5..2447cd6717 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -435,6 +435,9 @@  struct rtld_global
     size_t count;
     void *list[50];
   } *_dl_scope_free_list;
+#ifdef THREAD_GSCOPE_GLOBAL
+  EXTERN int _dl_thread_gscope_count;
+#endif
 #ifdef SHARED
 };
 # define __rtld_global_attribute__
diff --git a/sysdeps/mach/hurd/tls.h b/sysdeps/mach/hurd/tls.h
index faff87c7b5..47a9a357c8 100644
--- a/sysdeps/mach/hurd/tls.h
+++ b/sysdeps/mach/hurd/tls.h
@@ -27,6 +27,7 @@ 
 # include <sysdep.h>
 # include <mach/mig_errors.h>
 # include <mach.h>
+# include <atomic.h>
 
 
 /* This is the size of the initial TCB.  */
@@ -51,6 +52,21 @@ 
 # define GET_DTV(descr) \
   (((tcbhead_t *) (descr))->dtv)
 
+/* Temporary poor-man's global scope switch support */
+#define THREAD_GSCOPE_GLOBAL
+#define THREAD_GSCOPE_SET_FLAG()					      \
+	atomic_increment(&GL(dl_thread_gscope_count))
+#define THREAD_GSCOPE_RESET_FLAG() do {					      \
+	if (atomic_decrement_val(&GL(dl_thread_gscope_count)) == 0)	      \
+		lll_wake(&GL(dl_thread_gscope_count), 0);		      \
+} while (0)
+#define THREAD_GSCOPE_WAIT() do {					      \
+	int count;							      \
+	while ((count = GL(dl_thread_gscope_count))) {			      \
+		lll_wait(&GL(dl_thread_gscope_count), count, 0);	      \
+	}								      \
+} while (0)
+
 #endif /* !ASSEMBLER */