[Mesa-dev] [PATCH 08/32] i965/fs: Fix stack allocation of fs_inst and stop stealing src array provided on construction.

[Francisco Jerez]

Matt Turner <mattst88 at gmail.com> writes:

> On Fri, Feb 6, 2015 at 6:42 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>> Using 'ralloc*(this, ...)' is wrong if the object has automatic
>> storage or was allocated through any other means.  Use normal dynamic
>> memory instead.
>> ---
>
> I don't see any places we were allocating an fs_inst via a mechanism
> other than ralloc. Does one exist, or is this commit in preparation
> for doing so?
>
The latter, you're right that right now all fs_inst allocations are done
using ralloc.

> I understand that in general "stealing" the source array might be
> somewhat unexpected, but as far as I know we do this with
> load_payload, where it seems pretty clear. Is this just an issue of
> taste?

I don't think it's just a matter of taste.  It actually took me hours of
debugging to find out what was corrupting random memory in my program.
The cause turned out to be that I was passing a stack-allocated array as
the src argument of LOAD_PAYLOAD() (which gives no indication of the
argument having such particular memory semantics), the array is then
silently mutated by fs_inst::init() and bound to fs_inst:src with no
consideration of what the caller had in mind to do with it afterwards.
Later on my original stack allocation goes out of scope and the sources
of the instruction end up getting filled with garbage (and worst-case
the stack could get corrupted if someone modifies the sources of the
instruction later on).

You might argue against stack-allocating the source array, but it's by
far the most convenient (just see the number of lines of code saved in
the other fs_inst constructors by allocating the sources in the stack,
which allows you to use aggregate initializer syntax), cache-local and
memory-efficient (as the array allocation is released as soon as it goes
out of scope rather than letting it leak until the end of the
compilation).  For these reasons it's likely to happen again to someone
else in the future, I already learnt the lesson the hard way.

The other reason against using ralloc to allocate fs_insts is that it's
ridiculously memory-inefficient.  Each fs_inst allocation done using
ralloc requires 80 bytes (96 bytes in debug mode) of extra space for the
two header structures (one for the fs_inst itself and another one for
the source array), that's almost as much as the fs_inst structure itself
(104 bytes), and the whole thing leaks until the compilation terminates
even if as it frequently happens the instruction is eliminated along the
way by some optimization pass.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150209/40a13428/attachment.sig>