From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16898 invoked by alias); 28 Dec 2010 16:17:45 -0000 Received: (qmail 16881 invoked by uid 22791); 28 Dec 2010 16:17:42 -0000 X-SWARE-Spam-Status: No, hits=-2.2 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,RFC_ABUSE_POST X-Spam-Check-By: sourceware.org Received: from mail-ww0-f43.google.com (HELO mail-ww0-f43.google.com) (74.125.82.43) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 28 Dec 2010 16:17:02 +0000 Received: by wwi17 with SMTP id 17so9185401wwi.12 for ; Tue, 28 Dec 2010 08:16:59 -0800 (PST) Received: by 10.216.205.213 with SMTP id j63mr17128951weo.60.1293553019472; Tue, 28 Dec 2010 08:16:59 -0800 (PST) MIME-Version: 1.0 Received: by 10.216.182.206 with HTTP; Tue, 28 Dec 2010 08:16:39 -0800 (PST) In-Reply-To: <201012280952.17337.pedro@codesourcery.com> References: <201012271309.59475.pedro@codesourcery.com> <201012280952.17337.pedro@codesourcery.com> From: Hui Zhu Date: Tue, 28 Dec 2010 17:09:00 -0000 Message-ID: Subject: Re: [RFA/RFC] mips tracepoint: fix Bug 12013 To: Pedro Alves , Joel Brobecker Cc: gdb-patches@sourceware.org, Kevin Buettner Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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/msg00515.txt.bz2 Thanks Pedro. Fixed and checked in. Joel, do you think this patch can check in to 7.2.1. Thanks, Hui On Tue, Dec 28, 2010 at 17:52, Pedro Alves wrote: > 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) >> >> +{ >> >> + =A0int rawnum =3D reg % gdbarch_num_regs (gdbarch); >> >> + =A0gdb_assert (reg >=3D gdbarch_num_regs (gdbarch) >> >> + =A0 =A0 =A0 =A0 =A0 && reg < 2 * gdbarch_num_regs (gdbarch)); >> >> + =A0if (register_size (gdbarch, rawnum) >=3D register_size (gdbarch,= reg)) >> >> + =A0 =A0{ >> >> + =A0 =A0 =A0ax_reg (ax, rawnum); >> >> + >> >> + =A0 =A0 =A0if (register_size (gdbarch, rawnum) > register_size (gdb= arch, reg)) >> >> + =A0 =A0 =A0 =A0{ >> >> + =A0 =A0 =A0 if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs= _p >> >> + =A0 =A0 =A0 =A0 =A0 && gdbarch_byte_order (gdbarch) =3D=3D BFD_ENDI= AN_BIG) >> >> + =A0 =A0 =A0 =A0 { >> >> + =A0 =A0 =A0 =A0 =A0 ax->buf[ax->len] =3D aop_const8; >> >> + =A0 =A0 =A0 =A0 =A0 ax->buf[ax->len + 1] =3D 32; >> >> + =A0 =A0 =A0 =A0 =A0 ax->buf[ax->len + 2] =3D aop_rsh_unsigned; >> >> + =A0 =A0 =A0 =A0 =A0 ax->len +=3D 3; >> >> + =A0 =A0 =A0 =A0 } >> >> + =A0 =A0 =A0 else >> >> + =A0 =A0 =A0 =A0 { >> >> + =A0 =A0 =A0 =A0 =A0 ax->buf[ax->len] =3D aop_const32; >> >> + =A0 =A0 =A0 =A0 =A0 ax->buf[ax->len + 1] =3D 0xff; >> >> + =A0 =A0 =A0 =A0 =A0 ax->buf[ax->len + 2] =3D 0xff; >> >> + =A0 =A0 =A0 =A0 =A0 ax->buf[ax->len + 3] =3D 0xff; >> >> + =A0 =A0 =A0 =A0 =A0 ax->buf[ax->len + 4] =3D 0xff; >> >> + =A0 =A0 =A0 =A0 =A0 ax->buf[ax->len + 5] =3D aop_bit_and; >> >> + =A0 =A0 =A0 =A0 =A0 ax->len +=3D 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: >> =A0 =A0 =A0 if (register_size (gdbarch, rawnum) > register_size (gdbarch= , reg)) >> =A0 =A0 =A0 =A0 { >> =A0 =A0 =A0 =A0 if (!gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs= _p >> =A0 =A0 =A0 =A0 =A0 =A0 || gdbarch_byte_order (gdbarch) !=3D BFD_ENDIAN_= BIG) >> =A0 =A0 =A0 =A0 =A0 { >> =A0 =A0 =A0 =A0 =A0 =A0 ax_const_l (ax, 32); >> =A0 =A0 =A0 =A0 =A0 =A0 ax_simple (ax, aop_lsh); >> =A0 =A0 =A0 =A0 =A0 } >> =A0 =A0 =A0 =A0 ax_const_l (ax, 32); >> =A0 =A0 =A0 =A0 ax_simple (ax, aop_rsh_signed); >> =A0 =A0 =A0 } >> >> 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. =A0:-) =A0You're n= ow > 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 someth= ing > else. > > >> 2010-12-28 =A0Hui Zhu =A0 >> >> =A0 =A0 =A0 * gdbarch.sh (ax_pseudo_register_collect, >> =A0 =A0 =A0 ax_pseudo_register_push_stack): new callbacks. >> =A0 =A0 =A0 (agent_expr): New struct. > > Not really new. =A0Use something like "Forward declare." instead. > >> =A0 =A0 =A0 * gdbarch.c (gdbarch): Add ax_pseudo_register_collect and >> =A0 =A0 =A0 ax_pseudo_register_push_stack. >> =A0 =A0 =A0 (startup_gdbarch): Add 0 for ax_pseudo_register_collect and >> =A0 =A0 =A0 ax_pseudo_register_push_stack. >> =A0 =A0 =A0 (verify_gdbarch): Add comments for ax_pseudo_register_collec= t and >> =A0 =A0 =A0 ax_pseudo_register_push_stack. >> =A0 =A0 =A0 (gdbarch_dump): Add fprintf_unfiltered for ax_pseudo_registe= r_collect >> =A0 =A0 =A0 and ax_pseudo_register_push_stack. >> =A0 =A0 =A0 (gdbarch_ax_pseudo_register_collect_p, >> =A0 =A0 =A0 gdbarch_ax_pseudo_register_collect, >> =A0 =A0 =A0 set_gdbarch_ax_pseudo_register_collect, >> =A0 =A0 =A0 gdbarch_ax_pseudo_register_push_stack_p, >> =A0 =A0 =A0 gdbarch_ax_pseudo_register_push_stack, >> =A0 =A0 =A0 set_gdbarch_ax_pseudo_register_push_stack): New functions. >> =A0 =A0 =A0 * gdbarch.h (agent_expr): New struct. >> =A0 =A0 =A0 (gdbarch_ax_pseudo_register_collect_p, >> =A0 =A0 =A0 gdbarch_ax_pseudo_register_collect, >> =A0 =A0 =A0 set_gdbarch_ax_pseudo_register_collect, >> =A0 =A0 =A0 gdbarch_ax_pseudo_register_push_stack_p, >> =A0 =A0 =A0 gdbarch_ax_pseudo_register_push_stack, >> =A0 =A0 =A0 set_gdbarch_ax_pseudo_register_push_stack): New externs. >> =A0 =A0 =A0 (gdbarch_ax_pseudo_register_collect_ftype, >> =A0 =A0 =A0 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: > > =A0 =A0 =A0 =A0* gdbarch.h, gdbarch.c: Regenerate. > > That's all. > >> =A0 =A0 =A0 * ax-gdb.c (gen_expr): Remove pseudo-register check code. >> =A0 =A0 =A0 * ax-general.c (user-regs.h): New include. >> =A0 =A0 =A0 (ax_reg): Call gdbarch_ax_pseudo_register_push_stack. >> =A0 =A0 =A0 (ax_reg_mask): Call gdbarch_ax_pseudo_register_collect. >> =A0 =A0 =A0 * mips-tdep.c (ax.h): New include. >> =A0 =A0 =A0 (mips_ax_pseudo_register_collect, >> =A0 =A0 =A0 mips_ax_pseudo_register_push_stack): New functions. >> =A0 =A0 =A0 (mips_gdbarch_init): Set mips_ax_pseudo_register_collect and >> =A0 =A0 =A0 mips_ax_pseudo_register_push_stack. > > Otherwise okay. =A0Thanks! > > -- > Pedro Alves >