From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17123 invoked by alias); 20 Jun 2013 20:42:52 -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 17111 invoked by uid 89); 20 Jun 2013 20:42:51 -0000 X-Spam-SWARE-Status: No, score=-4.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL autolearn=ham version=3.3.1 Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Thu, 20 Jun 2013 20:42:50 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1Uplgu-0002mr-1x from Maciej_Rozycki@mentor.com ; Thu, 20 Jun 2013 13:42:48 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by svr-orw-fem-01.mgc.mentorg.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Thu, 20 Jun 2013 13:42:47 -0700 Received: from [172.30.64.40] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.2.247.3; Thu, 20 Jun 2013 21:42:43 +0100 Date: Thu, 20 Jun 2013 20:45:00 -0000 From: "Maciej W. Rozycki" To: Pedro Alves CC: Yao Qi , Subject: Re: [PATCH 1/3] Include asm/ptrace.h in mips-linux-nat.c In-Reply-To: <51C350A6.1020302@redhat.com> Message-ID: References: <1369881867-11372-1-git-send-email-yao@codesourcery.com> <1369881867-11372-2-git-send-email-yao@codesourcery.com> <51BA8252.3010303@codesourcery.com> <51C350A6.1020302@redhat.com> User-Agent: Alpine 1.10 (DEB 962 2008-03-14) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-SW-Source: 2013-06/txt/msg00570.txt.bz2 On Thu, 20 Jun 2013, Pedro Alves wrote: > >>> ../../src/gdb/mips-linux-nat.c:599: error: storage size of 'dummy_regs' > >>> isn't known > >>> > >>> etc. for the very reason the system I used for testing has old kernel > >>> headers installed (2.6.19 it would seem, according to ). > >>> > >>> Such a failure is not acceptable from the user's point of view; I think > >>> there are three ways to deal with this: > >>> > >>> 1. Add an autoconf test that checks for the presence of a key > >>> definition; I think 'struct pt_watch_regs' is a good > >>> candidate. If that test does not succeed, then the configure process > >>> fails gracefully stating the minimum released version of kernel headers > >>> required. > >>> > >>> 2. Add the same test, except in the failure case fall back to the internal > >>> definitions we already have, wrapped into #ifndef > >>> HAVE_STRUCT_PT_WATCH_REGS. > >>> > >>> 3. Add the same test and disable hardware watchpoint support in the > >>> failure case. > >> > >> I prefer #3. If 'struct pt_watch_regs' is not defined, hardware watchpoint is > >> not supported in the kernel. Because 'struct pt_watch_regs' was added in this > >> commit in linux kernel, > >> > >> commit 0926bf953ee79b8f139741b442e5a18520f81705 > >> Author: David Daney > >> Date: Tue Sep 23 00:11:26 2008 -0700 > >> > >> MIPS: Ptrace support for HARDWARE_WATCHPOINTS > >> > >> This is the final part of the watch register patch. Here we hook up > >> ptrace so that the user space debugger (gdb), can set and read the > >> registers. > >> > >> Signed-off-by: David Daney > >> Signed-off-by: Ralf Baechle > > > > Well, kernel headers bundled with the C library installed on a system GDB > > is being built on do not necessarily have to match the kernel binary the > > system uses (as is the case with the system I'm using for this validation) > > or GDB may be run on another system. So you cannot infer at the build > > time whether the kernel used on a system GDB is going to run on supports > > watchpoints until you actually query the kernel at the run time; you need > > to do a run-time check anyway as hardware may not even if the kernel will. > > > > Therefore I think my question here is a matter of policy in GDB rather > > than limitations posed by a system in a given configuration. Which makes > > your preference valid, although not for the reason you stated. > > We don't have a written or very fixed policy, afaik. > > For new ports, we try to push back on adding such fallback definitions, > in order push the submitter into actually adding them to their > right place (the kernel headers). (otherwise, it's been somewhat common > that the defines don't actually make it there! E.g., PT_TEXT_ADDR > and friends...) Then, we prefer assuming minimum kernel headers version > the first released version with all the necessary bits for the port. That's pretty straightforward a case, agreed, especially as the same kernel has to know the definitions for its own use anyway. > For new features, things get a bit more blurry. > > Given kernel headers guarantee backwards compatibility, it's my > understanding that you should always be able to build against the > latest kernel headers, even if you're running against an old kernel. Well, the rule isn't exactly that -- the rule is the kernel headers installed in the system for use by user programs must be exactly those the C library (typically glibc) has been built against. Otherwise all hell is bound to break loose. Of course not everyone is prepared to build their own C library, and we cannot expect one to be. Actually the case is I believe even more complicated. The rule expressed by Linus as I remember it used to be not to make use of the kernel headers in user programs at all. Any definitions and declarations required for user programs were expected to be either folded into the relevant C library headers (e.g. glibc frequently chooses for such stuff), or bundled with the piece of user software that relies on the relevant subsystem (that used to be the case with the more obscure stuff, e.g. all the programs relying on the netlink stuff used to do that, and perhaps still do). What we currently have in GDB with regard to MIPS hardware watchpoint functionality carefully follows the second choice. Now times are changing and I reckon over the years a project has been under way to split the kernel headers into a strictly internal set and an exported user-ABI set, with the latter intended to make user program developers' lives easier. I haven't tracked progress there and I don't know offhand how far the project has gone, but unless you have a better knowledge of what the situation is there and can give good counterarguments, we certainly have to be careful especially with such an old Linux port the MIPS one is. > ISTR that's how CS builds things. However, naturally not everyone > will do that. Most will rely on the system's kernel version's > headers. Well, with my upstream reviewer's hat on I try to avoid assuming one's build environment is going to be as sophisticated as one used by those who professionally work on toolchain components. The minimal and probably still typical recipe of a random developer or even system administrator out there roughly is: $ ./configure && make && make install and I think our objective is to make this as smooth as possible, avoiding making people pull their hair. Professional software packagers will cope either way, we don't need to care that much about them. > Weighing the maintenance cost of a autoconf glue plus #ifdefs > around the feature's code, vs defining a couple fallback constants > ourselves, I'd say the latter is simpler and less effort, thus my > preference. If we're talking about a set of missing complicated > structure definitions, then I may have the opposite opinion. In principle I agree, however as I noted above, regrettably the history of the Linux user API has not made this choice any more obvious or easier. > In this case, I was borderline when I initially reviewed the > Daney's watchpoints's code, but I accepted it then. Given they're > already in place, it just sounds like causing unnecessary trouble > to me to remove them, more so you've shown an example where they're > necessary. I'd vote for keeping them. Supposedly, MAX_DEBUG_REGISTER > or some other convenient define was added to the kernel's asm/ptrace.h > at the same time as struct pt_watch_regs etc, so we could > still include asm/ptrace.h, and only define the fallbacks if > that such macro is not defined? We have the choice between PTRACE_GET_WATCH_REGS and PTRACE_SET_WATCH_REGS only here, there have been no other macros added with the Linux commit 0926bf953ee79b8f139741b442e5a18520f81705 Yao referred to above. The change regrettably has the maximum architectural number of watchpoint register sets possible to implement in hardware hardcoded throughout as literal 8 rather than using a macro like our MAX_DEBUG_REGISTER (a misnomer actually, as I noted elsewhere). Overall, I wonder if it's not actually all of this header stuff should be moved to, hmm. Maciej