[PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Alan Hayward]

Max size set to 32bits, which I determined using regformats/reg-bfin.dat

Tested on a --enable-targets=all build using make check with board files
unix and native-gdbserver.

I do not have an BFIN machine to test on.

Ok to commit?

Alan.


2017-04-04  Alan Hayward  <alan.hayward@arm.com>

	* bfin-tdep.c (bfin_pseudo_register_read): Use BFIN_MAX_REGISTER_SIZE.
	(bfin_pseudo_register_write): Likewise.
	* bfin-tdep.h (BFIN_MAX_REGISTER_SIZE): Add.


diff --git a/gdb/bfin-tdep.h b/gdb/bfin-tdep.h
index 164466c2d6d1bd2dc0bc47ca3729d0e42e2d7ccb..e89e5c3138eb8245b10ccda98f0672e8e3fd728b 100644
--- a/gdb/bfin-tdep.h
+++ b/gdb/bfin-tdep.h
@@ -84,6 +84,9 @@ enum gdb_regnum {
 #define BFIN_NUM_REGS (BFIN_PC_REGNUM + 1)
 #define BFIN_NUM_PSEUDO_REGS (1)

+/* Big enough to hold the size of the largest register in bytes.  */
+#define BFIN_MAX_REGISTER_SIZE	4
+
 /* The ABIs for Blackfin.  */
 enum bfin_abi
 {
diff --git a/gdb/bfin-tdep.c b/gdb/bfin-tdep.c
index 3df1ba387a323dc6827b1189432f8877d1833184..9b45633cab15b8e0adb0d51a2fa650dc2bc6339b 100644
--- a/gdb/bfin-tdep.c
+++ b/gdb/bfin-tdep.c
@@ -689,7 +689,7 @@ static enum register_status
 bfin_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache,
 			   int regnum, gdb_byte *buffer)
 {
-  gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
+  gdb_byte *buf = (gdb_byte *) alloca (BFIN_MAX_REGISTER_SIZE);
   enum register_status status;

   if (regnum != BFIN_CC_REGNUM)
@@ -710,7 +710,7 @@ static void
 bfin_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
 			    int regnum, const gdb_byte *buffer)
 {
-  gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
+  gdb_byte *buf = (gdb_byte *) alloca (BFIN_MAX_REGISTER_SIZE);

   if (regnum != BFIN_CC_REGNUM)
     internal_error (__FILE__, __LINE__,

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Andreas Schwab]

On Apr 05 2017, Alan Hayward <Alan.Hayward@arm.com> wrote:

>> On 5 Apr 2017, at 11:28, Yao Qi <qiyaoltc@gmail.com> wrote:
>> 
>> Alan Hayward <Alan.Hayward@arm.com> writes:
>> 
>>> diff --git a/gdb/bfin-tdep.c b/gdb/bfin-tdep.c
>>> index 3df1ba387a323dc6827b1189432f8877d1833184..9b45633cab15b8e0adb0d51a2fa650dc2bc6339b 100644
>>> --- a/gdb/bfin-tdep.c
>>> +++ b/gdb/bfin-tdep.c
>>> @@ -689,7 +689,7 @@ static enum register_status
>>> bfin_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache,
>>> 			   int regnum, gdb_byte *buffer)
>>> {
>>> -  gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
>>> +  gdb_byte *buf = (gdb_byte *) alloca (BFIN_MAX_REGISTER_SIZE);
>> 
>> Why don't you do "gdb_byte buf[4];"?  It is only for CC register which
>> is 32-bit.
>> 
>
> Is it not clearer code to add and use a macro rather than a magic number ?
>
> It’s also not obvious anywhere that the astat register is 32bits. I had to go
> digging inside regformats/reg-bfin.dat before I found it out.
>
> Given that BFIN_MAX_REGISTER_SIZE is also 4, it compiles to the same size anyway.

Since BFIN_MAX_REGISTER_SIZE is a constant you don't need alloca either
way.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Alan Hayward]

> On 5 Apr 2017, at 14:50, Andreas Schwab <schwab@suse.de> wrote:
> 
> On Apr 05 2017, Alan Hayward <Alan.Hayward@arm.com> wrote:
> 
>>> On 5 Apr 2017, at 11:28, Yao Qi <qiyaoltc@gmail.com> wrote:
>>> 
>>> Alan Hayward <Alan.Hayward@arm.com> writes:
>>> 
>>>> diff --git a/gdb/bfin-tdep.c b/gdb/bfin-tdep.c
>>>> index 3df1ba387a323dc6827b1189432f8877d1833184..9b45633cab15b8e0adb0d51a2fa650dc2bc6339b 100644
>>>> --- a/gdb/bfin-tdep.c
>>>> +++ b/gdb/bfin-tdep.c
>>>> @@ -689,7 +689,7 @@ static enum register_status
>>>> bfin_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache,
>>>> 			   int regnum, gdb_byte *buffer)
>>>> {
>>>> -  gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
>>>> +  gdb_byte *buf = (gdb_byte *) alloca (BFIN_MAX_REGISTER_SIZE);
>>> 
>>> Why don't you do "gdb_byte buf[4];"?  It is only for CC register which
>>> is 32-bit.
>>> 
>> 
>> Is it not clearer code to add and use a macro rather than a magic number ?
>> 
>> It’s also not obvious anywhere that the astat register is 32bits. I had to go
>> digging inside regformats/reg-bfin.dat before I found it out.
>> 
>> Given that BFIN_MAX_REGISTER_SIZE is also 4, it compiles to the same size anyway.
> 
> Since BFIN_MAX_REGISTER_SIZE is a constant you don't need alloca either
> way.
> 

Sorry, yes, I meant to say:

Why not use:
gdb_byte buf[BFIN_MAX_REGISTER_SIZE];

Rather than
gdb_byte buf[4];

Given that a macro is clearer than a magic number.

Alan.

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Alan Hayward]

> On 5 Apr 2017, at 15:10, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
>> Is it not clearer code to add and use a macro rather than a magic number ?
>> 
> 
> Macro is better, but "buf[4]" is not that magic, because it is only used
> within bfin_pseudo_register_read, or we can define ASTAT_REGISTER_SIZE.
> 
>> It’s also not obvious anywhere that the astat register is 32bits. I had to go
>> digging inside regformats/reg-bfin.dat before I found it out.
> 
> It is easier to figure out the size of a specific register than the max
> size of a set of registers.
> 
>> 
>> Given that BFIN_MAX_REGISTER_SIZE is also 4, it compiles to the same
>> size anyway.
> 
> "gdb_byte buf[BFIN_MAX_REGISTER_SIZE]" is fine to me.  Could you define
> BFIN_MAX_REGISTER_SIZE in bfin-tdep.c instead of .h?  It is not used
> elsewhere.
> 

Ok, pushed the following:

2017-04-07  Alan Hayward  <alan.hayward@arm.com>

	* bfin-tdep.c (BFIN_MAX_REGISTER_SIZE): Add.
	(bfin_pseudo_register_read): Use BFIN_MAX_REGISTER_SIZE.
	(bfin_pseudo_register_write): Likewise


diff --git a/gdb/bfin-tdep.c b/gdb/bfin-tdep.c
index 3df1ba387a323dc6827b1189432f8877d1833184..c590a9132c00f539b5c6e57b8f156845ff50acad 100644
--- a/gdb/bfin-tdep.c
+++ b/gdb/bfin-tdep.c
@@ -241,6 +241,8 @@ static const int map_gcc_gdb[] =
   BFIN_LB1_REGNUM
 };

+/* Big enough to hold the size of the largest register in bytes.  */
+#define BFIN_MAX_REGISTER_SIZE	4

 struct bfin_frame_cache
 {
@@ -689,7 +691,7 @@ static enum register_status
 bfin_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache,
 			   int regnum, gdb_byte *buffer)
 {
-  gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
+  gdb_byte buf[BFIN_MAX_REGISTER_SIZE];
   enum register_status status;

   if (regnum != BFIN_CC_REGNUM)
@@ -710,7 +712,7 @@ static void
 bfin_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
 			    int regnum, const gdb_byte *buffer)
 {
-  gdb_byte *buf = (gdb_byte *) alloca (MAX_REGISTER_SIZE);
+  gdb_byte buf[BFIN_MAX_REGISTER_SIZE];

   if (regnum != BFIN_CC_REGNUM)
     internal_error (__FILE__, __LINE__,

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Alan Hayward]

> On 5 Apr 2017, at 16:03, Pedro Alves <palves@redhat.com> wrote:
> 
> On 04/04/2017 11:14 AM, Alan Hayward wrote:
>> Max size set to 32bits, which I determined using regformats/reg-bfin.dat
> 
> Makes me wonder sth (and in general, not for this patch in particular):
> 
> Is it possible that any of these code paths that hardcode an arch specific
> max register size end up seeing a larger register size because the reported
> xml target description includes such a larger register?
> 
> E.g., say arch A normally only has 32-bit registers, for as much GDB knows.
> And then some stub for some variant of A includes a register
> in the description like:
> 
>  <reg name="foo" bitsize="64" type="uint64"/>
> 
> It kinds of sounds like the max register size is capped by what target
> descriptions can describe for that architecture, not exactly by the size
> of the registers that GDB considers "core" registers.  That may
> already have been taken into account and it may well be that the paths
> that use the FOO_MAX_REGISTER_SIZE macros only ever work with registers
> that GDB does know about (haven't checked carefully), rendering the concern
> moot, but I wanted to put the thought out there anyway.
> 
> Thanks,
> Pedro Alves
> 

There is some existing code in regcache.c that checks that no register in the
target descriptor is greater than MAX_REGISTER_SIZE.
Obviously, this code will vanish when MAX_REGISTER_SIZE disappears.

If people think that this is an important check to have, then maybe there needs
to be an additional patch set. For each target with a FOO_MAX_REGISTER_SIZE,
in the init code for that target add:
  gdb_assert (FOO_MAX_REGISTER_SIZE >= max_register_size (gdbarch));
?

(To keep this simple I’d do it after adding these patches which add
 FOO_MAX_REGISTER_SIZE).


Alan.

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Yao Qi]

Alan Hayward <Alan.Hayward@arm.com> writes:

> There is some existing code in regcache.c that checks that no register in the
> target descriptor is greater than MAX_REGISTER_SIZE.
> Obviously, this code will vanish when MAX_REGISTER_SIZE disappears.
>
> If people think that this is an important check to have, then maybe there needs
> to be an additional patch set. For each target with a FOO_MAX_REGISTER_SIZE,
> in the init code for that target add:
>   gdb_assert (FOO_MAX_REGISTER_SIZE >= max_register_size (gdbarch));
> ?

It is not just about checking.  What if the assert fail?
FOO_MAX_REGISTER_SIZE is a strong claim for arch FOO.  That is why I
suggested "or we can define ASTAT_REGISTER_SIZE".

>
> (To keep this simple I’d do it after adding these patches which add
>  FOO_MAX_REGISTER_SIZE).

-- 
Yao (齐尧)

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Alan Hayward]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="us-ascii", Size: 7956 bytes --]


> On 7 Apr 2017, at 17:04, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
>> There is some existing code in regcache.c that checks that no register in the
>> target descriptor is greater than MAX_REGISTER_SIZE.
>> Obviously, this code will vanish when MAX_REGISTER_SIZE disappears.
>> 
>> If people think that this is an important check to have, then maybe there needs
>> to be an additional patch set. For each target with a FOO_MAX_REGISTER_SIZE,
>> in the init code for that target add:
>>  gdb_assert (FOO_MAX_REGISTER_SIZE >= max_register_size (gdbarch));
>> ?
> 
> It is not just about checking.  What if the assert fail?
> FOO_MAX_REGISTER_SIZE is a strong claim for arch FOO.  That is why I
> suggested "or we can define ASTAT_REGISTER_SIZE".
> 

The checks would be added as a replacement for what is already in
init_regcache_descr() code in regcache.c:

    for (i = 0; i < descr->nr_raw_registers; i++)
      {
	<snip>
	gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]);
        <snip>
      }

If the assert fails then the its not safe for gdb to run - the define
should always be big enough to hold any register on the current architecture


Alan.
From gdb-patches-return-138092-listarch-gdb-patches=sources.redhat.com@sourceware.org Fri Apr 07 17:33:02 2017
Return-Path: <gdb-patches-return-138092-listarch-gdb-patches=sources.redhat.com@sourceware.org>
Delivered-To: listarch-gdb-patches@sources.redhat.com
Received: (qmail 9935 invoked by alias); 7 Apr 2017 17:33:02 -0000
Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id: <gdb-patches.sourceware.org>
List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org>
List-Archive: <http://sourceware.org/ml/gdb-patches/>
List-Post: <mailto:gdb-patches@sourceware.org>
List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs>
Sender: gdb-patches-owner@sourceware.org
Delivered-To: mailing list gdb-patches@sourceware.org
Received: (qmail 9859 invoked by uid 89); 7 Apr 2017 17:33:01 -0000
Authentication-Results: sourceware.org; auth=none
X-Virus-Found: No
X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammyºking, normalize, upfront, discussing
X-HELO: mail-wr0-f176.google.com
Received: from mail-wr0-f176.google.com (HELO mail-wr0-f176.google.com) (209.85.128.176) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 07 Apr 2017 17:32:59 +0000
Received: by mail-wr0-f176.google.com with SMTP id t20so113815857wra.1        for <gdb-patches@sourceware.org>; Fri, 07 Apr 2017 10:33:00 -0700 (PDT)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;        d\x1e100.net; s 161025;        h=x-gm-message-state:subject:to:references:cc:from:message-id:date         :user-agent:mime-version:in-reply-to:content-transfer-encoding;        bh=wyA4OHz+qgAwKGk/7sCFbHEusSBJryjxvW/OO69yZAg=;        bþh6k3IPjbGuOsXvgcEW2vN10PCQyoVP5d4FOFk2+ZOhrq6GzRpsPvusD0JgBPTPNy         XpochaynJ+hltN3Ss//6heN4PkpQS6/PJUpSnyFTMR1j7atDpuLHLkD94sqzBzNxQljK         Z97cz+AoLJJqAhgdn19Ejq0WOK0jReBjC3jRdZ7NBzeHvCRyQY0C06+V7lSid11W+c+r         xxeePS1NwEjZRGa2kO+HbuYXvl13BJM9fR52zEMnbY4Zsii7OEuOuOJhKGWoNiDBi7go         qEkkw5KIt+tZgNMml2d5GgjJDUEmpkbGB/omQ4GzTFxEAC5X3lLF5jhi9CWBCWOKOBcd         6Xiw=X-Gm-Message-State: AFeK/H2Q8dX9PK1pgtkUarxVSQvYxx3XW7C+a4wODI19m6jak70i4VNNWDlFuu044ShCpXAu
X-Received: by 10.223.146.164 with SMTP id 33mr36092921wrn.17.1491586379015;        Fri, 07 Apr 2017 10:32:59 -0700 (PDT)
Received: from [192.168.0.101] ([37.189.166.198])        by smtp.gmail.com with ESMTPSA id 60sm6747573wrg.60.2017.04.07.10.32.57        (version=TLS1_2 cipherìDHE-RSA-AES128-GCM-SHA256 bits\x128/128);        Fri, 07 Apr 2017 10:32:58 -0700 (PDT)
Subject: Re: [PATCH v5 2/5] Share parts of gdb/gdbthread.h with gdbserver
To: Sergio Durigan Junior <sergiodj@redhat.com>
References: <1482464361-4068-1-git-send-email-sergiodj@redhat.com> <20170330014915.4894-1-sergiodj@redhat.com> <20170330014915.4894-3-sergiodj@redhat.com> <aa25e55e-8414-e7a1-9c95-36c84b6c900d@redhat.com> <87fuhl3p3t.fsf@redhat.com>
Cc: GDB Patches <gdb-patches@sourceware.org>
From: Pedro Alves <palves@redhat.com>
Message-ID: <180989f1-5f85-be38-a6fc-ea24ccc35465@redhat.com>
Date: Fri, 07 Apr 2017 17:33:00 -0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <87fuhl3p3t.fsf@redhat.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
X-SW-Source: 2017-04/txt/msg00176.txt.bz2
Content-length: 3168

On 04/07/2017 03:53 AM, Sergio Durigan Junior wrote:
> On Friday, March 31 2017, Pedro Alves wrote:
>
>> On 03/30/2017 02:49 AM, Sergio Durigan Junior wrote:
>>> +struct thread_info *
>>> +add_thread_silent (ptid_t ptid)
>>> +{
>>> +  pid_t pid = ptid_get_pid (ptid);
>>> +
>>> +  /* Check if there is a process already.  */
>>> +  if (find_process_pid (pid) == NULL)
>>> +    add_process (pid, 0);
>>> +
>>> +  return add_thread (ptid_build (pid, pid, 0), NULL);
>>
>> This ptid_build here is always using the "pid" as
>> lwpid.  This suggests to me that "add_thread_silent" was not the
>> right function entry point to share, since we're adding a hack
>> to one of the implementations.  Either we need a new function,
>> like "add_initial_thread (pid_t pid)", or maybe we could move the
>> add_thread_silent call in fork_inferior to the gdb-side, only?
>
> OOC, could you expand on why using pid as the lwpid means that this is a
> hack?

Sure.  The function's purpose is creating a thread with a given
PTID, but while that is what currently happens on the gdb side, and
you're not changing it, the new implementation on the gdbserver side
ignores the ptid's fields except the PID.  It'd be reasonable for
some other future shared bits of code to call add_thread_silent on
other contexts with the lwp/tid fields of ptid filled in,
but it wouldn't work because of this hack.  Trying to fix that
issue when we stumble into this by making gdbserver's implementation
NOT ignore the passed-in ptid wouldn't work because it'd break the
fork-child path.  That's force us to fix it properly then and revisit
this exact issue we're discussing.  So I'd rather address it now,
upfront.

Note, while the right ptid for the initial thread for
Linux is "(pid,pid,0)", that's not the case for all/other ports.
So that hack above is also baking in a Linux-ism that happens to
work because gdbserver has fewer Unix-like ports than GDB.

>
> On a side note, it seems from your comment that the best alternative
> would be to move the add_thread_silent call to the GDB-specific
> postfork_hook, and undo the changes on gdbserver/{linux,lynx}-low.c that
> add the "*_update_process" functions.  This would mean that, on GDB, the
> prefork_hook would always call add_thread_silent, but on gdbserver each
> target would be responsible for adding the process/thread after calling
> fork_inferior.

It'd be nice to be able to normalize the APIs between gdb and gdbserver.

It sounds like if we make each target responsible for adding the right main
thread after fork_inferior returns on the gdb side too, then we'll be able
to get rid of this hack in linux-nat.c:linux_nat_wait_1 while at it:

  /* The first time we get here after starting a new inferior, we may
     not have added it to the LWP list yet - this is the earliest
     moment at which we know its PID.  */
  if (ptid_is_pid (inferior_ptid))
    {
      /* Upgrade the main thread's ptid.  */
      thread_change_ptid (inferior_ptid,
			  ptid_build (ptid_get_pid (inferior_ptid),
				      ptid_get_pid (inferior_ptid), 0));

      lp = add_initial_lwp (inferior_ptid);
      lp->resumed = 1;
    }

Thanks,
Pedro Alves

Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

On 04 Apr 2017 10:14, Alan Hayward wrote:
> Max size set to 32bits, which I determined using regformats/reg-bfin.dat
> 
> Tested on a --enable-targets=all build using make check with board files
> unix and native-gdbserver.
> 
> I do not have an BFIN machine to test on.

yes, Blackfin only has 32-bit registers.  it has two 40-bit accumulators
(A0 & A1), but they're read/written as 32-bit (A0.W) & 8-bit (A0.X) pairs.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]