On Thu, 2016-10-27 at 15:23 -0700, Jason Ekstrand wrote: > On Thu, Oct 27, 2016 at 2:47 PM, Timothy Arceri <timothy.arceri@colla > bora.com> wrote: > > On Thu, 2016-10-27 at 13:57 -0700, Jason Ekstrand wrote: > > > On Thu, Oct 27, 2016 at 1:30 PM, Eric Anholt <e...@anholt.net> > > 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. > > 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. > > > 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? > > 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.
So I've made a bit of progress and I'm not sure how much I like this. The problem is because shader_info contains more than just the values we get from gather info we can end up passing it around quite a bit [1]. You can see more here at the end of the series [2]. Also it feels like shader_info is just sort of floating around, I'm not sure it makes the data flow any clearer. Anyway I'm done for the week so have a think about where this is headed and let me know if I should continue. I'm starting to think we should just go with 3) and sort it out later if there is a problem, I don't think it would be any more fragile than where this is heading. [1] https://github.com/tarceri/Mesa/commit/f18c4353d15453f0655ae69f27c7 9c8609da0633 [2] https://github.com/tarceri/Mesa/compare/master...fix_shader_info > > (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 didn't mean add another struct to wrap them in, I meant add a nir_shader pointer in shader_info (or what ever we want to call it). Basically flip them around. > 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. > > Anholt? > > > > > > > > > > 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 > > > > mesa-dev@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > > > > > > > _______________________________________________ > > > mesa-dev mailing list > > > mesa-dev@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
