From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10146 invoked by alias); 15 Jul 2004 00:22:45 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 10139 invoked from network); 15 Jul 2004 00:22:44 -0000 Received: from unknown (HELO mx1.redhat.com) (66.187.233.31) by sourceware.org with SMTP; 15 Jul 2004 00:22:44 -0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.12.10/8.12.10) with ESMTP id i6F0Mhe1000573 for ; Wed, 14 Jul 2004 20:22:44 -0400 Received: from pobox.corp.redhat.com (pobox.corp.redhat.com [172.16.52.156]) by int-mx1.corp.redhat.com (8.11.6/8.11.6) with ESMTP id i6F0Mh024990 for ; Wed, 14 Jul 2004 20:22:43 -0400 Received: from localhost.localdomain (vpn50-37.rdu.redhat.com [172.16.50.37]) by pobox.corp.redhat.com (8.12.8/8.12.8) with ESMTP id i6F0Mh38022197; Wed, 14 Jul 2004 20:22:43 -0400 Received: from saguaro (saguaro.lan [192.168.64.2]) by localhost.localdomain (8.12.11/8.12.10) with SMTP id i6F0MbMJ030633; Wed, 14 Jul 2004 17:22:37 -0700 Date: Thu, 15 Jul 2004 00:22:00 -0000 From: Kevin Buettner To: Jim Blandy Cc: gdb-patches@sources.redhat.com Subject: Re: RFA: PowerPC: add segment register numbers Message-Id: <20040714172237.7f1e26b0@saguaro> In-Reply-To: References: <20040712170129.47f4bc06@saguaro> Organization: Red Hat Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-SW-Source: 2004-07/txt/msg00179.txt.bz2 On 14 Jul 2004 18:23:30 -0500 Jim Blandy wrote: > > Kevin Buettner writes: > > *************** rs6000_gdbarch_init (struct gdbarch_info > > *** 2879,2884 **** > > --- 2879,2885 ---- > > tdep->ppc_mq_regnum = -1; > > tdep->ppc_fp0_regnum = 32; > > tdep->ppc_fpscr_regnum = power ? 71 : 70; > > + tdep->ppc_sr0_regnum = 71; > > > > ...what about the case when ``power'' is non-zero? Do we want > > ppc_fpscr_regnum and ppc_sr0_regnum to both be equal to 71? > > 'power' is initialized like this: > > power = arch == bfd_arch_rs6000; > > So ppc_sr0_regnum gets cleared below, whenever 'power' is true. I see. Thanks for the explanation. > *************** rs6000_gdbarch_init (struct gdbarch_info > *** 2907,2913 **** > else > tdep->lr_frame_offset = 8; > > ! if (v->arch == bfd_arch_powerpc) > switch (v->mach) > { > case bfd_mach_ppc: > --- 2908,2916 ---- > else > tdep->lr_frame_offset = 8; > > ! if (v->arch == bfd_arch_rs6000) > ! tdep->ppc_sr0_regnum = -1; > ! else if (v->arch == bfd_arch_powerpc) > switch (v->mach) > { > case bfd_mach_ppc: > > I'd be happy to tweak rs6000_gdbarch_init to consistently use 'power' > or 'arch == bfd_arch_rs6000' if you think either would be clearer. (I > would tend to prefer the latter: that would be one less step readers > would need to follow back through, and the code doesn't get too much > mileage out of the 'power' abbreviation anyway.) A patch which does the latter would be welcome. Consider it preapproved. > > Now the comment: Regarding the following portion of the patch... > > > > > *************** rs6000_gdbarch_init (struct gdbarch_info > > > *** 2939,2945 **** > > > --- 2943,2961 ---- > > > set_gdbarch_pseudo_register_read (gdbarch, e500_pseudo_register_read); > > > set_gdbarch_pseudo_register_write (gdbarch, e500_pseudo_register_write); > > > break; > > > + > > > + case bfd_mach_ppc64: > > > + case bfd_mach_ppc_620: > > > + case bfd_mach_ppc_630: > > > + case bfd_mach_ppc_a35: > > > + case bfd_mach_ppc_rs64ii: > > > + case bfd_mach_ppc_rs64iii: > > > + /* These processor's register sets don't have segment registers. */ > > > + tdep->ppc_sr0_regnum = -1; > > > + break; > > > } > > > + else > > > + gdb_assert (0); > > > > ...I don't really like the gdb_assert (0). > > > > With this patch, the code is structured as follows: > > > > if (v->arch == bfd_arch_powerpc) > > switch (v->mach) > > { > > ... > > } > > else > > gdb_assert (0); > > > > Could you revise your patch so that the above is instead structured > > like this...? > > > > gdb_assert(v->arch == bfd_arch_powerpc); > > switch (v->mach) > > { > > ... > > } > > > > BTW, the reason I don't like the assert (0) is that I like seeing a > > somewhat meaningful condition when an assert is triggered. Seeing 0 > > as the failed condition doesn't give much of a clue about what's going > > on. > > That's true. The problem with writing the condition out in the assert > is that one repeats the series of conditions that go before it. So > you're checking for one potential inconsistency, but introducing a new > one. > > As we discussed on the phone, calling internal_error and providing a > suitable error message explicitly, addresses both concerns. I'll > revise the patch. Thanks! Kevin