From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19179 invoked by alias); 8 Nov 2007 21:31:41 -0000 Received: (qmail 19156 invoked by uid 22791); 8 Nov 2007 21:31:36 -0000 X-Spam-Check-By: sourceware.org Received: from mtagate4.de.ibm.com (HELO mtagate4.de.ibm.com) (195.212.29.153) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 08 Nov 2007 21:31:31 +0000 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate4.de.ibm.com (8.13.8/8.13.8) with ESMTP id lA8LVS8r057072 for ; Thu, 8 Nov 2007 21:31:28 GMT Received: from d12av02.megacenter.de.ibm.com (d12av02.megacenter.de.ibm.com [9.149.165.228]) by d12nrmr1607.megacenter.de.ibm.com (8.13.8/8.13.8/NCO v8.6) with ESMTP id lA8LVSVk2207886 for ; Thu, 8 Nov 2007 22:31:28 +0100 Received: from d12av02.megacenter.de.ibm.com (loopback [127.0.0.1]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id lA8LVSlb015944 for ; Thu, 8 Nov 2007 22:31:28 +0100 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.12.11) with SMTP id lA8LVSk9015941; Thu, 8 Nov 2007 22:31:28 +0100 Message-Id: <200711082131.lA8LVSk9015941@d12av02.megacenter.de.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Thu, 8 Nov 2007 22:31:28 +0100 Subject: Re: [rfc] [02/05] Get rid of current_gdbarch in hppa-linux-nat.c To: deuling@de.ibm.com (Markus Deuling) Date: Thu, 08 Nov 2007 21:31:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org, mark.kettenis@xs4all.nl (Mark Kettenis), drow@false.org (Daniel Jacobowitz) In-Reply-To: <4731D0B9.8000809@de.ibm.com> from "Markus Deuling" at Nov 07, 2007 03:50:33 PM X-Mailer: ELM [version 2.5 PL2] MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: 2007-11/txt/msg00179.txt.bz2 Markus Deuling wrote: > Daniel Jacobowitz schrieb: > > That doesn't make any sense. The error is there to make sure we don't > > access memory outside the array. > > What this patch does is transform > > if ((unsigned) regno >= gdbarch_num_regs (current_gdbarch)) > error (_("Invalid register number %d."), regno); > > into > > if ((unsigned) regno >= gdbarch_num_regs (gdbarch)) > error (_("Invalid register number %d."), regno); > > The gdbarch comes from regcache in {fetch,store}_register. Why is that wrong ? It is not that this is *wrong* (this is, the code will continue to work correctly after this change), it just doesn't make any sense; I fully agree with Mark and Dan on this. Maybe it helps to think of how that code originally looked, before all the multi-arch related changes. A couple of years ago, platform back-end header files used to define a number of target macros, including NUM_REGS, to constants in their tm.h header files. These macros would be used throughout the debugger, both in the back end and in common code; any particular GDB build could only support one architecture variant. This changed when the gdbarch mechanism was introduced; it initially allowed back-end to support multiple variants of an architecture at the same time. To provide for some backwards compatibility, the code would continue to use macros like NUM_REGS, but those would now be defined by the gdbarch header file to resolve to the proper values dynamically, depending on the current architecture. For uses of those macros in common code that supports many different architectures, this flexibility was actually a good thing. However, for some back-end code that by its fundamental design only supports one single architecture variant anyway, this doesn't really make sense, it just adds unnecessary complexity. However, as the code still used the old macros, this wasn't really very obvious. Now, as we've completed the transition away from target macros, and all code uses gdbarch calls directly, it has become obvious that in some places, we should not have continued to use the generic macro, but gone back to whatever constant is appropriate for that file. To take the function your patch changes as example: static CORE_ADDR hppa_linux_register_addr (int regno, CORE_ADDR blockend) { CORE_ADDR addr; if ((unsigned) regno >= gdbarch_num_regs (current_gdbarch)) error (_("Invalid register number %d."), regno); if (u_offsets[regno] == -1) addr = 0; else { addr = (CORE_ADDR) u_offsets[regno]; } return addr; } This function is was clearly written with the expectation that the value of NUM_REGS (now replaced) is a constant; in fact, the expectation is that this constant is identical to the size of the u_offsets array. If -in the current code- gdbarch_num_regs (current_gdbarch) were to actually return anything *else* but that constant, the code would simply break. (Of course, it never does.) Thus, the correct change to this file would be to go back and replace the misleading "flexibility" of a gdbarch call by the value that's actually needed here; this could be written as ARRAY_SIZE (u_offsets). (Even better might be to move hppa32_num_regs as a macro to hppa-tdep.h, and declare that array with specified size to begin with.) Your patch goes into the completely opposite direction, by adding a parameter to the routine to pass in a variable gdbarch -- not only does this add unnecessary runtime overhead, but even worse, it reinforces the impression that there is really something variable here, when in fact that value really still must be constant (128) here. Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com