Re: [PATCH 7/11] Add BFIN_MAX_REGISTER_SIZE

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

[-- 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