Good Morning Stefan, On 02/13/2014 11:44 PM, Stefan Seefeld wrote: > On 02/13/2014 05:06 PM, Woegerer, Paul wrote: >> Let me put it this way ... >> >> If (hypothetically, just for the sake of the argument) we would have dlsym >> with the following signature: >> >> void *dlsym(void *handle, const char *symbol, void *dummy); >> >> instead of: >> >> void *dlsym(void *handle, const char *symbol); >> >> and we would call it with: >> >> af.calloc = dlsym(RTLD_NEXT, "calloc", &cur_alloc); >> >> then (because of the aliasing of cur_alloc (caused by &cur_alloc) the >> compiler would be forced to store the effects done on cur_alloc into memory >> prior to calling dlsym. > > Paul, I'm not convinced. > > Our compilation unit defines a bunch of functions with external linkage, > which access cur_alloc. And since gcc has no way to rule out that the > call to dlsym() will not cause any of these functions to be called, it > mustn't make any assumptions about whether or not the first > initialization of cur_alloc is redundant or not, and thus shouldn't > elide it.
Thinking about it again (with 7hrs of sleep in-between), I'm still not convinced that this is a compiler bug. See: https://www.kernel.org/doc/Documentation/memory-barriers.txt and search for "The compiler is within its rights to reorder memory accesses" and "the compiler is within its rights to omit a store entirely" Also related: http://stackoverflow.com/questions/5693274/is-function-call-a-memory-barrier http://stackoverflow.com/questions/10698253/is-function-call-an-effective-memory-barrier-for-modern-platforms Nonetheless, I suggest to replace the volatile fields (see my first patch) with ACCESS_ONCE() barriers as described in: https://www.kernel.org/doc/Documentation/memory-barriers.txt diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c index 8296ae2..44ce933 100644 --- a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c +++ b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c @@ -173,17 +173,17 @@ static void setup_static_allocator(void) { assert(cur_alloc.calloc == NULL); - cur_alloc.calloc = static_calloc; + CMM_ACCESS_ONCE(cur_alloc.calloc) = static_calloc; assert(cur_alloc.malloc == NULL); - cur_alloc.malloc = static_malloc; + CMM_ACCESS_ONCE(cur_alloc.malloc) = static_malloc; assert(cur_alloc.free == NULL); - cur_alloc.free = static_free; + CMM_ACCESS_ONCE(cur_alloc.free) = static_free; assert(cur_alloc.realloc == NULL); - cur_alloc.realloc = static_realloc; + CMM_ACCESS_ONCE(cur_alloc.realloc) = static_realloc; assert(cur_alloc.memalign == NULL); - cur_alloc.memalign = static_memalign; + CMM_ACCESS_ONCE(cur_alloc.memalign) = static_memalign; assert(cur_alloc.posix_memalign == NULL); - cur_alloc.posix_memalign = static_posix_memalign; + CMM_ACCESS_ONCE(cur_alloc.posix_memalign) = static_posix_memalign; } static @@ -207,7 +207,7 @@ void lookup_all_symbols(void) af.posix_memalign = dlsym(RTLD_NEXT, "posix_memalign"); /* Populate the new allocator functions */ - memcpy(&cur_alloc, &af, sizeof(cur_alloc)); + cur_alloc = af; } void *malloc(size_t size) -- Many Thanks, Paul > > (The above is in fact quite a frequent idiom in C/C++ framework > libraries. Just imagine dlsym() being a call into a GUI (such as an > event loop), and the functions in this CU unit as callbacks.) > > Stefan > -- Paul Woegerer, SW Development Engineer Sourcery Analyzer <http://go.mentor.com/sourceryanalyzer> Mentor Graphics, Embedded Software Division _______________________________________________ lttng-dev mailing list [email protected] http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
