From patchwork Thu Sep 13 00:30:19 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 29352 Received: (qmail 83779 invoked by alias); 13 Sep 2018 00:31:17 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 83663 invoked by uid 89); 13 Sep 2018 00:30:55 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.0 required=5.0 tests=BAYES_20, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS, TIME_LIMIT_EXCEEDED autolearn=unavailable version=3.3.2 spammy=termination, freeing, highlight, sk:symfile X-HELO: mail-wm0-f65.google.com Received: from mail-wm0-f65.google.com (HELO mail-wm0-f65.google.com) (74.125.82.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 13 Sep 2018 00:30:31 +0000 Received: by mail-wm0-f65.google.com with SMTP id 207-v6so4222068wme.5 for ; Wed, 12 Sep 2018 17:30:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=gY/XLB6kX8b7UYRN06BI6awKj1GQFt/NzKI5TNnr7vA=; b=cXzEQl0rozZkZPzNPz22EJ5nB3pr2bZwKNP2Dq7ZHUrj78EE5rOCyx8hRh2Bdk6KmQ 3CiwZ8G63PFqiXR8p1rUE4dVzhTd0IPujX7yHePvmkOioNIiKJdLkkejBuUr+rlULOdV JNoeyfnJAPMLhzO8DN1e2qNIqX4zYQZYTlB02Q4ZgX3v5h86FYJVDFrGDmOJ/As7Bp/Y +pQtZgr5pMxSps8AUN8SbJbedm8BEmS4zUmqei7xPpM7ntPjXCL+QNeMjeD7z1on2wzB zxepJJyp/6zm3kYN4I5l6mx4bGPKs+KrUCn4wmrvIiYXWjgnsNGQpJexkzeBAQ/iVvWK 4weQ== Return-Path: Received: from localhost ([2a00:23c5:3e84:8500:cb00:cea5:fbe4:a50]) by smtp.gmail.com with ESMTPSA id s13-v6sm3236147wrq.39.2018.09.12.17.30.19 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 12 Sep 2018 17:30:20 -0700 (PDT) Date: Thu, 13 Sep 2018 01:30:19 +0100 From: Andrew Burgess To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: Don't leak memory with TYPE_ALLOC / TYPE_ZALLOC Message-ID: <20180913003018.GB5952@embecosm.com> References: <20180911141147.14107-1-andrew.burgess@embecosm.com> <87lg87rcq3.fsf@tromey.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <87lg87rcq3.fsf@tromey.com> X-Fortune: Usage: fortune -P [] -a [xsz] [Q: [file]] [rKe9] -v6[+] dataspec ... inputdir X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes * Tom Tromey [2018-09-11 14:38:44 -0600]: > >>>>> "Andrew" == Andrew Burgess writes: > > Andrew> I ran into the following issue after runnig valgrind on GDB. > Andrew> Calling TYPE_ALLOC or TYPE_ZALLOC currently allocates memory from > Andrew> either the objfile obstack or by using malloc. The problem with this > Andrew> is that types are allocated either on the objfile obstack, or on the > Andrew> gdbarch obstack. > > Thank you for the patch. I found the analysis very clear, so thanks for > that too. > > Andrew> This commit ensures that auxiliary type data is allocated from the > Andrew> same obstack as the type itself, which should reduce leaked memory. > > I'm curious whether this is something you intend to do. gdb has many > things that are allocated for the lifetime of the process... No. This was more an observation that some allocated memory appeared to be getting lost. From there I went to read the code, and the comment on TYPE_ALLOC (about either using the objfile obstack OR malloc) seemed wrong (as types are on objfile or gdbarch obstack). Given I was looking at the code and the fix seemed obvious.... > > Andrew> The one problem case that I found with this change was in eval.c, > Andrew> where in one place we allocate a local type structure, and then used > Andrew> TYPE_ZALLOC to allocate some space for the type. This local type is > Andrew> neither object file owned, nor gdbarch owned, and so the updated > Andrew> TYPE_ALLOC code is unable to find an objstack to allocate space on. > > Andrew> My proposed solution for this issue is that the space should be > Andrew> allocated with a direct call to xzalloc. We could extend TYPE_ALLOC > Andrew> to check for type->gdbarch being null, and then fall back to a direct > Andrew> call to xzalloc, however, I think making the rare case of a local type > Andrew> requiring special handling is not a bad thing, this serves to > Andrew> highlight that clearing up the memory will require special handling > Andrew> too. > > I went and refreshed my memory of this area. > > My first thought was that this would be dangerous because -- surely -- > there was code allocating a type on the heap and manually managing it. > I was remembering Python, but I think we didn't do this because we > bailed on the whole "type GC" idea. So, there no problems that I could > find. > > I suppose if the type from fake_method ever ends up somewhere it > shouldn't -- passed to something that uses TYPE_ALLOC -- gdb will crash. > > I wonder if get_type_arch should assert that the result is not NULL. New patch does this. > > Andrew> * gdbtypes.h (TYPE_ALLOC): Allocate space on either the objfile > Andrew> obstack, or the gdbarch obstack. > Andrew> (TYPE_ZALLOC): Rewrite using TYPE_ALLOC. > Andrew> * eval.c (fake_method::fake_method): Call xzalloc directly for a > Andrew> type that is neither object file owned, nor gdbarch owned. > > Andrew> +/* See comment on TYPE_ALLOC. */ > Andrew> + > Andrew> +#define TYPE_ZALLOC(t,size) (memset (TYPE_ALLOC (t, size), 0, size)) > > Would you mind seeing what happens if you use obstack_zalloc here > instead? It seems like that would give us some extra compile-time > safety. If it fails, that would be interesting. Unless I'm missing something (very possible) there's no suitable obstack_zalloc to use here. There's this function: template static inline T* obstack_zalloc (struct obstack *ob); However, that requires an actual type, and all I have is a size at this point. Without much wider changes I don't think I can use that in TYPE_ZALLOC. Please let me know if I've missed something. > I think the eval.c change should have a comment explaining why > TYPE_ZALLOC is not used. Done. > This is ok with that nit changed. If obstack_zalloc works the I think > that variant would be better. So, once I started picking at this thread I found a few more cases that could be fixed in gdbtypes.c, these are included in the revised patch below. I know that there's more that could be done in this area, but I don't want this patch to grow too much. Thanks, Andrew --- gdb: Don't leak memory with TYPE_ALLOC / TYPE_ZALLOC This patch started as an observation from valgrind that GDB appeared to be loosing track of some memory associated with types. An example valgrind stack would be: 24 bytes in 1 blocks are possibly lost in loss record 419 of 5,361 at 0x4C2EA1E: calloc (vg_replace_malloc.c:711) by 0x623D26: xcalloc (common-utils.c:85) by 0x623D65: xzalloc(unsigned long) (common-utils.c:95) by 0x72A066: make_function_type(type*, type**) (gdbtypes.c:510) by 0x72A098: lookup_function_type(type*) (gdbtypes.c:521) by 0x73635D: gdbtypes_post_init(gdbarch*) (gdbtypes.c:5439) by 0x727590: gdbarch_data(gdbarch*, gdbarch_data*) (gdbarch.c:5230) by 0x735B99: builtin_type(gdbarch*) (gdbtypes.c:5313) by 0x514D95: elf_rel_plt_read(minimal_symbol_reader&, objfile*, bfd_symbol**) (elfread.c:542) by 0x51662F: elf_read_minimal_symbols(objfile*, int, elfinfo const*) (elfread.c:1121) by 0x5168A5: elf_symfile_read(objfile*, enum_flags) (elfread.c:1207) by 0x8520F5: read_symbols(objfile*, enum_flags) (symfile.c:794) When we look in make_function_type we find a call to TYPE_ZALLOC (inside the INIT_FUNC_SPECIFIC macro). It is this call to TYPE_ZALLOC that is allocating memory with xcalloc, that is then getting lost. The problem is tht calling TYPE_ALLOC or TYPE_ZALLOC currently allocates memory from either the objfile obstack or by using malloc. The problem with this is that types are allocated either on the objfile obstack, or on the gdbarch obstack. As a result, if we discard a type associated with an objfile then auxiliary data allocated with TYPE_(Z)ALLOC will be correctly discarded. But, if we were ever to discard a gdbarch then any auxiliary type data would be leaked. Right now there are very few places in GDB where a gdbarch is ever discarded, but it shouldn't hurt to close down these bugs as we spot them. This commit ensures that auxiliary type data is allocated from the same obstack as the type itself, which should reduce leaked memory. The one problem case that I found with this change was in eval.c, where in one place we allocate a local type structure, and then used TYPE_ZALLOC to allocate some space for the type. This local type is neither object file owned, nor gdbarch owned, and so the updated TYPE_ALLOC code is unable to find an objstack to allocate space on. My proposed solution for this issue is that the space should be allocated with a direct call to xzalloc. We could extend TYPE_ALLOC to check for type->gdbarch being null, and then fall back to a direct call to xzalloc, however, I think that making this rare case of a local type require special handling is not a bad thing, this serves to highlight that clearing up the memory will require special handling too. This special case of a local type is interesting as the types owner field (contained within the main_type) is completely null. While reflecting on this I looked at how types use the get_type_arch function. It seems clear that, based on how this is used, it is never intended that null will be returned from this function. This only goes to reinforce, how locally alloctaed types, with no owner, are both special, and need to be handled carefully. To help spot errors earlier, I added an assert into get_type_arch that the returned arch is not null. Inside gdbarch.c I found a few other places where auxiliary type data was being allocated directly on the heap rather than on the types obstack. I have fixed these to call TYPE_ALLOC now. Finally, it is worth noting that as we don't clean up our gdbarch objects yet, then this will not make much of an impact on the amount of memory reported as lost at program termination time. Memory allocated for auxiliary type information is still not freed, however, it is now on the correct obstack. If we do ever start freeing our gdbarch structures then the associated type data will be cleaned up correctly. Tested on X86-64 GNU/Linux with no regressions. gdb/ChangeLog: * eval.c (fake_method::fake_method): Call xzalloc directly for a type that is neither object file owned, nor gdbarch owned. * gdbtypes.c (get_type_gdbarch): Add an assert that returned gdbarch is non-NULL. (alloc_type_instance): Allocate non-objfile owned types on the gdbarch obstack. (copy_type_recursive): Allocate TYPE_FIELDS and TYPE_RANGE_DATA using TYPE_ALLOC to ensure memory is allocated on the correct obstack. * gdbtypes.h (TYPE_ALLOC): Allocate space on either the objfile obstack, or the gdbarch obstack. (TYPE_ZALLOC): Rewrite using TYPE_ALLOC. --- gdb/ChangeLog | 15 +++++++++++++++ gdb/eval.c | 6 +++++- gdb/gdbtypes.c | 21 ++++++++++++++++----- gdb/gdbtypes.h | 37 +++++++++++++++++++++---------------- 4 files changed, 57 insertions(+), 22 deletions(-) diff --git a/gdb/eval.c b/gdb/eval.c index 2e08e9355f5..f9349e61b38 100644 --- a/gdb/eval.c +++ b/gdb/eval.c @@ -683,9 +683,13 @@ fake_method::fake_method (type_instance_flags flags, } } + /* We don't use TYPE_ZALLOC here to allocate space as TYPE is owned by + neither an objfile nor a gdbarch. As a result we must manually + allocate memory for auxiliary fields, and free the memory ourselves + when we are done with it. */ TYPE_NFIELDS (type) = num_types; TYPE_FIELDS (type) = (struct field *) - TYPE_ZALLOC (type, sizeof (struct field) * num_types); + xzalloc (sizeof (struct field) * num_types); while (num_types-- > 0) TYPE_FIELD_TYPE (type, num_types) = param_types[num_types]; diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c index 05bf7b1134e..62d44217e29 100644 --- a/gdb/gdbtypes.c +++ b/gdb/gdbtypes.c @@ -233,10 +233,19 @@ alloc_type_copy (const struct type *type) struct gdbarch * get_type_arch (const struct type *type) { + struct gdbarch *arch; + if (TYPE_OBJFILE_OWNED (type)) - return get_objfile_arch (TYPE_OWNER (type).objfile); + arch = get_objfile_arch (TYPE_OWNER (type).objfile); else - return TYPE_OWNER (type).gdbarch; + arch = TYPE_OWNER (type).gdbarch; + + /* The ARCH can be NULL if TYPE is associated with neither an objfile nor + a gdbarch, however, this is very rare, and even then, in most cases + that get_type_arch is called, we assume that a non-NULL value is + returned. */ + gdb_assert (arch != NULL); + return arch; } /* See gdbtypes.h. */ @@ -277,7 +286,7 @@ alloc_type_instance (struct type *oldtype) /* Allocate the structure. */ if (! TYPE_OBJFILE_OWNED (oldtype)) - type = XCNEW (struct type); + type = GDBARCH_OBSTACK_ZALLOC (get_type_arch (oldtype), struct type); else type = OBSTACK_ZALLOC (&TYPE_OBJFILE (oldtype)->objfile_obstack, struct type); @@ -4903,7 +4912,8 @@ copy_type_recursive (struct objfile *objfile, int i, nfields; nfields = TYPE_NFIELDS (type); - TYPE_FIELDS (new_type) = XCNEWVEC (struct field, nfields); + TYPE_FIELDS (new_type) = (struct field *) + TYPE_ALLOC (new_type, nfields * sizeof (struct field)); for (i = 0; i < nfields; i++) { TYPE_FIELD_ARTIFICIAL (new_type, i) = @@ -4946,7 +4956,8 @@ copy_type_recursive (struct objfile *objfile, /* For range types, copy the bounds information. */ if (TYPE_CODE (type) == TYPE_CODE_RANGE) { - TYPE_RANGE_DATA (new_type) = XNEW (struct range_bounds); + TYPE_RANGE_DATA (new_type) = (struct range_bounds *) + TYPE_ALLOC (new_type, sizeof (struct range_bounds)); *TYPE_RANGE_DATA (new_type) = *TYPE_RANGE_DATA (type); } diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h index eb7c365b71d..32b58dcc618 100644 --- a/gdb/gdbtypes.h +++ b/gdb/gdbtypes.h @@ -49,6 +49,7 @@ #include "common/enum-flags.h" #include "common/underlying.h" #include "common/print-utils.h" +#include "gdbarch.h" /* Forward declarations for prototypes. */ struct field; @@ -1717,26 +1718,30 @@ extern const struct floatformat *floatformats_vax_d[BFD_ENDIAN_UNKNOWN]; extern const struct floatformat *floatformats_ibm_long_double[BFD_ENDIAN_UNKNOWN]; -/* * Allocate space for storing data associated with a particular +/* Allocate space for storing data associated with a particular type. We ensure that the space is allocated using the same mechanism that was used to allocate the space for the type structure itself. I.e. if the type is on an objfile's objfile_obstack, then the space for data associated with that type - will also be allocated on the objfile_obstack. If the type is not - associated with any particular objfile (such as builtin types), - then the data space will be allocated with xmalloc, the same as for - the type structure. */ - -#define TYPE_ALLOC(t,size) \ - (TYPE_OBJFILE_OWNED (t) \ - ? obstack_alloc (&TYPE_OBJFILE (t) -> objfile_obstack, size) \ - : xmalloc (size)) - -#define TYPE_ZALLOC(t,size) \ - (TYPE_OBJFILE_OWNED (t) \ - ? memset (obstack_alloc (&TYPE_OBJFILE (t)->objfile_obstack, size), \ - 0, size) \ - : xzalloc (size)) + will also be allocated on the objfile_obstack. If the type is + associated with a gdbarch, then the space for data associated with that + type will also be allocated on the gdbarch_obstack. + + If a type is not associated with neither an objfile or a gdbarch then + you should not use this macro to allocate space for data, instead you + should call xmalloc directly, and ensure the memory is correctly freed + when it is no longer needed. */ + +#define TYPE_ALLOC(t,size) \ + (obstack_alloc ((TYPE_OBJFILE_OWNED (t) \ + ? &TYPE_OBJFILE (t)->objfile_obstack \ + : gdbarch_obstack (TYPE_OWNER (t).gdbarch)), \ + size)) + + +/* See comment on TYPE_ALLOC. */ + +#define TYPE_ZALLOC(t,size) (memset (TYPE_ALLOC (t, size), 0, size)) /* Use alloc_type to allocate a type owned by an objfile. Use alloc_type_arch to allocate a type owned by an architecture. Use