This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v4 03/10] Commonise tdesc_reg
> On 23 Mar 2018, at 19:50, Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> 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.
Aha! I was trying to find a better way of doing those includes. That was the best I could reduce
it down to. Will update, and read through the links on that wiki page.
>
>> @@ -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->…
>
Yeah, after the 02 changes this patch needs a little bit of a rewrite.
I’m going to be away from a pc for two weeks now. Will update this (and part 4 too),
when I get back.
Alan.