[PATCH v4 03/10] Commonise tdesc_reg

[Simon Marchi]

On 2018-03-22 04:44 AM, alan.hayward@arm.com wrote:
> From: Alan Hayward <alan.hayward@arm.com>
> 
> This patch commonises tdesc_reg and makes use of it in gdbserver tdesc.
> 
> gdbserver tdesc_create_reg is changed to create a tdesc_reg instead of
> a reg_defs entry. The vector of tdesc_reg is held inside tdesc_feature.
> 
> However, other modules in gdbserver directly access the reg_defs structure.
> To work around this, init_target_desc fills in reg_defs by parsing the
> tdesc_reg vector.
> The long term goal is to remove reg_defs, replacing with accessor funcs.
> 
> I wanted to make tdesc_create_reg common, but I cannot do that until
> the next patch.
> 
> I also had to commonise tdesc_element_visitor and tdesc_element.
> 
> This patch only differs from the V3 version in init_target_desc(),
> where I now use reg_def references as introduced in the previous patch.

Hi Alan,

I have just a few superficial comments because I don't have the big picture
yet, I'll continue with the next patches and might come back.

> 2018-03-21  Alan Hayward  <alan.hayward@arm.com>
> 
> gdb/
> 	* Makefile.in: Add arch/tdesc.c
> 	* common/tdesc.c: New file.
> 	* common/tdesc.h (tdesc_element_visitor): Move to here.
> 	(tdesc_element): Likewise.
> 	(tdesc_reg): Likewise.
> 	(tdesc_reg_up): Likewise.
> 	* target-descriptions.c: (tdesc_element_visitor): Move from here.

Spurious colon after the filename.

> 	(tdesc_element): Likewise.
> 	(tdesc_reg): Likewise.
> 	(tdesc_reg_up): Likewise.
> 
> gdbserver/
> 	* Makefile.in: Add common/tdesc.c
> 	* tdesc.c (init_target_desc): init all reg_defs from register vector.
> 	(tdesc_create_reg): Create tdesc_reg.
> 	* tdesc.h (tdesc_feature): Add register vector.
> ---
>  gdb/Makefile.in           |   2 +
>  gdb/common/tdesc.c        |  40 ++++++++++++++++
>  gdb/common/tdesc.h        | 111 ++++++++++++++++++++++++++++++++++++++++++++
>  gdb/gdbserver/Makefile.in |   3 ++
>  gdb/gdbserver/tdesc.c     |  53 ++++++++++++---------
>  gdb/gdbserver/tdesc.h     |   5 +-
>  gdb/target-descriptions.c | 116 ----------------------------------------------
>  7 files changed, 191 insertions(+), 139 deletions(-)
>  create mode 100644 gdb/common/tdesc.c
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 44db1937b6..f1d2f0193a 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -966,6 +966,7 @@ COMMON_SFILES = \
>  	common/run-time-clock.c \
>  	common/signals.c \
>  	common/signals-state-save-restore.c \
> +	common/tdesc.c \
>  	common/vec.c \
>  	common/xml-utils.c \
>  	complaints.c \
> @@ -1443,6 +1444,7 @@ HFILES_NO_SRCDIR = \
>  	common/run-time-clock.h \
>  	common/signals-state-save-restore.h \
>  	common/symbol.h \
> +	common/tdesc.h \
>  	common/vec.h \
>  	common/version.h \
>  	common/x86-xstate.h \
> diff --git a/gdb/common/tdesc.c b/gdb/common/tdesc.c
> new file mode 100644
> index 0000000000..a5f2de4b5b
> --- /dev/null
> +++ b/gdb/common/tdesc.c
> @@ -0,0 +1,40 @@
> +/* Target description support for GDB.
> +
> +   Copyright (C) 2017 Free Software Foundation, Inc.

2018

> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifdef GDBSERVER
> +#include "server.h"
> +#else
> +#include "defs.h"
> +#endif

We are aiming to remove all uses of #ifdef GDBSERVER in the code base, so we shouldn't
add any.  You might want to read this if you haven't seen it already:

  https://sourceware.org/gdb/wiki/Common

Replace this with #include "common-defs.h".  You can also remove the same snippet in
common/tdesc.h.  Header files assume that defs.h/server.h/common-defs.h has been
included already, so they don't need to do it.

> @@ -236,26 +261,10 @@ tdesc_create_reg (struct tdesc_feature *feature, const char *name,
>  		  int regnum, int save_restore, const char *group,
>  		  int bitsize, const char *type)
>  {
> -  struct target_desc *tdesc = (struct target_desc *) feature;
> -  int current_size = tdesc->reg_defs.size ();
> -
> -  tdesc->reg_defs.resize (regnum != 0 ? regnum + 1 : current_size + 1);
> -
> -  while (current_size < regnum)
> -    {
> -      struct reg *reg = &tdesc->reg_defs[current_size];
> -      reg->name = "";
> -      reg->size = 0;
> -      current_size++;
> -    }
> -
> -  gdb_assert (regnum == 0
> -	      || regnum + 1 == tdesc->reg_defs.size ());
> -
> -  struct reg *reg = &tdesc->reg_defs.back ();
> +  tdesc_reg *reg = new tdesc_reg (feature, name, regnum, save_restore,
> +				  group, bitsize, type);
>  
> -  reg->name = name;
> -  reg->size = bitsize;
> +  tdesc->registers.emplace_back (reg);

This doesn't build, I guess it should be feature->...

Simon