Re: [Qemu-devel] [PATCH v2] target-ppc: improve "info registers" by printing SPRs

[Stuart Brady <sdb@zubnet.me.uk>]

On Mon, Mar 24, 2014 at 05:24:35PM +1100, Alexey Kardashevskiy wrote:
> On 03/23/2014 01:43 AM, Stuart Brady wrote:
> > This would leave the output without a trailing newline if the last spr
> > doesn't have a name registered.  Is it necessary to handle unnamed sprs
> > at all (maybe add an assert to the registration function)? ... or would
> > we just want to warn about them here?
> 
> In the current QEMU I do not see any place where SPR would be registered
> without a name so I would not bother.

Okay, although an assert probably wouldn't hurt.  OTOH, if we never want
an SPR without a name, removing the handling for !spr->name entirely
would make sense.  You could then get rid of the separate 'j' variable.

> > FWIW, my approach is often to write an outer loop that process one item
> > of output at a time, with an inner loop to obtain the next item of data,
> > and with prefixing of separators, as you then have a far simpler special
> > case for 'j == 0' instead of 'i == ARRAY_SIZE(env->spr_cb) - 1'.  You can
> > then unconditionally finish on a '\n'.
> 
> Oh. This is embarrassing, sorry :( This should do it, right? :)

Not that embarrassing, to be honest.  The code I once fixed that built up
a string of comma-separated values, and then tried to remove the trailing
comma even if the string was empty, now *that* was embarrassing.  This
was remarkably copied-and-pasted all over the shop and then fixed in some
files but not others.  Luckily, corruption of the preceding variable was
harmless in all occurrences since it was held in a register.  Erk. :-(
In that particular case it made more sense to increment to point past the
leading comma if present, and I've tended toward that style ever since.

> +    for (i = 0, j = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
> +        ppc_spr_t *spr = &env->spr_cb[i];
> +
> +        if (!spr->name) {
> +            continue;
> +        }
> +        if (j % 4) {
> +            cpu_fprintf(f, " ");
> +        } else if (j) {
> +            cpu_fprintf(f, "\n");
> +        }
> +        cpu_fprintf(f, "%-6s " TARGET_FMT_lx, spr->name, env->spr[i]);
> +        j++;
>      }
> +    cpu_fprintf(f, "\n");

Looks good.

> btw while grepping through the code, I found dump_ppc_sprs() which prints
> this (first chunk is what my patch adds and the second chunk is from
> dump_ppc_sprs()):
> 
[...]

> Special purpose registers:
[...]

> Which is nicer/more useful?

> The characters at the end tell what handler (read/write, oea/uea) is
> defined for SPR.

They're both perfectly fine.  For the list of registers, you want detail
about access type, etc. and perhaps not the current values, but for the
simple register dump would want values and register names but without any
extraneous information.
-- 
Cheers,
Stuart