From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1187 invoked by alias); 27 Jun 2014 10:39:53 -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 1173 invoked by uid 89); 27 Jun 2014 10:39:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 27 Jun 2014 10:39:49 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s5RAdmDp020823 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 27 Jun 2014 06:39:48 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s5RAdkOk005649; Fri, 27 Jun 2014 06:39:47 -0400 Message-ID: <53AD49F1.70709@redhat.com> Date: Fri, 27 Jun 2014 10:39:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Mark Kettenis , gbenson@redhat.com CC: gdb-patches@sourceware.org Subject: Re: [PATCH 0/7] Refactor shared code in {i386,amd64}-linux-nat.c References: <1403860209-475-1-git-send-email-gbenson@redhat.com> <201406270928.s5R9SZPG020219@glazunov.sibelius.xs4all.nl> In-Reply-To: <201406270928.s5R9SZPG020219@glazunov.sibelius.xs4all.nl> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-06/txt/msg00931.txt.bz2 On 06/27/2014 10:28 AM, Mark Kettenis wrote: > Sorry, no. Perhaps more code can be shared between i386-linux-nat.c > and amd64-linux-nat.c, but this goes too far and turns things into > #ifdef spagetthi. It also breaks established naming conventions. I think you may have stopped looking before patch #6. :-) Please also keep in mind that gdbserver also has its own copy of this code, and on the gdbserver side, we also started out with i386 and amd64 split in different files, and then they got merged: commit d0722149ad594a7d3892bb2fd53a72c6d4933793 Author: Doug Evans Date: Tue May 12 22:25:00 2009 +0000 Biarch support for i386/amd64 gdbserver. * Makefile.in (SFILES): Remove linux-i386-low.c, linux-x86-64-low.c. Add linux-x86-low.c. (linux-i386-low.o, linux-x86-64-low.o): Delete. (linux-x86-low.o): Add. ... Please look at the resulting x86-linux-nat.c, and gdbserver's existing linux-x86-low.c, the interim steps. (The latter we'll eventually want to merge gdb's code with.) This whole series can be found in git@github.com:gbenson/binutils-gdb.git gbenson/i386-dregs for convenience. I can't agree that this series went too far, on the contrary - gdbserver has everything in the linux-x86-low.c file, while Gary only moved the truly shared code. That's a fine incremental step IMNSHO. The resulting #ifdefery quite reasonable, IMNSHO -- it'd be bad if there were lots of different combinations of symbols we'd have to #ifdef on, but there's really only a few #ifdef __x86_64__. So if it's spaghetti, it's still dry, raw, straight and neatly lined up in the package. :-) As for naming, I suggested x86 myself. I think x86 for shared 32-bit/64-bit code, is quite an established convention itself. It's what we already use in gdbserver, and at least glibc and the Linux kernel use x86 for shared code too. E.g.: $ ls glibc/src/sysdeps/ | grep 86 i386 x86 x86_64 I'd _really_, _really_ love to see this go forward (really), and reduce the amount of places one has to touch when hacking on watchpoint stuff. and when adding new target descriptions, etc., from 3 places, to 2 places. See https://sourceware.org/ml/gdb-patches/2014-06/msg00773.html for a patch that just recently still had to touch 3 places with the same code. And going forward, I'd like to reduce that to 1 place only, even. Please take a look at the resulting code past patch #5, I believe you'll be pleasantly surprised! :-) -- Pedro Alves