From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28556 invoked by alias); 15 May 2003 19:03:00 -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 28521 invoked from network); 15 May 2003 19:02:59 -0000 Received: from unknown (HELO localhost.redhat.com) (24.157.166.107) by sources.redhat.com with SMTP; 15 May 2003 19:02:59 -0000 Received: from redhat.com (localhost [127.0.0.1]) by localhost.redhat.com (Postfix) with ESMTP id 4B7EB2B2F; Thu, 15 May 2003 15:02:45 -0400 (EDT) Message-ID: <3EC3E455.9080100@redhat.com> Date: Thu, 15 May 2003 19:03:00 -0000 From: Andrew Cagney User-Agent: Mozilla/5.0 (X11; U; NetBSD macppc; en-US; rv:1.0.2) Gecko/20030223 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Kevin Buettner Cc: gdb-patches@sources.redhat.com Subject: Re: [patch rfc] Eliminate extract_address References: <3EC23225.4090605@redhat.com> <1030514164201.ZM9355@localhost.localdomain> <3EC3C50F.1060700@redhat.com> <1030515182039.ZM13780@localhost.localdomain> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2003-05/txt/msg00248.txt.bz2 > On May 15, 12:49pm, Andrew Cagney wrote: > > >> > First, the return types are different. extract_address() returns >> > CORE_ADDR while extract_unsigned_integer returns ULONGEST. If >> > we were to encounter a scenario where this is a problem, it's easier >> > to fix a wrapper (extract_address()) instead of the myriad places in >> > the code which presently call extract_address(). (This point is >> > probably moot because I suspect we already have a lot of code which >> > assumes that CORE_ADDR may be interchanged with LONGEST or ULONGEST >> > anyway.) > >> >> sizeof(CORE_ADDR) <= sizeof(ULONGEST) so this isn't a problem. > > > Do we have a gdb_assert() somewhere to ensure that this is the case? > (This could happen at initialization time...) Magic in "defs.h" does it. An assert wouldn't hurt. >> > Second, having function calls to extract_address() provides >> > information to the reader that you don't get by having calls to >> > extract_unsigned_integer(). It tells the reader that we're expecting >> > to get an address and not an integer. This really helps when someone >> > reading gdb's code is wondering about what the thing is that's being >> > extracted. > >> >> The extract_address function doesn't extract an address, it extracts an >> unsigned integer. >> On the MIPS, extract_address needs to sign extend. On the d10v, extract >> address needs to know the address space. > > > Yes, I understand that. Doing the substitution you propose will make > it more difficult to make the correct fix (of using extract_typed_address) > at a later time. > > >> If the code needs to extract an address it can use extract_typed_address >> which corectly handles all these cases. > > > Yes. > > >> Is it a good thing? It eliminates a lie. > > > At the expense of making the code marginally less comprehensible and > making it more difficult to identify the potential cases where > extract_typed_address() should be used instead. I think it makes it more comprehensible - it is now very clear exactly how the value is being obtained. The ``extract_address'' function gives the misleading impression that it is correctly extracting an address, and that (per MIPS and d10v) isn't the case. It also takes away the assumption that extract_address can, some how, be made cross architecture. > Or have all of those cases already been identified? If so, then I > withdraw my objection. (Though I still like having "address" in the > function name to help to document what it is that's being extracted.) It tinkers with the following: - ada/jv-* where things are pretty broken - dwarf2 which is extracting/assuming an an unsigned integer - unsigned_pointer_to_address making its implementation consistent with signed_pointer_to_address - solib* where it is now (worryingly) clear what the code is doing. - stack.c where it's printing out an integer value After that, it's all target dependant code. Andrew