<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Oct 27, 2016 at 2:47 PM, Timothy Arceri <<a href="mailto:timothy.arceri@collabora.com" target="_blank">timothy.arceri@collabora.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Thu, 2016-10-27 at 13:57 -0700, Jason Ekstrand wrote: > On Thu, Oct 27, 2016 at 1:30 PM, Eric Anholt <<a href="mailto:eric@anholt.net">eric@anholt.net</a>> wrote: > > Fixes use-after-free-caused segfaults in the tgsi_to_nir() path. > > --- > > > > I don't like the boolean I added here, any better plans, though? I > > haven't fully piglited this, but it's enough to get gears to not > > crash > > on master (it at least touches the GLSL to NIR and TGSI to NIR > > paths.) > > I don't either. Let's consider the options... > > Would it be better to simply make nir_shader_create() always allocate > a shader_info and just memcpy old one if provided or zero it out if > no shader info is provided? Having a pointer which may or may not be > parented to the nir_shader is a disaster waiting to happen. Since > the one provided to nir_shader_create() isn't required to be > ralloc'd, we can't even use ralloc_parent() to check. > > Another option would be to say that the NIR shader doesn't own the > info ever. Maybe even get rid of the shader_info pointer in > nir_shader. It would probably mean manually passing the shader_info > around a few more places, but it would work. > > Tim, thoughts? Hi guys, Options: 1) Make nir_shader_create() always allocate a shader_info and just memcpy old one if provided or zero it out if no shader info is provided. 2) Detach shader_info from nir_shader. Downside we need to pass both around. This looks like it will be most painful in Vulkan were nir_shader seems to be the highest level stucture we pass around and we don't have something like gl_program in GLSL where we can get access to them both. </blockquote><div> </div><div>I don't think it's all that difficult in Vulkan. We just need to make anv_shader_compile_to_nir and anv_pipeline_compile take a shader_info. We already do this for prog_data so it's really not a big deal. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 3) Allocate the info to attach to the shader being produced, and then assert that nir_shader_create() always gets an info. 4) Switch the order. Have shader_info (we could rename it if that would help make more sense) contain a pointer to nir_shader. This is how we do things in GLSL IR, gl_program already contains a pointer to nir. This way the shader_info is created and managed by the implementation rather than nir. Personally I think I like option 4. I suggested this originally but Jason wasn't sold at the time. However looking at the problem again this make the most sense to me. What do you guys think?</blockquote><div> </div><div>Without knowing for 100% sure what the code would look like, I think my preference would be (2). It would let us keep the shader_info struct in gl_program on GL and do whatever we want in Vulkan/blorp. One of the things I don't like today is that we have this shader_info in the shader which may or may not be correct depending on where it came from and whether or not we've run nir_gather_info. If, instead, nir_gather_info is simply a pass that takes a nir_shader and a shader_info and fills out the shader_info, the data flow is a bit more clear. </div><div>(1) or (3) are fairly simple but they seem fairly fragile to me and I don't think I like them long-term. I'm still not sold on (4) because I don't think that having a struct which just wraps up two other structs really gains us anything beyond having them separate. I think I could be ok with it, but I'm still not sold. Unfortunately, both (3) and (4) are going to require a decent bit of typing. </div><div>Anholt? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="HOEnZb"><div class="h5"> > > > src/compiler/nir/nir.c | 2 ++ > > src/compiler/nir/nir.h | 2 ++ > > src/compiler/nir/nir_clone.c | 14 +++++++++++++- > > src/compiler/nir/nir_sweep.c | 3 +++ > > 4 files changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/src/compiler/nir/nir.c b/src/compiler/nir/nir.c > > index 09aad57e87f8..1448837d8fd1 100644 > > --- a/src/compiler/nir/nir.c > > +++ b/src/compiler/nir/nir.c > > @@ -45,6 +45,8 @@ nir_shader_create(void *mem_ctx, > > shader->options = options; > > > > shader->info = si ? si : rzalloc(shader, shader_info); > > + if (!si) > > + shader->shader_owns_info = true; > > > > exec_list_make_empty(&shader->functions); > > exec_list_make_empty(&shader->registers); > > diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h > > index 92647632462b..7dde8bd4ebd6 100644 > > --- a/src/compiler/nir/nir.h > > +++ b/src/compiler/nir/nir.h > > @@ -1807,6 +1807,8 @@ typedef struct nir_shader { > > /** Various bits of compile-time information about a given > > shader */ > > struct shader_info *info; > > > > + bool shader_owns_info; > > + > > /** list of global variables in the shader (nir_variable) */ > > struct exec_list globals; > > > > diff --git a/src/compiler/nir/nir_clone.c > > b/src/compiler/nir/nir_clone.c > > index 9f3bd7ff6baa..cd6063325274 100644 > > --- a/src/compiler/nir/nir_clone.c > > +++ b/src/compiler/nir/nir_clone.c > > @@ -710,7 +710,19 @@ nir_shader_clone(void *mem_ctx, const > > nir_shader *s) > > clone_reg_list(&state, &ns->registers, &s->registers); > > ns->reg_alloc = s->reg_alloc; > > > > - ns->info = s->info; > > + if (s->shader_owns_info) { > > + ns->info = ralloc(ns, struct shader_info); > > + *ns->info = *s->info; > > + ns->shader_owns_info = true; > > + } else { > > + ns->info = s->info; > > + } > > + > > + /* XXX: Note that for !shader_owns_info, we're making the info > > pointed to > > + * by s have data owned by ns, so if ns is freed before s then > > we might > > + * use-after-free the name/label. We should probably have > > name/label be > > + * owned by the info. > > + */ > > ns->info->name = ralloc_strdup(ns, ns->info->name); > > if (ns->info->label) > > ns->info->label = ralloc_strdup(ns, ns->info->label); > > diff --git a/src/compiler/nir/nir_sweep.c > > b/src/compiler/nir/nir_sweep.c > > index faf696d6decd..43664d4d5b96 100644 > > --- a/src/compiler/nir/nir_sweep.c > > +++ b/src/compiler/nir/nir_sweep.c > > @@ -153,6 +153,9 @@ nir_sweep(nir_shader *nir) > > /* First, move ownership of all the memory to a temporary > > context; assume dead. */ > > ralloc_adopt(rubbish, nir); > > > > + if (nir->shader_owns_info) > > + ralloc_steal(nir, nir->info); > > + > > ralloc_steal(nir, (char *)nir->info->name); > > if (nir->info->label) > > ralloc_steal(nir, (char *)nir->info->label); > > -- > > 2.10.1 > > > > _______________________________________________ > > mesa-dev mailing list > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > > > _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> </div></div>