From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27760 invoked by alias); 28 Dec 2010 09:52:27 -0000 Received: (qmail 27751 invoked by uid 22791); 28 Dec 2010 09:52:26 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 28 Dec 2010 09:52:21 +0000 Received: (qmail 11601 invoked from network); 28 Dec 2010 09:52:19 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 28 Dec 2010 09:52:19 -0000 From: Pedro Alves To: Hui Zhu Subject: Re: [RFA/RFC] mips tracepoint: fix Bug 12013 Date: Tue, 28 Dec 2010 10:30:00 -0000 User-Agent: KMail/1.13.5 (Linux/2.6.33-29-realtime; KDE/4.4.5; x86_64; ; ) Cc: gdb-patches@sourceware.org, Kevin Buettner References: <201012271309.59475.pedro@codesourcery.com> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201012280952.17337.pedro@codesourcery.com> X-IsSubscribed: yes 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 X-SW-Source: 2010-12/txt/msg00500.txt.bz2 On Tuesday 28 December 2010 08:01:47, Hui Zhu wrote: > >> +static int > >> +mips_ax_pseudo_reg (struct gdbarch *gdbarch, struct agent_expr *ax, int reg) > >> +{ > >> + int rawnum = reg % gdbarch_num_regs (gdbarch); > >> + gdb_assert (reg >= gdbarch_num_regs (gdbarch) > >> + && reg < 2 * gdbarch_num_regs (gdbarch)); > >> + if (register_size (gdbarch, rawnum) >= register_size (gdbarch, reg)) > >> + { > >> + ax_reg (ax, rawnum); > >> + > >> + if (register_size (gdbarch, rawnum) > register_size (gdbarch, reg)) > >> + { > >> + if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p > >> + && gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG) > >> + { > >> + ax->buf[ax->len] = aop_const8; > >> + ax->buf[ax->len + 1] = 32; > >> + ax->buf[ax->len + 2] = aop_rsh_unsigned; > >> + ax->len += 3; > >> + } > >> + else > >> + { > >> + ax->buf[ax->len] = aop_const32; > >> + ax->buf[ax->len + 1] = 0xff; > >> + ax->buf[ax->len + 2] = 0xff; > >> + ax->buf[ax->len + 3] = 0xff; > >> + ax->buf[ax->len + 4] = 0xff; > >> + ax->buf[ax->len + 5] = aop_bit_and; > >> + ax->len += 6; > > > > Hmm, I'm not sure, but don't you need to sign extend? > > mips-tdep.c:mips_pseudo_register_read treats this as signed. > > > > Why do you apply the "and 0xffffffff" logic when > > mips64_transfers_32bit_regs_p is false? > > I change this part to: > if (register_size (gdbarch, rawnum) > register_size (gdbarch, reg)) > { > if (!gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p > || gdbarch_byte_order (gdbarch) != BFD_ENDIAN_BIG) > { > ax_const_l (ax, 32); > ax_simple (ax, aop_lsh); > } > ax_const_l (ax, 32); > ax_simple (ax, aop_rsh_signed); > } > > If mips64_transfers_32bit_regs_p is false or arch is little endian, > use the low 32bit. > If not, use the the high 32bit. > > Do you think it is OK? I'm not sure I understand the double shift logic, but I also don't care enough to spend the time to understand it either. :-) You're now using the right interfaces, and if you've tested this (e.g., try collecting an expression that involves adding and subtracting values of registers, and see if the result is sane, and/or if your agent supports it, cooking up a small test that involves a conditional tracepoint involving arithmetic with register values, all this with a 32-bit app running on a 64-bit board), then it's fine with me. I also have to say that I'm also not sure what does mips64_transfers_32bit_regs_p being true mean WRT the agent's register size (as opposed to the remote protocol register size), but, I understand that's a legacy mode, so, we can adjust if turns out we need to do something else. > 2010-12-28 Hui Zhu > > * gdbarch.sh (ax_pseudo_register_collect, > ax_pseudo_register_push_stack): new callbacks. > (agent_expr): New struct. Not really new. Use something like "Forward declare." instead. > * gdbarch.c (gdbarch): Add ax_pseudo_register_collect and > ax_pseudo_register_push_stack. > (startup_gdbarch): Add 0 for ax_pseudo_register_collect and > ax_pseudo_register_push_stack. > (verify_gdbarch): Add comments for ax_pseudo_register_collect and > ax_pseudo_register_push_stack. > (gdbarch_dump): Add fprintf_unfiltered for ax_pseudo_register_collect > and ax_pseudo_register_push_stack. > (gdbarch_ax_pseudo_register_collect_p, > gdbarch_ax_pseudo_register_collect, > set_gdbarch_ax_pseudo_register_collect, > gdbarch_ax_pseudo_register_push_stack_p, > gdbarch_ax_pseudo_register_push_stack, > set_gdbarch_ax_pseudo_register_push_stack): New functions. > * gdbarch.h (agent_expr): New struct. > (gdbarch_ax_pseudo_register_collect_p, > gdbarch_ax_pseudo_register_collect, > set_gdbarch_ax_pseudo_register_collect, > gdbarch_ax_pseudo_register_push_stack_p, > gdbarch_ax_pseudo_register_push_stack, > set_gdbarch_ax_pseudo_register_push_stack): New externs. > (gdbarch_ax_pseudo_register_collect_ftype, > gdbarch_ax_pseudo_register_push_stack_ftype): New typedeies. That's not what I meant with > > You need to mention that gdbarch.c and gdbarch.h were regenerated. Write this, literaly: * gdbarch.h, gdbarch.c: Regenerate. That's all. > * ax-gdb.c (gen_expr): Remove pseudo-register check code. > * ax-general.c (user-regs.h): New include. > (ax_reg): Call gdbarch_ax_pseudo_register_push_stack. > (ax_reg_mask): Call gdbarch_ax_pseudo_register_collect. > * mips-tdep.c (ax.h): New include. > (mips_ax_pseudo_register_collect, > mips_ax_pseudo_register_push_stack): New functions. > (mips_gdbarch_init): Set mips_ax_pseudo_register_collect and > mips_ax_pseudo_register_push_stack. Otherwise okay. Thanks! -- Pedro Alves