Re: [PATCHv2 2/2] gdb/tui: asm window handles invalid memory and scrolls better

[Shahab Vahedi <Shahab.Vahedi@synopsys.com>]

Great job Andrew!

Functionality testing:
I have tested this patch and it is impossible to break it.

Code review:
The comments that have been added are very self explanatory.
The logic that is used seems sound and thorough. I have one
small remark though. For most part of the
tui_find_disassembly_address() function, there are usages of
new_low, prev_low, and possible_new_low variables. What these
variables are actually used for is finding a new _high_. I
suggest replacing every instance of "low/LOW" with "high/HIGH".
The region of code I am talking about is listed below:

region of code begins
  else
    {
      ...
      gdb::optional<CORE_ADDR> possible_new_low;
      ...
      CORE_ADDR prev_low;

      do
        {
          ...
        }
      while (...)
      ...
      if (asm_lines.size () < max_lines)
        {
          if (!possible_new_low.has_value ())
            return pc;

          new_low = *possible_new_low;
          next_addr = tui_disassemble (gdbarch, asm_lines,
                                       new_low, max_lines);
          ...
        }
region of code ends

Nevertheless, the code looks (very) fine to me as it is.


Cheers,
Shahab
From gdb-patches-return-163326-listarch-gdb-patches=sources.redhat.com@sourceware.org Tue Jan 21 16:27:30 2020
Return-Path: <gdb-patches-return-163326-listarch-gdb-patches=sources.redhat.com@sourceware.org>
Delivered-To: listarch-gdb-patches@sources.redhat.com
Received: (qmail 54158 invoked by alias); 21 Jan 2020 16:27:29 -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 53625 invoked by uid 89); 21 Jan 2020 16:27:29 -0000
Authentication-Results: sourceware.org; auth=none
X-Spam-SWARE-Status: No, score=-3.7 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.1 spammy=sometime, reader, smoke
X-HELO: esa1.hgst.iphmx.com
Received: from esa1.hgst.iphmx.com (HELO esa1.hgst.iphmx.com) (68.232.141.245) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 21 Jan 2020 16:27:19 +0000
DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple;  d=wdc.com; i=@wdc.com; q=dns/txt; s=dkim.wdc.com;  t\x1579624038; x\x1611160038;  hÚte:from:to:cc:subject:in-reply-to:message-id:   references:mime-version;  bh=Xn3j90Cj5upky+IQLmTyeKGLmWkcrVwjNqKUy99EAi8=;  b=e+JO5SFwFwpLuBiZJ1iN37Wcm8CVNheEZNfBI3Xp9g5GJblQ/4gzqEO6   gIpGM/eMhRRG3YirZpLJt2ET7VNHREbD517kdhG4uHVY5VtsTB85ATMNh   ZNP6b7aGTvqUnqP2u7QAvmFfBlvmA4+Z6GSyHu7xy2jhIdRldAoq62w6L   zsgMufo794Kv+yzxxSyTPnURLsnjMfD4iU5KRqau9gDW5LxkYpBZPuwaI   Qwl9Tj4fB50A2wbIP9+89m+YoGfuFWubFwJI39bxT8Gbv2ER6KppoC8T1   hfTZeJJHay/kCnyC1dsS4tKeEgiULOcNsX21t8LOksQT0wFy+hLWLcBus   A==;
IronPort-SDR: lhbgwV5TMlogd2j1izX2JenXpCh+G4xIPLRQpabDMZi2c5WDyHcmI9JNUe9DS0kEBiu++IJGKE WqYXpRAZ72/JdiKuh/8YMqLEpIagfglf8wwq+oWiSCY7O9b/26Qt3gPGkQCtTUWle6KSIAqxUD KMQcRVpzsnAoWr7dmNwJXVP+31Yex6J3ZGfcWwpjEb/PYZZh8/1O9aIFEcFAscXiYQO4ixhPkE 9TFFl8tOU50cy/qwzVCdkWFgj0nFPVZeC3poSKNy31bKJV36uuH/MitvURzvAcMdIWRi2Th2b0 UtUReceived: from uls-op-cesaip01.wdc.com (HELO uls-op-cesaep01.wdc.com) ([199.255.45.14])  by ob1.hgst.iphmx.com with ESMTP; 22 Jan 2020 00:27:16 +0800
IronPort-SDR: 9FbTDrH9+YYqFejxBm4VW3y1EOkuqVYdlUXAwCSVps3IYS41Pr2PnM628x3MVPqfAp/cGsz4V2 8pJorWJN18dpRkZbYYdTkd/vxjNrrrdv+G7HOxzZ4CoxfkScSlmMKOd+wWSCmyVB7Ix635XRbI gA3cxLcsZsRtEBd7FQwT+0OLQJ445aHDd+D7OnC1+XKsw3IgCKksejFi6UUjZ60/NZSlhd6Bcd XVTjwIhzjzsK8w/Y6BiUEX6iI4ElMEebH0YwOBdpz330cNRP0RMCyMeyJkQJ/JT/4qULM4Dbjf I9nZXT6yq3qJRZBf0QqqnKi+
Received: from uls-op-cesaip02.wdc.com ([10.248.3.37])  by uls-op-cesaep01.wdc.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Jan 2020 08:20:42 -0800
IronPort-SDR: Fh8P2YoXRYYAEd8Px6p1dEmC7PtxHWAWOzYxV6zOwuty8CBs4CqJbdT2NLfv90LLRIqAXFYecP 0zTK/Gmv3fRDyCtHb5O38H1wqCeriUAp9BmP8ii+n1BHnO4zCejEl2Lgm9AtL501zEH5clqscu MKKzExPfH5niPEiaaO6gpHDtXWw1MvEqGniss2lhNWsyvpXe7ZGbvJ9+sMsPJla64XvsQwz6Lf acYXEHLv+UbE3nQsq3721BbPehrF7qfEpfQByl5mhA7UGpQeKZ9Z4NgESKGI6s9dF2fT60zLSI hd4WDCIronportException: Internal
Received: from unknown (HELO redsun52) ([10.149.66.28])  by uls-op-cesaip02.wdc.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Jan 2020 08:27:15 -0800
Date: Tue, 21 Jan 2020 17:11:00 -0000
From: "Maciej W. Rozycki" <macro@wdc.com>
To: Andrew Burgess <andrew.burgess@embecosm.com>
cc: Simon Marchi <simark@simark.ca>, Jim Wilson <jimw@sifive.com>,     jiangshuai_li@c-sky.com, guoren@kernel.org, gdb-patches@sourceware.org,     =?UTF-8?B?5aSP56uL5pa5?= <lifang_xia@c-sky.com>,     yunhai_shang <yunhai_shang@c-sky.com>
Subject: Re: [PATCH] riscv: add gdbserver support
In-Reply-To: <20200121125657.GH3865@embecosm.com>
Message-ID: <alpine.LFD.2.21.2001211610190.15714@redsun52.ssa.fujisawa.hgst.com>
References: <00e401d5cb52$63a4d000$2aee7000$@c-sky.com> <CAFyWVaa7ADP_SmBVoan9AOkWK9parEz5EENZZL5vY+_GcD9SrA@mail.gmail.com> <alpine.LFD.2.21.2001202307240.15714@redsun52.ssa.fujisawa.hgst.com> <3a15e9f5-099f-3be0-e3f1-0e17c2959158@simark.ca> <20200121125657.GH3865@embecosm.com>
User-Agent: Alpine 2.21 (LFD 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
X-SW-Source: 2020-01/txt/msg00640.txt.bz2
Content-length: 1598

On Tue, 21 Jan 2020, Andrew Burgess wrote:

> I had planned to go back and review this code next w/e, but I might
> hold off now to see if the xml issue is addressed.

 I have given it some thought and I'll actually post the whole series of
my changes; there are 4 patches total, 3 of which addressing some bugs
before the actual `gdbserver' implementation is put on top of them (I
could ignore some of these bugs, but I couldn't persuade myself to put in
bug-compatible nonsense only to be fixed later on).  Since this whole
series does not address the XML arch acceptance bug (which I'm fairly sure
will require poking at BFD too) I do not think this can go in as it stands
anyway and I will not rerun full verification.

 I do need to do some smoke testing however, including for the bug fixes,
as Tom has been working on some build infrastructure rework and I had to
take it into account while updating the patches from their last October's
state.  I expect to be ready with this sometime tomorrow.

 There's a minor ptrace(2) Linux kernel API abuse bug in RISC-V arch code
that I find worth addressing while this work is being done.  I have a
patch for that too, which needs final verification.  By chance it doesn't
trigger in GDB's use, and it may not at all, but the way the RISC-V kernel
backend has been implemented mismatches the core Linux kernel API as
documented and is at the very least confusing to the reader.

 NB I have been verifying this work with the HiFive Unleashed real
hardware running Linux (including native support as a reference).

 FWIW,

  Maciej