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 [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id CDEBF3838023 for ; Thu, 24 Jun 2021 21:43:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CDEBF3838023 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-561-m03AGlcYOlSAt5OKCQ00vw-1; Thu, 24 Jun 2021 17:43:39 -0400 X-MC-Unique: m03AGlcYOlSAt5OKCQ00vw-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 4D3AA1084F4C; Thu, 24 Jun 2021 21:43:38 +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 B7D3E604CD; Thu, 24 Jun 2021 21:43:34 +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 15OLhXCh447870; Thu, 24 Jun 2021 17:43:33 -0400 From: DJ Delorie To: Siddhesh Poyarekar Cc: libc-alpha@sourceware.org, carlos@redhat.com, fweimer@redhat.com Subject: Re: [PATCH 3/8] glibc.malloc.check: Wean away from malloc hooks In-Reply-To: <20210624182312.236596-4-siddhesh@sourceware.org> Date: Thu, 24 Jun 2021 17:43:33 -0400 Message-ID: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-14.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=unavailable 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:43:43 -0000 Siddhesh Poyarekar writes: > Set up a new internal debugging hooks flag variable and migrate > glibc.malloc.check to it so that it no longer uses the malloc hooks. > diff --git a/malloc/hooks.c b/malloc/hooks.c > index 4960aafd08..77855801c8 100644 > --- a/malloc/hooks.c > +++ b/malloc/hooks.c > @@ -21,6 +21,8 @@ > corrupt pointer is detected: do nothing (0), print an error message > (1), or call abort() (2). */ > > +# include > + Ok. > @@ -29,6 +31,14 @@ > # define weak_variable weak_function > #endif > > +/* The internal malloc debugging hooks. */ > +enum malloc_debug_hooks > +{ > + MALLOC_NONE_HOOK = 0, > + MALLOC_CHECK_HOOK = 1 << 0, /* MALLOC_CHECK_ or glibc.malloc.check. */ > +}; > +static unsigned __malloc_debugging_hooks; > + Ok. > @@ -48,6 +58,24 @@ void *weak_variable (*__memalign_hook) > > static void ptmalloc_init (void); > > +static __always_inline bool > +__is_malloc_debug_enabled (enum malloc_debug_hooks flag) > +{ > + return __malloc_debugging_hooks & flag; > +} > + > +static __always_inline void > +__malloc_debug_enable (enum malloc_debug_hooks flag) > +{ > + __malloc_debugging_hooks |= flag; > +} > + > +static __always_inline void > +__malloc_debug_disable (enum malloc_debug_hooks flag) > +{ > + __malloc_debugging_hooks &= ~flag; > +} > + Ok. > @@ -63,6 +91,12 @@ _malloc_debug_before (size_t bytes, void **victimp, const void *address) > *victimp = (*hook)(bytes, address); > return true; > } > + > + if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))) > + { > + *victimp = malloc_check (bytes); > + return true; > + } > return false; > } Ok. Too bad there isn't a way to tag the *function* as likely/unlikely, rather than the *calls*. > @@ -76,6 +110,12 @@ _free_debug_before (void *mem, const void *address) > (*hook)(mem, address); > return true; > } > + > + if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))) > + { > + free_check (mem); > + return true; > + } > return false; > } Ok. > @@ -91,6 +131,12 @@ _realloc_debug_before (void *oldmem, size_t bytes, void **victimp, > return true; > } > > + if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))) > + { > + *victimp = realloc_check (oldmem, bytes); > + return true; > + } > + > return false; > } Ok. > @@ -105,6 +151,13 @@ _memalign_debug_before (size_t alignment, size_t bytes, void **victimp, > *victimp = (*hook)(alignment, bytes, address); > return true; > } > + > + if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))) > + { > + *victimp = memalign_check (alignment, bytes); > + return true; > + } > + > return false; > } Ok. > @@ -120,6 +173,14 @@ _calloc_debug_before (size_t bytes, void **victimp, const void *address) > memset (*victimp, 0, bytes); > return true; > } > + > + if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))) > + { > + *victimp = malloc_check (bytes); > + if (*victimp != NULL) > + memset (*victimp, 0, bytes); > + return true; > + } > return false; > } Ok. > @@ -195,7 +256,7 @@ malloc_set_state (void *msptr) > __realloc_hook = NULL; > __free_hook = NULL; > __memalign_hook = NULL; > - using_malloc_checking = 0; > + __malloc_debug_disable (MALLOC_CHECK_HOOK); Ok. > diff --git a/malloc/malloc-check.c b/malloc/malloc-check.c > index dcab880510..a85e519498 100644 > --- a/malloc/malloc-check.c > +++ b/malloc/malloc-check.c > @@ -18,20 +18,6 @@ > not, see . */ > > > -/* Whether we are using malloc checking. */ > -static int using_malloc_checking; > - > -/* Activate a standard set of debugging hooks. */ > -void > -__malloc_check_init (void) > -{ > - using_malloc_checking = 1; > - __malloc_hook = malloc_check; > - __free_hook = free_check; > - __realloc_hook = realloc_check; > - __memalign_hook = memalign_check; > -} > - Ok. > @@ -69,7 +55,7 @@ malloc_check_get_size (mchunkptr p) > unsigned char c; > unsigned char magic = magicbyte (p); > > - assert (using_malloc_checking == 1); > + assert (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)); Ok. > @@ -203,7 +189,7 @@ top_check (void) > } > > static void * > -malloc_check (size_t sz, const void *caller) > +malloc_check (size_t sz) Ok. > @@ -222,7 +208,7 @@ malloc_check (size_t sz, const void *caller) > } > > static void > -free_check (void *mem, const void *caller) > +free_check (void *mem) Ok. > @@ -256,7 +242,7 @@ free_check (void *mem, const void *caller) > } > > static void * > -realloc_check (void *oldmem, size_t bytes, const void *caller) > +realloc_check (void *oldmem, size_t bytes) Ok. > @@ -269,11 +255,11 @@ realloc_check (void *oldmem, size_t bytes, const void *caller) > return NULL; > } > if (oldmem == 0) > - return malloc_check (bytes, NULL); > + return malloc_check (bytes); Ok. > if (bytes == 0) > { > - free_check (oldmem, NULL); > + free_check (oldmem); Ok. > @@ -348,12 +334,12 @@ invert: > } > > static void * > -memalign_check (size_t alignment, size_t bytes, const void *caller) > +memalign_check (size_t alignment, size_t bytes) > { > void *mem; > > if (alignment <= MALLOC_ALIGNMENT) > - return malloc_check (bytes, NULL); > + return malloc_check (bytes); Ok > diff --git a/malloc/malloc.c b/malloc/malloc.c > index 75ca6ec3f0..60753446a1 100644 > --- a/malloc/malloc.c > +++ b/malloc/malloc.c > @@ -1124,13 +1124,6 @@ static void munmap_chunk(mchunkptr p); > static mchunkptr mremap_chunk(mchunkptr p, size_t new_size); > #endif > > -static void* malloc_check(size_t sz, const void *caller); > -static void free_check(void* mem, const void *caller); > -static void* realloc_check(void* oldmem, size_t bytes, > - const void *caller); > -static void* memalign_check(size_t alignment, size_t bytes, > - const void *caller); Ok. > @@ -5078,7 +5071,7 @@ musable (void *mem) > > p = mem2chunk (mem); > > - if (__builtin_expect (using_malloc_checking == 1, 0)) > + if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))) > return malloc_check_get_size (p); Ok. > if (chunk_is_mmapped (p)) > diff --git a/malloc/arena.c b/malloc/arena.c > index 1861d20006..357a3b0b30 100644 > --- a/malloc/arena.c > +++ b/malloc/arena.c > @@ -210,7 +210,7 @@ TUNABLE_CALLBACK (set_mallopt_check) (tunable_val_t *valp) > { > int32_t value = (int32_t) valp->numval; > if (value != 0) > - __malloc_check_init (); > + __malloc_debug_enable (MALLOC_CHECK_HOOK); > } Ok. > # define TUNABLE_CALLBACK_FNDECL(__name, __type) \ > @@ -400,7 +400,7 @@ ptmalloc_init (void) > } > } > if (s && s[0] != '\0' && s[0] != '0') > - __malloc_check_init (); > + __malloc_debug_enable (MALLOC_CHECK_HOOK); > #endif Ok. LGTM Reviewed-by: DJ Delorie