Re: [PATCH 6/7] Arm: Use read_description funcs in gdbserver

[Alan Hayward <Alan.Hayward@arm.com>]

> On 10 Jul 2019, at 05:04, Simon Marchi <simark@simark.ca> wrote:
> 
>> diff --git a/gdb/gdbserver/linux-aarch32-tdesc.c b/gdb/gdbserver/linux-aarch32-tdesc.c
>> new file mode 100644
>> index 0000000000..6f0e8c9aa9
>> --- /dev/null
>> +++ b/gdb/gdbserver/linux-aarch32-tdesc.c
>> @@ -0,0 +1,46 @@
>> +/* Copyright (C) 2019 Free Software Foundation, Inc.
>> +
>> +   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/>.  */
>> +
>> +#include "server.h"
>> +#include "tdesc.h"
>> +#include "arch/aarch32.h"
>> +#include <inttypes.h>
>> +
>> +struct target_desc *tdesc_aarch32;
> 
> static

Done.

> 
>> diff --git a/gdb/gdbserver/linux-arm-tdesc.c b/gdb/gdbserver/linux-arm-tdesc.c
>> new file mode 100644
>> index 0000000000..fa54e48592
>> --- /dev/null
>> +++ b/gdb/gdbserver/linux-arm-tdesc.c
>> @@ -0,0 +1,62 @@
>> +/* Copyright (C) 2019 Free Software Foundation, Inc.
>> +
>> +   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/>.  */
>> +
>> +#include "server.h"
>> +#include "tdesc.h"
>> +#include "arch/arm.h"
>> +#include <inttypes.h>
>> +
>> +/* All possible Arm target descriptors.  */
>> +struct target_desc *tdesc_arm_list[ARM_FP_TYPE_INVALID];
> 
> static

Done.

> 
>> +
>> +/* See linux-arm-tdesc.h.  */
>> +
>> +const target_desc *
>> +arm_linux_read_description (arm_fp_type fp_type)
>> +{
>> +  struct target_desc *tdesc = tdesc_arm_list[fp_type];
>> +
>> +  if (tdesc == nullptr)
>> +    {
>> +      tdesc = arm_create_target_description (fp_type);
>> +
>> +      static const char *expedite_regs[] = { "r11", "sp", "pc", 0 };
>> +      init_target_desc (tdesc, expedite_regs);
>> +
>> +      tdesc_arm_list[fp_type] = tdesc;
>> +    }
>> +
>> +  return tdesc;
>> +}
>> +
>> +/* See linux-arm-tdesc.h.  */
>> +
>> +arm_fp_type arm_linux_get_tdesc_fp_type (const target_desc *tdesc)
> 
> Return type on its own line.

Done.

> 
>> +{
>> +  if (tdesc == nullptr)
>> +    return ARM_FP_TYPE_INVALID;
> 
> Can this (tdesc == nullptr) actually happen?  If you expect it's not possible,
> don't hesitate to use a gdb_assert instead.  It helps catch bugs and acts as
> some kind of self-documentation of the allowed values.

It would mean that regcache->tdesc was null, which as far as I can tell is not
possible. There are no fails when testing either.
I’ll switch to gdb_assert.

> 
>> +
>> +  /* Many of the tdesc_arm_list entries may not have been initialised yet.  This
>> +     is ok, because tdesc must be one of the initialised ones.  */
>> +  for (int i = ARM_FP_TYPE_VFPV2; i < ARM_FP_TYPE_INVALID; i++)
> 
> Is it intended here that you skip ARM_FP_TYPE_NONE?  Why?

My mistake. An earlier version didn’t have ARM_FP_TYPE_NONE, and I missed this.
Fixed to start with ARM_FP_TYPE_NONE.


> 
> Simon
>