[PATCH 6/6] Make aop_map 'static'

[John Baldwin]

On 6/19/23 2:06 PM, Tom Tromey wrote:
> This changes aop_map to be 'static'.
> 
> It also changes a runtime assertion into a static assert.  I'm not
> sure if this assert provides much value -- by construction, it can't
> really fail -- but it's clearly more useful as a static assert than a
> runtime one.
> ---
>   gdb/ax-general.c | 40 +++++++++++++++++++++++++++++++++-------
>   gdb/ax.h         | 30 ------------------------------
>   2 files changed, 33 insertions(+), 37 deletions(-)
> 
> diff --git a/gdb/ax-general.c b/gdb/ax-general.c
> index 7a37aff3d70..b1ee2753bd8 100644
> --- a/gdb/ax-general.c
> +++ b/gdb/ax-general.c
> @@ -294,7 +294,36 @@ ax_string (struct agent_expr *x, const char *str, int slen)
>   /* Functions for disassembling agent expressions, and otherwise
>      debugging the expression compiler.  */
>   
> -struct aop_map aop_map[] =
> +/* An entry in the opcode map.  */
> +struct aop_map
> +  {
> +
> +    /* The name of the opcode.  Null means that this entry is not a
> +       valid opcode --- a hole in the opcode space.  */
> +    const char *name;
> +
> +    /* All opcodes take no operands from the bytecode stream, or take
> +       unsigned integers of various sizes.  If this is a positive number
> +       n, then the opcode is followed by an n-byte operand, which should
> +       be printed as an unsigned integer.  If this is zero, then the
> +       opcode takes no operands from the bytecode stream.
> +
> +       If we get more complicated opcodes in the future, don't add other
> +       magic values of this; that's a crock.  Add an `enum encoding'
> +       field to this, or something like that.  */
> +    int op_size;
> +
> +    /* The size of the data operated upon, in bits, for bytecodes that
> +       care about that (ref and const).  Zero for all others.  */
> +    int data_size;
> +
> +    /* Number of stack elements consumed, and number produced.  */
> +    int consumed, produced;
> +  };
> +
> +/* Map of the bytecodes, indexed by bytecode number.  */
> +
> +static struct aop_map aop_map[] =
>   {
>     {0, 0, 0, 0, 0}
>   #define DEFOP(NAME, SIZE, DATA_SIZE, CONSUMED, PRODUCED, VALUE) \
> @@ -303,6 +332,9 @@ struct aop_map aop_map[] =
>   #undef DEFOP
>   };
>   
> +/* Check the size of the name array against the number of entries in
> +   the enum, to catch additions that people didn't sync.  */
> +gdb_static_assert ((sizeof (aop_map) / sizeof (aop_map[0])) == aop_last);

Maybe use ARRAY_SIZE here while you are at it?

Looking around in ax-general.c more, it seems we already do runtime
validation of potential indices before indexing the array, so I'm not sure
the assertion adds much value and I'd be tempted to remove aop_last entirely.

BTW, the various other places in ax-general.c that do the assertion are
all using the expanded form of ARRAY_SIZE and would be a bit more readable
perhaps if they used ARRAY_SIZE instead.

-- 
John Baldwin