From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1803 invoked by alias); 20 Oct 2003 20:13:28 -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 1796 invoked from network); 20 Oct 2003 20:13:27 -0000 Received: from unknown (HELO mx1.redhat.com) (66.187.233.31) by sources.redhat.com with SMTP; 20 Oct 2003 20:13:27 -0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.11.6/8.11.6) with ESMTP id h9KKDRM12349 for ; Mon, 20 Oct 2003 16:13:27 -0400 Received: from pobox.corp.redhat.com (pobox.corp.redhat.com [172.16.52.156]) by int-mx1.corp.redhat.com (8.11.6/8.11.6) with ESMTP id h9KKDMr25973; Mon, 20 Oct 2003 16:13:22 -0400 Received: from localhost.localdomain (vpn50-50.rdu.redhat.com [172.16.50.50]) by pobox.corp.redhat.com (8.12.8/8.12.8) with ESMTP id h9KKDLwC031181; Mon, 20 Oct 2003 16:13:21 -0400 Received: (from kev@localhost) by localhost.localdomain (8.11.6/8.11.6) id h9KKDG220660; Mon, 20 Oct 2003 13:13:16 -0700 Date: Mon, 20 Oct 2003 20:13:00 -0000 From: Kevin Buettner Message-Id: <1031020201315.ZM20659@localhost.localdomain> In-Reply-To: "J. Johnston" "RFA: ia64 tdep patch" (Oct 17, 3:58pm) References: <3F9049EF.8060209@redhat.com> To: "J. Johnston" , gdb-patches@sources.redhat.com Subject: Re: RFA: ia64 tdep patch MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2003-10/txt/msg00622.txt.bz2 On Oct 17, 3:58pm, J. Johnston wrote: > The attached ia64 patch fixes a few problems, most notably > backtracing through signal handlers. I think these changes are mostly okay, but... In your ChangeLog entry, you say: > * ia64-tdep.c: Change all references of DEPRECATED_REGISTER_RAW_SIZE > to use register_size() instead. Yet, later on, in the patch, I see that you're reintroducing a use of DEPRECATED_REGISTER_RAW_SIZE: + else if (regnum == IA64_BR0_REGNUM) + { + CORE_ADDR br0 = 0; + CORE_ADDR addr = cache->saved_regs[IA64_BR0_REGNUM]; + if (addr != 0) + { + *lvalp = lval_memory; + *addrp = addr; + read_memory (addr, buf, DEPRECATED_REGISTER_RAW_SIZE (IA64_BR0_REGNUM)); + br0 = extract_unsigned_integer (buf, 8); + } + store_unsigned_integer (valuep, 8, br0); + } Also, regarding: /* We want to calculate the previous bsp as the end of the previous register stack frame. This corresponds to what the hardware bsp register will be if we pop the frame back which is why we might have been called. We know the beginning of the current - frame is cache->bsp - cache->sof. This value in the previous frame points to + frame is cache->bsp - cache->sof. This value in the previous frame points to the start of the output registers. We can calculate the end of that frame by adding the size of output (sof (size of frame) - sol (size of locals)). */ ia64_frame_prev_register (next_frame, this_cache, IA64_CFM_REGNUM, &cfm_optim, &cfm_lval, &cfm_addr, &cfm_realnum, cfm_valuep); prev_cfm = extract_unsigned_integer (cfm_valuep, 8); - + bsp = rse_address_add (cache->bsp, -(cache->sof)); prev_bsp = rse_address_add (bsp, (prev_cfm & 0x7f) - ((prev_cfm >> 7) & 0x7f)); - + The first white space change is okay, but the latter two are not. I'd really prefer to see whitespace changes occur via a separate patch anyway. So, if you don't mind, could you please submit separate patches for: 1) whitespace changes 2) DEPRECATED_... elimination 3) the CFM / backtracing changes. Patches (1) and (2) are preapproved -- just make sure that you don't introduce extra white space on an otherwise blank line. For (1) and (2), please post the patches that you end up committing. Once (1) and (2) are split out, I'd like another chance to review what's left (patch 3). With regard to (3), one of the questions that I already have concerns the following line in ia64_sigtramp_frame_init_saved_regs(): + CORE_ADDR sp = cache->base + 16; Could you explain what this is about? (Preferably with a comment in the code?) Thanks, Kevin P.S. If you'd prefer to reverse things and submit patch 3 first, that'd be okay too.