From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 845A33854821 for ; Thu, 24 Jun 2021 21:10:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 845A33854821 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-194-pn3TbRJNO4ugWr8Ukpag0Q-1; Thu, 24 Jun 2021 17:10:44 -0400 X-MC-Unique: pn3TbRJNO4ugWr8Ukpag0Q-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2C7931835AC5; Thu, 24 Jun 2021 21:10:43 +0000 (UTC) Received: from greed.delorie.com (ovpn-112-111.rdu2.redhat.com [10.10.112.111]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 99CDA5D9D3; Thu, 24 Jun 2021 21:10:39 +0000 (UTC) Received: from greed.delorie.com.redhat.com (localhost [127.0.0.1]) by greed.delorie.com (8.15.2/8.15.2) with ESMTP id 15OLAcJX447593; Thu, 24 Jun 2021 17:10:38 -0400 From: DJ Delorie To: Siddhesh Poyarekar Cc: libc-alpha@sourceware.org, carlos@redhat.com, fweimer@redhat.com Subject: Re: [PATCH 2/8] malloc: Move malloc hook references to hooks.c In-Reply-To: <20210624182312.236596-3-siddhesh@sourceware.org> Date: Thu, 24 Jun 2021 17:10:38 -0400 Message-ID: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Jun 2021 21:10:47 -0000 Siddhesh Poyarekar writes: > Make the core malloc code hooks free and instead only have entry > points for _*_debug_before inline functions. > diff --git a/malloc/arena.c b/malloc/arena.c > index 7eb110445e..1861d20006 100644 > --- a/malloc/arena.c > +++ b/malloc/arena.c > @@ -44,8 +44,6 @@ > > /***************************************************************************/ > > -#define top(ar_ptr) ((ar_ptr)->top) > - Moved to malloc-internal.h; ok > diff --git a/malloc/hooks.c b/malloc/hooks.c > index 57a9b55788..4960aafd08 100644 > --- a/malloc/hooks.c > +++ b/malloc/hooks.c > @@ -21,35 +21,107 @@ > corrupt pointer is detected: do nothing (0), print an error message > (1), or call abort() (2). */ > > -/* Hooks for debugging versions. The initial hooks just call the > - initialization routine, then do the normal work. */ > +/* Define and initialize the hook variables. These weak definitions must > + appear before any use of the variables in a function (arena.c uses one). */ > +#ifndef weak_variable > +/* In GNU libc we want the hook variables to be weak definitions to > + avoid a problem with Emacs. */ > +# define weak_variable weak_function > +#endif > + > +/* Forward declarations. */ > + > +#if HAVE_MALLOC_INIT_HOOK > +void (*__malloc_initialize_hook) (void) __attribute__ ((nocommon)); > +compat_symbol (libc, __malloc_initialize_hook, > + __malloc_initialize_hook, GLIBC_2_0); > +#endif > + > +void weak_variable (*__free_hook) (void *__ptr, > + const void *) = NULL; > +void *weak_variable (*__malloc_hook) > + (size_t __size, const void *) = NULL; > +void *weak_variable (*__realloc_hook) > + (void *__ptr, size_t __size, const void *) = NULL; > +void *weak_variable (*__memalign_hook) > + (size_t __alignment, size_t __size, const void *) = NULL; > + > +static void ptmalloc_init (void); ok. > -static void * > -malloc_hook_ini (size_t sz, const void *caller) > +#include "malloc-check.c" Moved from later, ok. > +static __always_inline bool > +_malloc_debug_before (size_t bytes, void **victimp, const void *address) > { > - __malloc_hook = NULL; > - ptmalloc_init (); > - return __libc_malloc (sz); > + _Static_assert (PTRDIFF_MAX <= SIZE_MAX / 2, > + "PTRDIFF_MAX is not more than half of SIZE_MAX"); > + > + void *(*hook) (size_t, const void *) > + = atomic_forced_read (__malloc_hook); > + if (__builtin_expect (hook != NULL, 0)) > + { > + *victimp = (*hook)(bytes, address); > + return true; > + } > + return false; > } Ok. > -static void * > -realloc_hook_ini (void *ptr, size_t sz, const void *caller) > +static __always_inline bool > +_free_debug_before (void *mem, const void *address) > { > - __malloc_hook = NULL; > - __realloc_hook = NULL; > - ptmalloc_init (); > - return __libc_realloc (ptr, sz); > + void (*hook) (void *, const void *) > + = atomic_forced_read (__free_hook); > + if (__builtin_expect (hook != NULL, 0)) > + { > + (*hook)(mem, address); > + return true; > + } > + return false; > } Ok. > -static void * > -memalign_hook_ini (size_t alignment, size_t sz, const void *caller) > +static __always_inline bool > +_realloc_debug_before (void *oldmem, size_t bytes, void **victimp, > + const void *address) > { > - __memalign_hook = NULL; > - ptmalloc_init (); > - return __libc_memalign (alignment, sz); > + void *(*hook) (void *, size_t, const void *) = > + atomic_forced_read (__realloc_hook); > + if (__builtin_expect (hook != NULL, 0)) > + { > + *victimp = (*hook)(oldmem, bytes, address); > + return true; > + } > + > + return false; > } Ok. > -#include "malloc-check.c" Ok. > +static __always_inline bool > +_memalign_debug_before (size_t alignment, size_t bytes, void **victimp, > + const void *address) > +{ > + void *(*hook) (size_t, size_t, const void *) = > + atomic_forced_read (__memalign_hook); > + if (__builtin_expect (hook != NULL, 0)) > + { > + *victimp = (*hook)(alignment, bytes, address); > + return true; > + } > + return false; > +} Ok. > +static __always_inline bool > +_calloc_debug_before (size_t bytes, void **victimp, const void *address) > +{ > + void *(*hook) (size_t, const void *) = > + atomic_forced_read (__malloc_hook); > + if (__builtin_expect (hook != NULL, 0)) > + { > + *victimp = (*hook)(bytes, address); > + if (*victimp != NULL) > + memset (*victimp, 0, bytes); > + return true; > + } > + return false; > +} Ok. > diff --git a/malloc/malloc-internal.h b/malloc/malloc-internal.h > index 258f29584e..ee0f5697af 100644 > --- a/malloc/malloc-internal.h > +++ b/malloc/malloc-internal.h > @@ -61,6 +61,7 @@ > /* The corresponding bit mask value. */ > #define MALLOC_ALIGN_MASK (MALLOC_ALIGNMENT - 1) > > +#define top(ar_ptr) ((ar_ptr)->top) Ok. > diff --git a/malloc/malloc.c b/malloc/malloc.c > index 0e2e1747e0..75ca6ec3f0 100644 > --- a/malloc/malloc.c > +++ b/malloc/malloc.c > @@ -2013,30 +2013,6 @@ static void malloc_consolidate (mstate); > # define weak_variable weak_function > #endif > > -/* Forward declarations. */ > -static void *malloc_hook_ini (size_t sz, > - const void *caller) __THROW; > -static void *realloc_hook_ini (void *ptr, size_t sz, > - const void *caller) __THROW; > -static void *memalign_hook_ini (size_t alignment, size_t sz, > - const void *caller) __THROW; > - > -#if HAVE_MALLOC_INIT_HOOK > -void (*__malloc_initialize_hook) (void) __attribute__ ((nocommon)); > -compat_symbol (libc, __malloc_initialize_hook, > - __malloc_initialize_hook, GLIBC_2_0); > -#endif > - > -void weak_variable (*__free_hook) (void *__ptr, > - const void *) = NULL; > -void *weak_variable (*__malloc_hook) > - (size_t __size, const void *) = malloc_hook_ini; > -void *weak_variable (*__realloc_hook) > - (void *__ptr, size_t __size, const void *) > - = realloc_hook_ini; > -void *weak_variable (*__memalign_hook) > - (size_t __alignment, size_t __size, const void *) > - = memalign_hook_ini; > void weak_variable (*__after_morecore_hook) (void) = NULL; Moved, ok. > @@ -2065,6 +2041,9 @@ free_perturb (char *p, size_t n) > > #include > > +/* ----------------- Support for debugging hooks -------------------- */ > +#include "hooks.c" > + > /* ------------------- Support for multiple arenas -------------------- */ > #include "arena.c" > > @@ -2425,10 +2404,6 @@ do_check_malloc_state (mstate av) > #endif > > > -/* ----------------- Support for debugging hooks -------------------- */ > -#include "hooks.c" > - > - > /* ----------- Routines dealing with system allocation -------------- */ Ok. > @@ -3225,13 +3200,12 @@ __libc_malloc (size_t bytes) > mstate ar_ptr; > void *victim; > > - _Static_assert (PTRDIFF_MAX <= SIZE_MAX / 2, > - "PTRDIFF_MAX is not more than half of SIZE_MAX"); > + if (__malloc_initialized < 0) > + ptmalloc_init (); > + > + if (_malloc_debug_before (bytes, &victim, RETURN_ADDRESS (0))) > + return victim; > > - void *(*hook) (size_t, const void *) > - = atomic_forced_read (__malloc_hook); > - if (__builtin_expect (hook != NULL, 0)) > - return (*hook)(bytes, RETURN_ADDRESS (0)); Ok. > @@ -3292,13 +3266,11 @@ __libc_free (void *mem) > mstate ar_ptr; > mchunkptr p; /* chunk corresponding to mem */ > > - void (*hook) (void *, const void *) > - = atomic_forced_read (__free_hook); > - if (__builtin_expect (hook != NULL, 0)) > - { > - (*hook)(mem, RETURN_ADDRESS (0)); > - return; > - } > + if (__malloc_initialized < 0) > + ptmalloc_init (); > + > + if (_free_debug_before (mem, RETURN_ADDRESS (0))) > + return; Ok. > @@ -3351,10 +3323,11 @@ __libc_realloc (void *oldmem, size_t bytes) > > void *newp; /* chunk to return */ > > - void *(*hook) (void *, size_t, const void *) = > - atomic_forced_read (__realloc_hook); > - if (__builtin_expect (hook != NULL, 0)) > - return (*hook)(oldmem, bytes, RETURN_ADDRESS (0)); > + if (__malloc_initialized < 0) > + ptmalloc_init (); > + > + if (_realloc_debug_before (oldmem, bytes, &newp, RETURN_ADDRESS (0))) > + return newp; Ok. > @@ -3490,6 +3463,9 @@ void * > __libc_memalign (size_t alignment, size_t bytes) > { > void *address = RETURN_ADDRESS (0); > + if (__malloc_initialized < 0) > + ptmalloc_init (); > + > return _mid_memalign (alignment, bytes, address); > } Ok. > @@ -3499,10 +3475,8 @@ _mid_memalign (size_t alignment, size_t bytes, void *address) > mstate ar_ptr; > void *p; > > - void *(*hook) (size_t, size_t, const void *) = > - atomic_forced_read (__memalign_hook); > - if (__builtin_expect (hook != NULL, 0)) > - return (*hook)(alignment, bytes, address); > + if (_memalign_debug_before (alignment, bytes, &p, address)) > + return p; Ok. > @@ -3612,16 +3586,11 @@ __libc_calloc (size_t n, size_t elem_size) > > sz = bytes; > > - void *(*hook) (size_t, const void *) = > - atomic_forced_read (__malloc_hook); > - if (__builtin_expect (hook != NULL, 0)) > - { > - mem = (*hook)(sz, RETURN_ADDRESS (0)); > - if (mem == 0) > - return 0; > + if (__malloc_initialized < 0) > + ptmalloc_init (); > > - return memset (mem, 0, sz); > - } > + if (_calloc_debug_before (sz, &mem, RETURN_ADDRESS (0))) > + return mem; Ok. LGTM Reviewed-by: DJ Delorie