[Mesa-dev] [PATCH 6/9] i965: Add annotation_insert_error() and support for printing errors.
Kenneth Graunke
kenneth at whitecape.org
Tue Nov 3 21:47:20 PST 2015
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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151103/b630536b/attachment.sig>
More information about the mesa-dev
mailing list