[Mesa-dev] [PATCH 01/11] aubinator: Store enum textual name in iter->value.

[Lionel Landwerlin]

On 20/03/17 09:13, Kenneth Graunke wrote:
> gen_field_iterator_next() produces a string representing the value of
> the field.  For enum values, it also produced a separate "description"
> string containing the textual name of the enum.
>
> The only caller of this function combines the two, printing enums as
> "<numeric value> (<texture enum name>)".  We may as well just store
> that in item->value directly, eliminating the description field, and
> a layer of wrapping.
> ---
>   src/intel/tools/aubinator.c |  7 +------
>   src/intel/tools/decoder.c   | 26 ++++++++++++++------------
>   src/intel/tools/decoder.h   |  1 -
>   3 files changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/src/intel/tools/aubinator.c b/src/intel/tools/aubinator.c
> index fe6127ee312..6a37da11650 100644
> --- a/src/intel/tools/aubinator.c
> +++ b/src/intel/tools/aubinator.c
> @@ -116,12 +116,7 @@ print_iterator_values(struct gen_field_iterator *iter, int *idx)
>   {
>       char *token = NULL;
>       if (strstr(iter->value, "struct") == NULL) {
> -       if (strlen(iter->description) > 0) {
> -          fprintf(outfile, "    %s: %s (%s)\n",
> -                 iter->name, iter->value, iter->description);
> -       } else {
> -          fprintf(outfile, "    %s: %s\n", iter->name, iter->value);
> -       }
> +       fprintf(outfile, "    %s: %s\n", iter->name, iter->value);
>       } else {
>           token = strtok(iter->value, " ");
>           if (token != NULL) {
> diff --git a/src/intel/tools/decoder.c b/src/intel/tools/decoder.c
> index 72913601c04..ac1c74d7d5d 100644
> --- a/src/intel/tools/decoder.c
> +++ b/src/intel/tools/decoder.c
> @@ -726,16 +726,15 @@ gen_field_iterator_init(struct gen_field_iterator *iter,
>      iter->print_colors = print_colors;
>   }
>   
> -static void
> -gen_enum_write_value(char *str, size_t max_length,
> -                      struct gen_enum *e, uint64_t value)
> +static const char *
> +gen_get_enum_name(struct gen_enum *e, uint64_t value)
>   {
>      for (int i = 0; i < e->nvalues; i++) {
>         if (e->values[i]->value == value) {
> -         strncpy(str, e->values[i]->name, max_length);
> -         return;
> +         return e->values[i]->name;
>         }
>      }
> +   return NULL;
>   }
>   
>   bool
> @@ -759,7 +758,7 @@ gen_field_iterator_next(struct gen_field_iterator *iter)
>      else
>         v.qw = iter->p[index];
>   
> -   iter->description[0] = '\0';
> +   const char *enum_name = NULL;
>   
>      switch (f->type.kind) {
>      case GEN_TYPE_UNKNOWN:
> @@ -767,16 +766,14 @@ gen_field_iterator_next(struct gen_field_iterator *iter)
>         uint64_t value = field(v.qw, f->start, f->end);
>         snprintf(iter->value, sizeof(iter->value),
>                  "%"PRId64, value);
> -      gen_enum_write_value(iter->description, sizeof(iter->description),
> -                           &f->inline_enum, value);
> +      enum_name = gen_get_enum_name(&f->inline_enum, value);
>         break;
>      }
>      case GEN_TYPE_UINT: {
>         uint64_t value = field(v.qw, f->start, f->end);
>         snprintf(iter->value, sizeof(iter->value),
>                  "%"PRIu64, value);
> -      gen_enum_write_value(iter->description, sizeof(iter->description),
> -                            &f->inline_enum, value);
> +      enum_name = gen_get_enum_name(&f->inline_enum, value);
>         break;
>      }
>      case GEN_TYPE_BOOL: {
> @@ -797,6 +794,7 @@ gen_field_iterator_next(struct gen_field_iterator *iter)
>      case GEN_TYPE_STRUCT:
>         snprintf(iter->value, sizeof(iter->value),
>                  "<struct %s %d>", f->type.gen_struct->name, (f->start / 32));
> +

Any reason for this additional space?

>         break;
>      case GEN_TYPE_UFIXED:
>         snprintf(iter->value, sizeof(iter->value),
> @@ -812,11 +810,15 @@ gen_field_iterator_next(struct gen_field_iterator *iter)
>         uint64_t value = field(v.qw, f->start, f->end);
>         snprintf(iter->value, sizeof(iter->value),
>                  "%"PRId64, value);
> -      gen_enum_write_value(iter->description, sizeof(iter->description),
> -                           f->type.gen_enum, value);
> +      enum_name = gen_get_enum_name(f->type.gen_enum, value);
>         break;
>      }
>      }
>   
> +   if (enum_name) {
> +      snprintf(iter->value, sizeof(iter->value),
> +               "%s (%s)", iter->value, enum_name);
> +   }
> +

Looking at the manpage of snprintf :

"
Some programs imprudently rely on code such as the following

     sprintf(buf, "%s some further text", buf);

to append text to buf.  However, the standards explicitly note that the
results are undefined if source and destination  buffers  overlap when
calling  sprintf(), snprintf(), vsprintf(), and vsnprintf(). Depending
on the version of gcc(1) used, and the compiler options employed, calls
such as the above will not produce the expected results.
"

I think we might want to avoid that and have an indirection buffer.

>      return true;
>   }
> diff --git a/src/intel/tools/decoder.h b/src/intel/tools/decoder.h
> index 9f0aa4f35f1..b17be1d5fbf 100644
> --- a/src/intel/tools/decoder.h
> +++ b/src/intel/tools/decoder.h
> @@ -54,7 +54,6 @@ struct gen_field_iterator {
>      struct gen_group *group;
>      const char *name;
>      char value[128];
> -   char description[128];
>      const uint32_t *p;
>      int i;
>      bool print_colors;