On Thu, Oct 27, 2016 at 2:47 PM, Timothy Arceri <
timothy.arc...@collabora.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.
(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.
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
