From: Richard Biener <richard.guenther@gmail.com>
To: Trevor Saunders <tbsaunde@tbsaunde.org>
Cc: tbsaunde+gcc@tbsaunde.org, GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH 01/13] improve safety of freeing bitmaps
Date: Wed, 10 May 2017 11:11:00 -0000 [thread overview]
Message-ID: <CAFiYyc05z9dQYpndPYNi=eiH+Ua_nv6UP3yqbY2G6rLm1DXP0A@mail.gmail.com> (raw)
In-Reply-To: <20170510105104.n2dqgl4tfpnvay27@ball>
On Wed, May 10, 2017 at 12:52 PM, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:
> On Wed, May 10, 2017 at 10:14:17AM +0200, Richard Biener wrote:
>> On Tue, May 9, 2017 at 10:52 PM, <tbsaunde+gcc@tbsaunde.org> wrote:
>> > From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org>
>> >
>> > There's two groups of changes here, first taking a sbitmap &, so that we
>> > can assign null to the pointer after freeing the sbitmap to prevent use
>> > after free through that pointer. Second we define overloads of
>> > sbitmap_free and bitmap_free taking auto_sbitmap and auto_bitmap
>> > respectively, so that you can't double free the bitmap owned by a
>> > auto_{s,}bitmap.
>>
>> Looks good - but what do you need the void *& overload for?! That at least
>> needs a comment.
>
> yeah, its gross, I put it in to be compatible with the previous macro.
> The first problem with removing it is that cfgexpand.c:663 and
> presumably other places do BITMAP_FREE(bb->aux) which of course
> depends on being able to pass in a void *. I'll add a comment and try
> and look into removing it.
Yeah, please remove it by fixing callers instead.
Richard.
> Trev
>
>>
>> Richard.
>>
>> > gcc/ChangeLog:
>> >
>> > 2017-05-09 Trevor Saunders <tbsaunde+gcc@tbsaunde.org>
>> >
>> > * bitmap.h (BITMAP_FREE): Convert from macro to inline function
>> > and add overloaded decl for auto_bitmap.
>> > * sbitmap.h (inline void sbitmap_free): Add overload for
>> > auto_sbitmap, and change sbitmap to point to null.
>> > ---
>> > gcc/bitmap.h | 21 +++++++++++++++++++--
>> > gcc/sbitmap.h | 7 ++++++-
>> > 2 files changed, 25 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/gcc/bitmap.h b/gcc/bitmap.h
>> > index f158b447357..7508239cff9 100644
>> > --- a/gcc/bitmap.h
>> > +++ b/gcc/bitmap.h
>> > @@ -129,6 +129,8 @@ along with GCC; see the file COPYING3. If not see
>> >
>> > #include "obstack.h"
>> >
>> > + class auto_bitmap;
>> > +
>> > /* Bitmap memory usage. */
>> > struct bitmap_usage: public mem_usage
>> > {
>> > @@ -372,8 +374,23 @@ extern hashval_t bitmap_hash (const_bitmap);
>> > #define BITMAP_GGC_ALLOC() bitmap_gc_alloc ()
>> >
>> > /* Do any cleanup needed on a bitmap when it is no longer used. */
>> > -#define BITMAP_FREE(BITMAP) \
>> > - ((void) (bitmap_obstack_free ((bitmap) BITMAP), (BITMAP) = (bitmap) NULL))
>> > +inline void
>> > +BITMAP_FREE (bitmap &b)
>> > +{
>> > + bitmap_obstack_free ((bitmap) b);
>> > + b = NULL;
>> > +}
>> > +
>> > +inline void
>> > +BITMAP_FREE (void *&b)
>> > +{
>> > + bitmap_obstack_free ((bitmap) b);
>> > + b = NULL;
>> > +}
>> > +
>> > +/* Intentionally unimplemented to ensure it is never called with an
>> > + auto_bitmap argument. */
>> > +void BITMAP_FREE (auto_bitmap);
>> >
>> > /* Iterator for bitmaps. */
>> >
>> > diff --git a/gcc/sbitmap.h b/gcc/sbitmap.h
>> > index ce4d27d927c..cba0452cdb9 100644
>> > --- a/gcc/sbitmap.h
>> > +++ b/gcc/sbitmap.h
>> > @@ -82,6 +82,8 @@ along with GCC; see the file COPYING3. If not see
>> > #define SBITMAP_ELT_BITS (HOST_BITS_PER_WIDEST_FAST_INT * 1u)
>> > #define SBITMAP_ELT_TYPE unsigned HOST_WIDEST_FAST_INT
>> >
>> > +class auto_sbitmap;
>> > +
>> > struct simple_bitmap_def
>> > {
>> > unsigned int n_bits; /* Number of bits. */
>> > @@ -208,11 +210,14 @@ bmp_iter_next (sbitmap_iterator *i, unsigned *bit_no ATTRIBUTE_UNUSED)
>> > bmp_iter_next (&(ITER), &(BITNUM)))
>> > #endif
>> >
>> > -inline void sbitmap_free (sbitmap map)
>> > +inline void sbitmap_free (sbitmap &map)
>> > {
>> > free (map);
>> > + map = NULL;
>> > }
>> >
>> > +void sbitmap_free (auto_sbitmap);
>> > +
>> > inline void sbitmap_vector_free (sbitmap * vec)
>> > {
>> > free (vec);
>> > --
>> > 2.11.0
>> >
next prev parent reply other threads:[~2017-05-10 10:57 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-09 20:53 [PATCH 00/13] misc data structure stuff tbsaunde+gcc
2017-05-09 20:53 ` [PATCH 06/13] replace some manual stacks with auto_vec tbsaunde+gcc
2017-05-10 8:26 ` Richard Biener
2017-05-09 20:53 ` [PATCH 05/13] allow constructing a auto_vec with a preallocation, and a possibly larger actual allocation size tbsaunde+gcc
2017-05-10 6:58 ` Richard Sandiford
2017-05-11 7:50 ` Trevor Saunders
2017-05-11 8:18 ` Richard Biener
2017-05-11 8:23 ` Trevor Saunders
2017-05-11 9:04 ` Richard Sandiford
2017-05-09 20:53 ` [PATCH 03/13] store the bitmap_head within the auto_bitmap tbsaunde+gcc
2017-05-10 8:25 ` Richard Biener
2017-05-09 20:53 ` [PATCH 02/13] improve bitmap / sbitmap compatability of bitmap_set_bit tbsaunde+gcc
2017-05-10 6:54 ` Richard Sandiford
2017-05-11 8:01 ` Trevor Saunders
2017-05-09 20:53 ` [PATCH 01/13] improve safety of freeing bitmaps tbsaunde+gcc
2017-05-10 8:15 ` Richard Biener
2017-05-10 10:55 ` Trevor Saunders
2017-05-10 11:11 ` Richard Biener [this message]
2017-05-09 20:53 ` [PATCH 12/13] make depth_first_search_ds a class tbsaunde+gcc
2017-05-10 8:29 ` Richard Biener
2017-05-09 20:53 ` [PATCH 09/13] use auto_bitmap more with alternate obstacks tbsaunde+gcc
2017-05-10 8:31 ` Richard Biener
2017-05-09 20:53 ` [PATCH 13/13] make inverted_post_order_compute() operate on a vec tbsaunde+gcc
2017-05-10 8:44 ` Richard Biener
2017-05-09 20:53 ` [PATCH 04/13] allow auto_bitmap to use other bitmap obstacks tbsaunde+gcc
2017-05-10 8:27 ` Richard Biener
2017-05-09 20:53 ` [PATCH 10/13] make a member an auto_sbitmap tbsaunde+gcc
2017-05-10 8:26 ` Richard Biener
2017-05-09 20:53 ` [PATCH 08/13] move several bitmaps from gc memory to the default obstack and use auto_bitmap tbsaunde+gcc
2017-05-10 8:26 ` Richard Biener
2017-05-09 20:53 ` [PATCH 07/13] use auto_bitmap more tbsaunde+gcc
2017-05-10 8:28 ` Richard Biener
2017-05-09 20:55 ` [PATCH 11/13] make more vars auto_sbitmaps tbsaunde+gcc
2017-05-10 8:27 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFiYyc05z9dQYpndPYNi=eiH+Ua_nv6UP3yqbY2G6rLm1DXP0A@mail.gmail.com' \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=tbsaunde+gcc@tbsaunde.org \
--cc=tbsaunde@tbsaunde.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).