[Mesa-dev] [PATCH 2/2] nir: Fix clone/sweep of info allocated by nir_builder_init_simple_shader().
Timothy Arceri
timothy.arceri at collabora.com
Fri Oct 28 13:13:50 UTC 2016
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 at 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 <eric at 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 at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > >
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list