<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Aug 17, 2017 at 5:49 PM, Kenneth Graunke <span dir="ltr"><<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thursday, August 17, 2017 4:54:05 PM PDT Timothy Arceri wrote:<br>
><br>
> On 18/08/17 09:05, Kenneth Graunke wrote:<br>
> > On Thursday, August 17, 2017 10:22:15 AM PDT Jason Ekstrand wrote:<br>
> >> ---<br>
> >>   src/util/ralloc.c | 2 +-<br>
> >>   1 file changed, 1 insertion(+), 1 deletion(-)<br>
> >><br>
> >> diff --git a/src/util/ralloc.c b/src/util/ralloc.c<br>
> >> index bf46439..4015c7e 100644<br>
> >> --- a/src/util/ralloc.c<br>
> >> +++ b/src/util/ralloc.c<br>
> >> @@ -285,7 +285,7 @@ ralloc_steal(const void *new_ctx, void *ptr)<br>
> >>         return;<br>
> >><br>
> >>      info = get_header(ptr);<br>
> >> -   parent = get_header(new_ctx);<br>
> >> +   parent = new_ctx ? get_header(new_ctx) : NULL;<br>
> >><br>
> >>      unlink_block(info);<br>
> >><br>
> >><br>
> ><br>
> > This patch is:<br>
> > Reviewed-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>><br>
> ><br>
> > ralloc_adopt() doesn't properly handle NULL either, but frankly...<br>
> > reparenting an unknown set of children to the NULL context sounds like a<br>
> > recipe for leaks.  :)<br>
><br>
> I agree. I was wondering if we should create a special function for this:<br>
><br>
> ralloc_steal_to_NULL_ctx(void *ptr)<br>
><br>
> To void issues like:<br>
><br>
> ralloc_free(parent->child);<br>
> parent->child = NULL;<br>
><br>
> ... lots of code in between ...<br>
><br>
> ralloc_steal(parent->child, ptr);<br>
><br>
> ralloc_free(parent);<br>
><br>
> Feel free to tell me I'm being paranoid.<br>
<br>
</div></div>I think steal to NULL is fine, because you're only stealing one item, and<br>
you can easily hold on to a pointer and free it later.  Adopt is bad because<br>
it steals a whole bunch of things, so unless you had a reference to all of<br>
them, you've effectively leaked them.  And if you had such a list, you could<br>
just steal them all and not use adopt in the first place.<br>
<br>
I'm not really sure why steal-to-NULL is useful, but I guess it can be.<br></blockquote><div><br></div><div>The next patch makes us parent the nir_shader to the vtn_builder while we're doing SPIR-V -> NIR conversion.  At the end, we unparent the nir_shader, delete the vtn_builder, and return the shader.  if we error out, it means that all we have to do is delete the vtn_builder.  I also considered adding a "if (b->shader) ralloc_free(b->shader);" which would work just as wel.</div><div><br></div><div>--Jason<br></div></div></div></div>