From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 849 invoked by alias); 14 Jul 2004 23:28:38 -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 825 invoked from network); 14 Jul 2004 23:28:36 -0000 Received: from unknown (HELO mx1.redhat.com) (66.187.233.31) by sourceware.org with SMTP; 14 Jul 2004 23:28:36 -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 i6ENSae1021515 for ; Wed, 14 Jul 2004 19:28:36 -0400 Received: from zenia.home.redhat.com (porkchop.devel.redhat.com [172.16.58.2]) by int-mx1.corp.redhat.com (8.11.6/8.11.6) with ESMTP id i6ENSX012842; Wed, 14 Jul 2004 19:28:34 -0400 To: Kevin Buettner Cc: gdb-patches@sources.redhat.com Subject: Re: RFA: PowerPC: add segment register numbers References: <20040712170129.47f4bc06@saguaro> From: Jim Blandy Date: Wed, 14 Jul 2004 23:28:00 -0000 In-Reply-To: <20040712170129.47f4bc06@saguaro> Message-ID: User-Agent: Gnus/5.09 (Gnus v5.9.0) Emacs/21.3 MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2004-07/txt/msg00177.txt.bz2 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. *************** 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.) > 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.