[Mesa-dev] [PATCH 7/9] i965: Avoid null access in fs_generator::generate_code()

[Pohjolainen, Topi]

On Thu, Jul 03, 2014 at 05:31:19PM +0300, Juha-Pekka Heikkila wrote:
> On 03.07.2014 16:26, Pohjolainen, Topi wrote:
> > On Thu, Jul 03, 2014 at 11:13:17AM +0300, Juha-Pekka Heikkila wrote:
> >> Avoid null access while printing debug infos. On the same go
> >> change local variable name to avoid confusion because there
> >> already is class member with same name.
> >>
> >> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> >> index 52e88d4..6e201d1 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> >> @@ -1783,9 +1783,13 @@ fs_generator::generate_code(exec_list *instructions)
> >>                dispatch_width, before_size / 16, before_size, after_size,
> >>                100.0f * (before_size - after_size) / before_size);
> >>  
> > 
> > I had to check the context a bit, just before there is:
> > 
> >       if (prog) {
> >          ...
> >       } else if (fp) {
> >          ...
> >       } else {
> >          fprintf(stderr, "Native code for blorp program (SIMD%d dispatch):\n",
> >                  dispatch_width);
> >       }
> > 
> > As I remembered you are now addressing the special case of blorp programs.
> > After your change we can't dump them anymore (using env setting
> > INTEL_DEBUG=blorp).
> 
> We should go to dump_assembly even if prog is NULL? When looking at
> dump_assembly I see prog being used only at intel_asm_printer around
> line 55 where it goes like:
> 
>       if (last_annotation_ir != annotation[i].ir) {
>          last_annotation_ir = annotation[i].ir;
>          if (last_annotation_ir) {
>             fprintf(stderr, "   ");
>             if (!prog->Instructions)
>                fprint_ir(stderr, annotation[i].ir);
>             else {
>                const struct prog_instruction *pi =
>                   (const struct prog_instruction *)annotation[i].ir;
>                fprintf(stderr, "%d: ",
>                        (int)(pi - prog->Instructions));
>                _mesa_fprint_instruction_opt(stderr,
>                                             pi,
>                                             0, PROG_PRINT_DEBUG, NULL);
>             }
>             fprintf(stderr, "\n");
>          }
> 
> Line 55 is that "if (!prog->Instructions)".
> 
> "if (last_annotation_ir != annotation[i].ir)" never matches when prog is
> also NULL?

In blorp there are no annotations and hence this path is not taken. So yes,
we should allow dumping without prog but then probably just ignore
annotations altogether.
I think the long term plan is still to get rid of blorp programs and
something simple should suffice, I think.

> 
> >> -      const struct gl_program *prog = fp ? &fp->Base : NULL;
> >> +      const struct gl_program *fp_prog = fp ? &fp->Base : NULL;
> >> +
> >> +      if (fp_prog) {
> >> +         dump_assembly(p->store, annotation.ann_count, annotation.ann, brw,
> >> +                       fp_prog);
> >> +      }
> >>  
> >> -      dump_assembly(p->store, annotation.ann_count, annotation.ann, brw, prog);
> >>        ralloc_free(annotation.ann);
> >>     }
> >>  }
> >> -- 
> >> 1.8.1.2
> >>
>