This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Use IE model for static variables in glibc
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Siddhesh Poyarekar <siddhesh at redhat dot com>, libc-alpha at sourceware dot org
- Date: Thu, 16 Jul 2015 00:44:16 -0400
- Subject: Re: [PATCH] Use IE model for static variables in glibc
- Authentication-results: sourceware.org; auth=none
- References: <20150709180544 dot GA8522 at spoyarek dot pnq dot redhat dot com>
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.