Matt Turner <matts...@gmail.com> writes: > On Fri, Feb 6, 2015 at 6:42 AM, Francisco Jerez <curroje...@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.
pgpHS9K2ZL3jy.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
