[Mesa-dev] [PATCH] util/ralloc: Remove double zero'ing of rzalloc buffers

[Kenneth Graunke]

On Friday, January 22, 2016 12:09:18 PM PST Nicolai Hähnle wrote:
> On 22.01.2016 02:53, Jordan Justen wrote:
> > Juha-Pekka found this back in May 2015:
> > <1430915727-28677-1-git-send-email-juhapekka.heikkila at gmail.com>
> >
> >  From the discussion, obviously it would be preferable to make
> > ralloc_size no longer return zeroed memory, but Juha-Pekka found that
> > it would break Mesa.
> >
> > For now, let's point out the flaw, and stop doing the double zeroing
> > of rzalloc buffers.
> >
> > Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> > Cc: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
> > Cc: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >
> >   For a release build, I saw the code size shrink by 64 bytes.
> >
> >   src/util/ralloc.c | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/util/ralloc.c b/src/util/ralloc.c
> > index 6d4032b..24c1eee 100644
> > --- a/src/util/ralloc.c
> > +++ b/src/util/ralloc.c
> > @@ -49,6 +49,14 @@ _CRTIMP int _vscprintf(const char *format, va_list 
argptr);
> >   #endif
> >   #endif
> >
> > +/* ralloc_size has always used calloc to allocate memory. This has 
allowed
> > + * code using ralloc_size to depend on always receiving a cleared buffer.
> > + *
> > + * FIXME: Clean up the code base to allow this to be set to false, and 
then
> > + * remove it altogether.
> > + */
> > +static const bool always_allocate_zeroed_memory = true;
> > +
> >   #define CANARY 0x5A1106
> >
> >   struct ralloc_header
> > @@ -110,7 +118,10 @@ ralloc_context(const void *ctx)
> >   void *
> >   ralloc_size(const void *ctx, size_t size)
> >   {
> > -   void *block = calloc(1, size + sizeof(ralloc_header));
> > +   void *block =
> > +      always_allocate_zeroed_memory ?
> > +      calloc(1, size + sizeof(ralloc_header)) :
> > +      malloc(size + sizeof(ralloc_header));
> 
> There's an integer overflow here which would be good to fix.

Please explain?  ralloc_header is 40-44 bytes - the only way this will
overflow is if you asked for an absurd amount of memory (already near
the max value of size_t).  And, if you did, I'm not sure what we're
supposed to do about it...

> Since it was there already in the older version, the patch is
> 
> Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
> 
> as is.
> 
> >      ralloc_header *info;
> >      ralloc_header *parent;
> >
> > @@ -132,7 +143,7 @@ void *
> >   rzalloc_size(const void *ctx, size_t size)
> >   {
> >      void *ptr = ralloc_size(ctx, size);
> > -   if (likely(ptr != NULL))
> > +   if (!always_allocate_zeroed_memory && likely(ptr != NULL))
> >         memset(ptr, 0, size);
> >      return ptr;
> >   }
> >
> 

Dropping the memset seems reasonable.  I would prefer it if we simply
moved the contents of ralloc_size into rzalloc_size, and made
ralloc_size call rzalloc_size with a comment.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20160122/35fbf479/attachment.sig>