Re: [PATCH] libc: Extend __libc_freeres framework (Bug 23329).

[Florian Weimer <fweimer at redhat dot com>]

On 06/27/2018 05:06 AM, Carlos O'Donell wrote:

> diff --git a/include/libc-symbols.h b/include/libc-symbols.h
> index 6137304b0b..89a5604ee7 100644
> --- a/include/libc-symbols.h
> +++ b/include/libc-symbols.h

> +/* Resource pointers to freed in libc.so.  */
> +#define libc_freeres_ptr(decl) \
> +  __make_section_unallocated ("__libc_freeres_ptrs, \"aw\", %nobits") \
> +  decl __attribute__ ((section ("__libc_freeres_ptrs" __sec_comment)))
> +
> +/* Resource freeing functions from libc.so.  */
> +#define __libc_freeres_fn_section \
> +  __attribute__ ((section ("__libc_freeres_fn")))

These are okay.

> +/* Resource freeing functions for threads in libc.so.  */
> +#define __libc_thread_freeres_fn_section \
> +  __attribute__ ((section ("__libc_thread_freeres_fn")))

This is an unrelated change.  I think you should drop it.  Since these 
functions run during normal operation, it is unclear whether placing 
them into a separate section is actually beneficial.

> +/* Resource freeing functions for libdl.so  */
> +#define __libdl_freeres_fn_section \
> +  __attribute__ ((section ("__libdl_freeres_fn")))
> +
> +/* Resource freeing functions for libpthread.so.  */
> +#define __libpthread_freeres_fn_section \
> +  __attribute__ ((section ("__libpthread_freeres_fn")))

I suggest to drop those as well, for now.  The reason is that you didn't 
add placement for the section in the linker script.

> diff --git a/malloc/set-freeres.c b/malloc/set-freeres.c
> index f4a0e7bda4..9e0de0c403 100644
> --- a/malloc/set-freeres.c
> +++ b/malloc/set-freeres.c
> @@ -26,6 +26,10 @@ DEFINE_HOOK (__libc_subfreeres, (void));
>   
>   symbol_set_define (__libc_freeres_ptrs);
>   
> +extern __attribute__((weak)) void __libdl_freeres (void);
> +
> +extern __attribute__((weak)) void __libpthread_freeres (void);
> +

Missing space after __attribute__.  I think that's the current convention.

>   void __libc_freeres_fn_section
>   __libc_freeres (void)
>   {
> @@ -39,8 +43,19 @@ __libc_freeres (void)
>   
>         _IO_cleanup ();
>   
> +      /* We run the resource freeing after IO cleanup.  */
>         RUN_HOOK (__libc_subfreeres, ());
>   
> +      /* Call the libdl list of cleanup functions
> +	 (weak-ref-and-check).  */
> +      if (&__libdl_freeres != NULL)
> +	call_function_static_weak (__libdl_freeres);
> +
> +      /* Call the libpthread list of cleanup functions
> +	 (weak-ref-and-check).  */
> +      if (&__libpthread_freeres != NULL)
> +	call_function_static_weak (__libpthread_freeres);

Weak declared twice here.  I think you should call these functions 
directly, without call_function_static_weak.

Rest looks okay, thanks.

Florian