From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 46544 invoked by alias); 5 Apr 2017 16:01:05 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 44799 invoked by uid 89); 5 Apr 2017 16:01:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.4 required=5.0 tests=BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS,UNSUBSCRIBE_BODY autolearn=ham version=3.3.2 spammy=secondly X-HELO: mail-wr0-f179.google.com Received: from mail-wr0-f179.google.com (HELO mail-wr0-f179.google.com) (209.85.128.179) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 05 Apr 2017 16:01:02 +0000 Received: by mail-wr0-f179.google.com with SMTP id w43so20169005wrb.0 for ; Wed, 05 Apr 2017 09:01:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-transfer-encoding; bh=RrKmn2LQlcXuRcW5CkKkHGaPwqFE1EhjiwCdk6SVOlg=; b=V8mQj5oUWjd2G481ThxsMHZERRKEjWnt9N/FkIAmfSz2q1zzDHcxarSRCt34TSldCZ ynKoeZGzR95Vm7NmPHfMAplFltiIfNWsfeH5dj1d/p3cL4/l3hu2Zpz2NNRvfIoFBeJz pZFg+hlweU8l1h6iw/VTK8IW0MuN3N39aFJ4SDP/oA1Jh0qCUZbUEUgagi+4rugLhI8b VAe2hrPfHocm9/U4BiauyHNS0VqwTsNE3fMLSrNiG/Mj6eQN3VyxCX1xT1G72ASMCbHZ mLpAMFXVIBMIqvYTiCxC0lV0BT1eeXFQCY9EXYqi26qO/u+GscdBxNLrb9LhmyIivt0p 5Eog== X-Gm-Message-State: AFeK/H0YQSXE0K+KPpSUychQSm3zPkAwqqgufMEokmXuYs+bNtMAVK1Pppa0Fh5F/+89oQ== X-Received: by 10.223.139.142 with SMTP id o14mr26579789wra.9.1491408061170; Wed, 05 Apr 2017 09:01:01 -0700 (PDT) Received: from E107787-LIN ([194.214.185.158]) by smtp.gmail.com with ESMTPSA id t68sm26821125wrc.55.2017.04.05.09.00.59 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Wed, 05 Apr 2017 09:01:00 -0700 (PDT) From: Yao Qi To: Alan Hayward Cc: "gdb-patches\@sourceware.org" , Subject: Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c References: <562B2F6F-F3C6-4A76-9489-57539F396C94@arm.com> <868tnvukjh.fsf@gmail.com> <7359B5C0-BF61-42E2-9886-B322C1825865@arm.com> <0DADF920-69B9-4F96-A153-6965E56B5DA8@arm.com> Date: Wed, 05 Apr 2017 16:01:00 -0000 In-Reply-To: <0DADF920-69B9-4F96-A153-6965E56B5DA8@arm.com> (Alan Hayward's message of "Wed, 5 Apr 2017 14:53:47 +0000") Message-ID: <868tneq1xj.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00116.txt.bz2 Alan Hayward writes: > diff --git a/gdb/msp430-tdep.c b/gdb/msp430-tdep.c > index 75329dfcc5ed94fff19639db4db21dd0874d0e96..d9eebf0cc2647a079db2f8221= 45d0fb74ea301e4 100644 > --- a/gdb/msp430-tdep.c > +++ b/gdb/msp430-tdep.c > @@ -221,10 +221,9 @@ msp430_pseudo_register_read (struct gdbarch *gdbarch, > struct regcache *regcache, > int regnum, gdb_byte *buffer) > { > - enum register_status status =3D REG_UNKNOWN; > - > if (MSP430_NUM_REGS <=3D regnum && regnum < MSP430_NUM_TOTAL_REGS) > { > + enum register_status status; > ULONGEST val; > enum bfd_endian byte_order =3D gdbarch_byte_order (gdbarch); > int regsize =3D register_size (gdbarch, regnum); > @@ -234,11 +233,10 @@ msp430_pseudo_register_read (struct gdbarch *gdbarc= h, > if (status =3D=3D REG_VALID) > store_unsigned_integer (buffer, regsize, byte_order, val); > > + return status; > } > else > gdb_assert_not_reached ("invalid pseudo register number"); > - > - return status; > } This looks reasonable to me, but could you put it into a separate patch so that people interested in msp430 may take a look? > > /* Implement the "pseudo_register_write" gdbarch method. */ > diff --git a/gdb/nds32-tdep.c b/gdb/nds32-tdep.c > index 05c48aa27d84bc0286712f0143a9447a79ae066b..e9955d60a8132d5bc3af234df= f0c86c8e59d4855 100644 > --- a/gdb/nds32-tdep.c > +++ b/gdb/nds32-tdep.c > @@ -445,11 +445,11 @@ nds32_pseudo_register_read (struct gdbarch *gdbarch, > struct gdbarch_tdep *tdep =3D gdbarch_tdep (gdbarch); > gdb_byte reg_buf[8]; > int offset, fdr_regnum; > - enum register_status status =3D REG_UNKNOWN; > + enum register_status status; > > /* Sanity check. */ > - if (tdep->fpu_freg =3D=3D -1 || tdep->use_pseudo_fsrs =3D=3D 0) > - return status; > + gdb_assert (tdep->fpu_freg !=3D -1); > + gdb_assert (tdep->use_pseudo_fsrs > 0); > The nds32 change can go to a separate patch as well, so that nds32 people can take a look too. Secondly, it is not easy to see your change is right or not, unless I read the code in nds32_gdbarch_init, if (fpu_freg =3D=3D -1) num_regs =3D NDS32_NUM_REGS; else if (use_pseudo_fsrs =3D=3D 1) { set_gdbarch_pseudo_register_read (gdbarch, nds32_pseudo_register_read= ); set_gdbarch_pseudo_register_write (gdbarch, nds32_pseudo_register_wri= te); so please add some comments in the assert like /* This function is only registered when fpu_regs !=3D -1 and use_pseudo_fsrs =3D=3D 1 in nds32_gdbarch_init. */ gdb_assert (tdep->fpu_freg !=3D -1); gdb_assert (tdep->use_pseudo_fsrs =3D=3D 1); > regnum -=3D gdbarch_num_regs (gdbarch); > > @@ -466,9 +466,11 @@ nds32_pseudo_register_read (struct gdbarch *gdbarch, > status =3D regcache_raw_read (regcache, fdr_regnum, reg_buf); > if (status =3D=3D REG_VALID) > memcpy (buf, reg_buf + offset, 4); > + > + return status; > } > > - return status; > + gdb_assert_not_reached ("invalid pseudo register number"); > } also, it would be nice to do the same change in nds32_pseudo_register_write too. > @@ -379,7 +388,7 @@ regcache_restore (struct regcache *dst, > void *cooked_read_context) > { > struct gdbarch *gdbarch =3D dst->descr->gdbarch; > - gdb_byte buf[MAX_REGISTER_SIZE]; > + std::vector buf (max_register_size (gdbarch)); > int regnum; > > /* The dst had better not be read-only. If it is, the `restore' > @@ -395,9 +404,9 @@ regcache_restore (struct regcache *dst, > { > enum register_status status; Can we move "buf" here? and initialize it with the register_size, std::vector buf (register_size (gdbarch, regnum)); then, we don't need max_register_size (). > @@ -1480,17 +1488,19 @@ regcache_dump (struct regcache *regcache, struct = ui_file *file, > fprintf_unfiltered (file, "Cooked value"); > else > { > - enum register_status status; > + struct value *value =3D regcache_cooked_read_value (regcache, > + regnum); > > - status =3D regcache_cooked_read (regcache, regnum, buf); > - if (status =3D=3D REG_UNKNOWN) > - fprintf_unfiltered (file, ""); > - else if (status =3D=3D REG_UNAVAILABLE) > + if (value_optimized_out (value) > + || !value_entirely_available (value)) > fprintf_unfiltered (file, ""); It is still not right to me. With your changes to msp430 and nds32, we won't get REG_UNKNOWN for pseudo registers, but we may still get REG_UNKNOWN from raw registers (from regcache->register_status[]). How is this? gdb_byte *buf =3D NULL; enum register_status status; struct value * value =3D NULL; if (regnum < regcache->descr->nr_raw_registers) { regcache_raw_update (regcache, regnum); status =3D regcache->register_status[regnum]; buf =3D register_buffer (regcache, regnum); } else { value =3D regcache_cooked_read_value (regcache, regnum); if (value_entirely_available (value)) { status =3D REG_VALID; buf =3D value_contents_all (value); } else status =3D REG_REG_UNAVAILABLE; } if (status =3D=3D REG_UNKNOWN) fprintf_unfiltered (file, ""); else if (status =3D=3D REG_UNAVAILABLE) fprintf_unfiltered (file, ""); else print_hex_chars (file, buf, regcache->descr->sizeof_register[regnum], gdbarch_byte_order (gdbarch)); if (value !=3D NULL) { release_value (value); value_free (value); } --=20 Yao (=E9=BD=90=E5=B0=A7)