From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13668 invoked by alias); 31 Jan 2003 21:26:58 -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 13658 invoked from network); 31 Jan 2003 21:26:56 -0000 Received: from unknown (HELO zenia.red-bean.com) (66.244.67.22) by 172.16.49.205 with SMTP; 31 Jan 2003 21:26:56 -0000 Received: from zenia.red-bean.com (localhost.localdomain [127.0.0.1]) by zenia.red-bean.com (8.12.5/8.12.5) with ESMTP id h0VLLD8A026404; Fri, 31 Jan 2003 16:21:13 -0500 Received: (from jimb@localhost) by zenia.red-bean.com (8.12.5/8.12.5/Submit) id h0VLLCD9026400; Fri, 31 Jan 2003 16:21:12 -0500 To: Gerhard Tonn Cc: gdb-patches@sources.redhat.com Subject: Re: [Patch] Fix ABI incompatibilities on s390x References: <03012312571000.14253@fmtc804> From: Jim Blandy Date: Fri, 31 Jan 2003 21:26:00 -0000 In-Reply-To: <03012312571000.14253@fmtc804> Message-ID: User-Agent: Gnus/5.09 (Gnus v5.9.0) Emacs/21.2.92 MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2003-01/txt/msg00850.txt.bz2 Gerhard Tonn writes: > "Gerhard Tonn" writes: > >> I don't think that it is worthwhile to duplicate the functions. The only > >> difference between s390 and s390x is really the REGISTER_SIZE. I will > >> rework the patch so that this becomes more clear. I will also address the > >> other issues. > > >Certainly, if two functions can be made identical by the appropriate > >use of REGISTER_SIZE, then there's no reason to duplicate them. > >However, I thought I had seen another sort of conditionality which is > >definitely of the sort we are trying to avoid. > > >I'll watch for your new patches. Thanks again for your work. > > Please find below the revised patch. The number of floating point registers is > another difference between s390 and s390x besides the register size. > > As soon as you have approved the patch we will start the copyright assignment . I > will let you know any progress. I'm afraid I have the same concerns about this patch as I did about the first one. It seems that there are two main differences between the s390 and s390x ABI's: - the s390 registers are larger. - the s390x does not have a special DOUBLE_ARG class of arguments that it handles specially. Most of the first sort of difference you can account for using REGISTER_SIZE, but there is one case that cannot be handled this way. You handle this by testing GDB_TARGET_IS_ESAME. Since there are several bits of code that know about the existence of DOUBLE_ARGs (i.e., we have to exclude DOUBLE_ARGs from the other classes of arguments), you handle each of those by testing GDB_TARGET_IS_ESAME. Please handle the first case by defining a function: static int is_power_of_two (unsigned int n) { return ((n & (n-1)) == 0); } and then saying ! (is_power_of_two (length) && length <= REGISTER_SIZE) in pass_by_copy_ref. Please handle the second case by explicitly calling is_double_arg from is_simple_arg, and then change is_double_arg as follows: static int is_double_arg (struct type *type) { unsigned length = TYPE_LENGTH (type); /* The s390x ABI doesn't handle DOUBLE_ARGS specially. */ if (GDB_TARGET_IS_ESAME) return 0; return ((is_integer_like (type) || is_struct_like (type)) && length == 8); } I'll be happy to review the patch again once these changes have been made. A while back, I spent several weeks fixing bugs in the parameter passing code of the s390 GDB port that had just been contributed --- bugs that could have been found and fixed by anyone willing to run the test suite. But this is not unusual; of the half-dozen or so gdb ports I've been asked to fix over the years, the parameter passing code is the part that is most often broken. Those sorts of problems usually occupy the better part of the time required to bring the test suite into good shape for a given architecture. I think other GDB developers with exposure to a broad range of architectures can cite similar experiences. So I think it is very important that we not waste future developers' time by allowing the same sorts of coding practices that have led to such poor reliability in the past. As I said in my first review of the patch, one of the hallmarks of parameter-passing code that will not survive the test of time is code that is shared between several ABI variants. Each variant has its quirks (i.e., the special handling, or lack thereof, for DOUBLE_ARG arguments), and its bugs (which are not always carried from one variant to another, as the double and float singleton bug was on the s390 family), and in the end, being able to cope with each ABI as a separate entity is the better approach. What I'm suggesting above is a compromise, that at least localizes the architecture tests to a single place, which is clearly related to the important distinction between the ABI's.