On Wednesday, October 21, 2015 03:58:14 PM Matt Turner wrote: > Will allow annotations to contain error messages (indicating an > instruction violates a rule for instance) that are printed after the > disassembly of the block. > --- > src/mesa/drivers/dri/i965/intel_asm_annotation.c | 60 > ++++++++++++++++++++++++ > src/mesa/drivers/dri/i965/intel_asm_annotation.h | 7 +++ > 2 files changed, 67 insertions(+) > > diff --git a/src/mesa/drivers/dri/i965/intel_asm_annotation.c > b/src/mesa/drivers/dri/i965/intel_asm_annotation.c > index 58830db..eaee386 100644 > --- a/src/mesa/drivers/dri/i965/intel_asm_annotation.c > +++ b/src/mesa/drivers/dri/i965/intel_asm_annotation.c > @@ -69,6 +69,10 @@ dump_assembly(void *assembly, int num_annotations, struct > annotation *annotation > > brw_disassemble(devinfo, assembly, start_offset, end_offset, stderr); > > + if (annotation[i].error) { > + fputs(annotation[i].error, stderr); > + } > + > if (annotation[i].block_end) { > fprintf(stderr, " END B%d", annotation[i].block_end->num); > foreach_list_typed(struct bblock_link, successor_link, link, > @@ -152,3 +156,59 @@ annotation_finalize(struct annotation_info *annotation, > } > annotation->ann[annotation->ann_count].offset = next_inst_offset; > } > + > +void > +annotation_insert_error(struct annotation_info *annotation, unsigned offset, > + const char *error) > +{ > + struct annotation *ann = NULL; > + > + if (!annotation->ann_count) > + return;
If I'm reading this correctly, it means that we won't report any errors if there are no annotations. Shouldn't we instead insert a new annotation in this case? Something like: if (annotation->ann_count == 0) { struct annotation *ann = &annotation->ann[0]; annotation->ann_count++; ann->offset = 0; ann->block_start = NULL; ann->block_end = NULL; ann->ir = NULL; ann->annotation = NULL; ann->error = ralloc_strdup(annotation->mem_ctx, error); return; } > + > + /* We may have to split an annotation, so ensure we have enough space > + * allocated for that case up front. > + */ > + if (annotation->ann_size <= annotation->ann_count) { > + int old_size = annotation->ann_size; > + annotation->ann_size = MAX2(1024, annotation->ann_size * 2); > + annotation->ann = reralloc(annotation->mem_ctx, annotation->ann, > + struct annotation, annotation->ann_size); > + if (!annotation->ann) > + return; > + > + memset(annotation->ann + old_size, 0, > + (annotation->ann_size - old_size) * sizeof(struct annotation)); > + } I agree with Topi, let's use a helper function, such as: /** * Make sure the annotation->ann[] array has room for one more element. */ static void grow_annotation_array(struct annotation_info *annotation) { ... } > + > + for (int i = 0; i <= annotation->ann_count; i++) { Tricky...you're relying on the fact that annotation_finalize() has made ann[ann_count] exist and have an offset equal to the end of the program. So, there's a sentinel annotation of sorts, which makes <= okay (as opposed to the obvious choice of <), and also guarantees that you'll actually find an element whose offset is larger. But this isn't safe when i = 0, as cur will be out of bounds... > + if (annotation->ann[i].offset <= offset) > + continue; > + > + struct annotation *cur = &annotation->ann[i - 1]; > + struct annotation *next = &annotation->ann[i]; Okay, having "cur" be the (i - 1)th element and "next" be the ith element is just mean :( That's not what those mean. > + ann = cur; > + > + if (offset + sizeof(brw_inst) != next->offset) { > + memmove(next, cur, > + (annotation->ann_count - i + 2) * sizeof(struct > annotation)); > + cur->error = NULL; > + cur->error_length = 0; > + cur->block_end = NULL; > + next->offset = offset + sizeof(brw_inst); > + next->block_start = NULL; > + annotation->ann_count++; > + } > + break; > + } I think this would be easier to follow: /* We want to insert the error comment /after/ the instruction. */ unsigned insertion_point = offset + sizeof(brw_inst); /* Note that annotation_finalize() has placed a sentinel annotation * at annotation->ann[annotation->ann_count] with an offset that is * the end of the program. So we're guaranteed to find an entry. */ for (int i = 0; i <= annotation->ann_count; i++) { struct annotation *cur = &annotation->ann[i]; /* If the current annotation begins exactly where we want to * insert the error, we can simply use it. */ if (cur->offset == insertion_point) { ann = cur; break; } /* If the current annotation starts beyond our insertion point, * stop. Insert a new annotation before cur, shifting the rest * of the array over to make room. */ if (cur->offset > insertion_point) { cur->block_start = NULL; // XXX: why? memmove(cur + 1, cur, (annotation->ann_count - i + 2) * sizeof(struct annotation)); ann = cur; ann->error = NULL; ann->error_length = 0; ann->block_end = NULL; ann->offset = insertion_point; annotation->ann_count++; break; } } Actually, if you make annotation_finalize() add the sentinel even when ann_count == 0, you should be able to safely use this code without needing to special case ann_count == 0 up above. Which would be nice. > + > + assume(ann != NULL); > + > + ralloc_asprintf_rewrite_tail(&ann->error, &ann->error_length, error); Using asprintf with an ordinary (non-format) string is wrong...it'll interpret % characters...and you haven't given it any arguments. You could do (..., "%s", error)...but this just seems like the wrong fit. I would just do: if (ann->error) ralloc_strcat(&ann->error, error); else ann->error = ralloc_strdup(annotation->mem_ctx, error); You can drop the FIXME code below, as you'll be using the right memory context. You can also drop the error_length field, as it's not needed. Sure, you gain extra strlen() calls when appending, but it's not worth optimizing...this is debug-only code...error strings aren't that long...and hopefully you don't have a cacophony of errors in the first place... > + > + /* FIXME: ralloc_vasprintf_rewrite_tail() allocates memory out of the > + * null context. We have to reparent the it if we want it to be freed > + * with the rest of the annotation context. > + */ > + ralloc_steal(annotation->mem_ctx, ann->error); > +} > diff --git a/src/mesa/drivers/dri/i965/intel_asm_annotation.h > b/src/mesa/drivers/dri/i965/intel_asm_annotation.h > index 6c72326..662a4b4 100644 > --- a/src/mesa/drivers/dri/i965/intel_asm_annotation.h > +++ b/src/mesa/drivers/dri/i965/intel_asm_annotation.h > @@ -37,6 +37,9 @@ struct cfg_t; > struct annotation { > int offset; > > + size_t error_length; > + char *error; > + > /* Pointers to the basic block in the CFG if the instruction group starts > * or ends a basic block. > */ > @@ -69,6 +72,10 @@ annotate(const struct brw_device_info *devinfo, > void > annotation_finalize(struct annotation_info *annotation, unsigned offset); > > +void > +annotation_insert_error(struct annotation_info *annotation, unsigned offset, > + const char *error); > + > #ifdef __cplusplus > } /* extern "C" */ > #endif >
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev