This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Use IE model for static variables in glibc


On 07/09/2015 02:05 PM, Siddhesh Poyarekar wrote:
> 	[BZ #18457]
> 	* nptl/Makefile (tests): New test case tst-join7.
> 	(modules-names): New test case module tst-join7mod.
> 	* nptl/tst-join7.c: New file.
> 	* nptl/tst-join7mod.c: New file.
> 	* stdlib/cxa_thread_atexit_impl.c (tls_dtor_list,
> 	dso_symbol_cache, lm_cache): Mark variables as IE.
> 	* string/strerror_l.c (last_value): Likewise.

OK if you fix the nits below.

This is now a release blocker :-)

> ---
>  nptl/Makefile                   | 10 ++++++++--
>  nptl/tst-join7.c                | 12 ++++++++++++
>  nptl/tst-join7mod.c             | 30 ++++++++++++++++++++++++++++++
>  stdlib/cxa_thread_atexit_impl.c |  6 +++---
>  string/strerror_l.c             |  2 +-
>  5 files changed, 54 insertions(+), 6 deletions(-)
>  create mode 100644 nptl/tst-join7.c
>  create mode 100644 nptl/tst-join7mod.c
> 
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 4544aa2..f14f4d6 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -245,7 +245,7 @@ tests = tst-typesizes \
>  	tst-basic7 \
>  	tst-kill1 tst-kill2 tst-kill3 tst-kill4 tst-kill5 tst-kill6 \
>  	tst-raise1 \
> -	tst-join1 tst-join2 tst-join3 tst-join4 tst-join5 tst-join6 \
> +	tst-join1 tst-join2 tst-join3 tst-join4 tst-join5 tst-join6 tst-join7 \

OK

>  	tst-detach1 \
>  	tst-eintr1 tst-eintr2 tst-eintr3 tst-eintr4 tst-eintr5 \
>  	tst-tsd1 tst-tsd2 tst-tsd3 tst-tsd4 tst-tsd5 tst-tsd6 \
> @@ -323,7 +323,8 @@ endif
>  modules-names = tst-atfork2mod tst-tls3mod tst-tls4moda tst-tls4modb \
>  		tst-tls5mod tst-tls5moda tst-tls5modb tst-tls5modc \
>  		tst-tls5modd tst-tls5mode tst-tls5modf tst-stack4mod \
> -		tst-_res1mod1 tst-_res1mod2 tst-execstack-mod tst-fini1mod
> +		tst-_res1mod1 tst-_res1mod2 tst-execstack-mod tst-fini1mod \
> +		tst-join7mod

OK.

>  extra-test-objs += $(addsuffix .os,$(strip $(modules-names))) tst-cleanup4aux.o
>  test-extras += $(modules-names) tst-cleanup4aux
>  test-modules = $(addprefix $(objpfx),$(addsuffix .so,$(modules-names)))
> @@ -528,6 +529,11 @@ $(objpfx)tst-tls6.out: tst-tls6.sh $(objpfx)tst-tls5 \
>  	$(evaluate-test)
>  endif
>  
> +$(objpfx)tst-join7: $(libdl) $(shared-thread-library)
> +$(objpfx)tst-join7.out: $(objpfx)tst-join7mod.so
> +$(objpfx)tst-join7mod.so: $(shared-thread-library)
> +LDFLAGS-tst-join7mod.so = -Wl,-soname,tst-join7mod.so
> +

OK.

>  $(objpfx)tst-dlsym1: $(libdl) $(shared-thread-library)
>  
>  $(objpfx)tst-fini1: $(shared-thread-library) $(objpfx)tst-fini1mod.so
> diff --git a/nptl/tst-join7.c b/nptl/tst-join7.c
> new file mode 100644
> index 0000000..bf6fc76
> --- /dev/null
> +++ b/nptl/tst-join7.c
> @@ -0,0 +1,12 @@

Needs header, and one line description of test, along with bug reference.

> +#include <dlfcn.h>
> +

As roland pointed out, this needs to describe what it's doing in a
lengthy comment :-)

> +int
> +do_test (void)
> +{
> +  void *f = dlopen ("tst-join7mod.so", RTLD_NOW | RTLD_GLOBAL);
> +  if (f) dlclose (f); else return 1;
> +  return 0;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> diff --git a/nptl/tst-join7mod.c b/nptl/tst-join7mod.c
> new file mode 100644
> index 0000000..9960b76
> --- /dev/null
> +++ b/nptl/tst-join7mod.c

Needs header, one line bug description, and bug ID.

> @@ -0,0 +1,30 @@
> +#include <stdio.h>
> +#include <pthread.h>
> +#include <atomic.h>
> +
> +static pthread_t th;
> +static int running = 1;
> +
> +static void *
> +test_run (void *p)
> +{
> +  while (atomic_load_relaxed (&running))
> +    printf ("XXX test_run\n");
> +  printf ("XXX test_run FINISHED\n");

I don't like the use of "XXX" since it indictes unfinished
code or other bad comments.

Why not just "Test running..." and "Test finished." ?

> +  return NULL;
> +}
> +
> +static void __attribute__ ((constructor))
> +do_init (void)
> +{
> +  pthread_create (&th, NULL, test_run, NULL);

Check error.

> +}
> +
> +static void __attribute__ ((destructor))
> +do_end (void)
> +{
> +  atomic_store_relaxed (&running, 0);
> +  printf ("thread_join...\n");

Similar complaint:

"Calling pthread_join...\n"

> +  pthread_join (th, NULL);

Check error.

> +  printf ("thread_join DONE\n");

"Thread joined"

> +}
> diff --git a/stdlib/cxa_thread_atexit_impl.c b/stdlib/cxa_thread_atexit_impl.c
> index 54e2812..9120162 100644
> --- a/stdlib/cxa_thread_atexit_impl.c
> +++ b/stdlib/cxa_thread_atexit_impl.c
> @@ -29,9 +29,9 @@ struct dtor_list
>    struct dtor_list *next;
>  };
>  
> -static __thread struct dtor_list *tls_dtor_list;
> -static __thread void *dso_symbol_cache;
> -static __thread struct link_map *lm_cache;
> +static __thread struct dtor_list *tls_dtor_list attribute_tls_model_ie;
> +static __thread void *dso_symbol_cache attribute_tls_model_ie;
> +static __thread struct link_map *lm_cache attribute_tls_model_ie;

OK.

>  
>  /* Register a destructor for TLS variables declared with the 'thread_local'
>     keyword.  This function is only called from code generated by the C++
> diff --git a/string/strerror_l.c b/string/strerror_l.c
> index 2ed78b5..0b8bf2a 100644
> --- a/string/strerror_l.c
> +++ b/string/strerror_l.c
> @@ -23,7 +23,7 @@
>  #include <sys/param.h>
>  
>  
> -static __thread char *last_value;
> +static __thread char *last_value attribute_tls_model_ie;
>  

OK.

>  
>  static const char *
> 

Cheers,
Carlos.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]