From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5354 invoked by alias); 9 Nov 2012 00:10:37 -0000 Received: (qmail 5343 invoked by uid 22791); 9 Nov 2012 00:10:34 -0000 X-SWARE-Spam-Status: No, hits=-7.8 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 09 Nov 2012 00:10:21 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qA90AImk018172 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 8 Nov 2012 19:10:18 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id qA90A8C5004951; Thu, 8 Nov 2012 19:10:17 -0500 Message-ID: <509C49E0.8070404@redhat.com> Date: Fri, 09 Nov 2012 00:10:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20121016 Thunderbird/16.0.1 MIME-Version: 1.0 To: Tom Tromey CC: Joel Brobecker , gdb-patches@sourceware.org Subject: Re: [RFC] Document replacement for frame_register_read (deprecated). References: <1350941919-9862-1-git-send-email-brobecker@adacore.com> <87objtf6mu.fsf@fleche.redhat.com> In-Reply-To: <87objtf6mu.fsf@fleche.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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 X-SW-Source: 2012-11/txt/msg00223.txt.bz2 On 10/23/2012 06:19 PM, Tom Tromey wrote: >>>>>> "Joel" == Joel Brobecker writes: > > Joel> This is inspired by a discussion from a long time ago: > Joel> http://www.sourceware.org/ml/gdb-patches/2011-03/msg01095.html > > Joel> If you agree with the patch, we will also have to update the ARI > Joel> script to mention get_frame_register_value as the alternative. > > Joel> And we might also want to rename the function as well. I don't think > Joel> we can rely on the ARI alone to avoid new uses of this function. > Joel> And once new uses are in, it's much harder to make sure we undo them. > > Joel> Thoughts? > > The patch seems reasonable to me. Me too. > > Renaming seems like a decent idea, since there is a use in jit.c, which > was added long after this was deprecated. Yeah. > I count 22 mentions (including the declaration) of frame_register_read > in the tree right now. Getting rid of all of them doesn't seem so > difficult, aside from the testing. This would be clearly better... > > At least one of the current uses seems suspicious since it ignores the > result. I assume I'm missing some context though. I took a look too. > frame.c:775:do_frame_register_read (void *src, int regnum, gdb_byte *buf) > frame.c:777: if (!frame_register_read (src, regnum, buf)) Probably innocuous, but need to check if there's a path that calls this for frames other than #0, or if possible to get here for cores and tracing (IIRC, this is used for infcalls, which can't work on those cases). If so, since failure always maps to REG_UNAVAILABLE, we're losing info here. > frame.c:1218: frame_register_read (frame, regnum, buf); A read-modify-write that doesn't take in account whether the read was successful. Can't be good. > i386-tdep.c:1748: && frame_register_read (this_frame, cache->saved_sp_reg, buf)) I think it's okay. > infcmd.c:2052: if (! frame_register_read (frame, i, value_contents_raw (val))) I guess we should be saying "*optimized out*" if the register is not saved, and leave "*value not available*" for the case where we can't get at it because it's missing from the truncated core or from the tracebuffer. > jit.c:1056: value->defined = frame_register_read (priv->this_frame, gdb_reg, No idea. > mips-tdep.c:4606: if (!frame_register_read (frame, regno, raw_buffer)) > mips-tdep.c:4643: if (!frame_register_read (frame, regno, rare_buffer)) > mips-tdep.c:4879: if (!frame_register_read (frame, regnum, raw_buffer)) > mt-tdep.c:688: frame_register_read (frame, regnum, buff); > mt-tdep.c:713: frame_register_read (frame, MT_COPRO_REGNUM, buf); > mt-tdep.c:734: frame_register_read (frame, MT_MAC_REGNUM, buf); > mt-tdep.c:740: frame_register_read (frame, MT_EXMAC_REGNUM, buf); > sh64-tdep.c:2054: if (!frame_register_read (frame, regnum, raw_buffer)) > sh64-tdep.c:2170: if (!frame_register_read (frame, regnum, raw_buffer)) All these should be fixed to handle unavailableness gracefully. It just hasn't been pressing no target with with arch can do tracepoints, AFAIK (though there are other ways the registers could be missing). -- Pedro Alves