From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 60644 invoked by alias); 10 Sep 2018 06:32:51 -0000 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 Received: (qmail 60634 invoked by uid 89); 10 Sep 2018 06:32:51 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-12.6 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=2018-09-07, 20180827, Supply, Hx-spam-relays-external:sk:2018091 X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0b-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.158.5) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 10 Sep 2018 06:32:47 +0000 Received: from pps.filterd (m0098421.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w8A6TBsT010988 for ; Mon, 10 Sep 2018 02:32:45 -0400 Received: from smtp.notes.na.collabserv.com (smtp.notes.na.collabserv.com [192.155.248.75]) by mx0a-001b2d01.pphosted.com with ESMTP id 2mdf1js0fx-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Mon, 10 Sep 2018 02:32:45 -0400 Received: from localhost by smtp.notes.na.collabserv.com with smtp.notes.na.collabserv.com ESMTP for from ; Mon, 10 Sep 2018 06:32:44 -0000 Received: from us1a3-smtp07.a3.dal06.isc4sb.com (10.146.103.14) by smtp.notes.na.collabserv.com (10.106.227.123) with smtp.notes.na.collabserv.com ESMTP; Mon, 10 Sep 2018 06:32:40 -0000 Received: from us1a3-mail142.a3.dal06.isc4sb.com ([10.146.38.78]) by us1a3-smtp07.a3.dal06.isc4sb.com with ESMTP id 2018091006324010-112582 ; Mon, 10 Sep 2018 06:32:40 +0000 In-Reply-To: <20180904161827.7049008a@pinnacle.lan> To: Kevin Buettner Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Adding support for reding signal handler frame in AIX From: "Sangamesh Mallayya" Date: Mon, 10 Sep 2018 06:32:00 -0000 References: <20180904161827.7049008a@pinnacle.lan> MIME-Version: 1.0 X-KeepSent: E73EF236:0037C8EA-65258304:00227754; type=4; name=$KeepSent X-LLNOutbound: False X-Disclaimed: 31307 X-TNEFEvaluated: 1 x-cbid: 18091006-3815-0000-0000-00000810148F X-IBM-SpamModules-Scores: BY=0; FL=0; FP=0; FZ=0; HX=0; KW=0; PH=0; SC=0.425523; ST=0; TS=0; UL=0; ISC=; MB=0.122080 X-IBM-SpamModules-Versions: BY=3.00009694; HX=3.00000242; KW=3.00000007; PH=3.00000004; SC=3.00000266; SDB=6.01086150; UDB=6.00560755; IPR=6.00866161; BA=6.00006093; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00023206; XFM=3.00000015; UTC=2018-09-10 06:32:43 X-IBM-AV-DETECTION: SAVI=unsuspicious REMOTE=unsuspicious XFE=unused X-IBM-AV-VERSION: SAVI=2018-09-10 02:08:35 - 6.00008946 x-cbparentid: 18091006-3816-0000-0000-0000BFC41EFE Message-Id: Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: quoted-printable X-Proofpoint-Spam-Reason: safe X-SW-Source: 2018-09/txt/msg00250.txt.bz2 Hi Kevin, Thanks a lot for review and comments. > > + sigconext structure have the mstsave saved under the >=20 > typo, I think - s/sigconext/sigcontext/ >=20 Yes. Will correct this. > > + sc_jmpbuf.jmp_context. STKMIN(minimum stack size) is 56 for 32-bit > > + processes, and iar offset under sc_jmpbuf.jmp_context is 40. > > + ie offsetof(struct sigcontext, sc_jmpbuf.jmp_context.iar). > > + so PC offset in this case is STKMIN+iar offset, which is 96 */ >=20 > Please add a period at the end of this sentence. >=20 Sure. > > + > > #define SIG_FRAME_PC_OFFSET 96 > > #define SIG_FRAME_LR_OFFSET 108 > > +/* STKMIN+grp1 offset, which is 56+228=3D284 */ > > #define SIG_FRAME_FP_OFFSET 284 > >=20 > > +/* 64 bit process > > + STKMIN64 is 112 and iar offset is 312. So 112+312=3D424 */ > > +#define SIG_FRAME_LR_OFFSET64 424=20 > > +/* STKMIN64+grp1 offset. 112+56=3D168 */ > > +#define SIG_FRAME_FP_OFFSET64 168 > >=20 > > /* Core file support. */ > >=20 > > @@ -103,6 +118,104 @@ > > -1 /* vrsave_offset */ > > }; > >=20 > > +static void=20 > > +aix_sigtramp_cache (struct frame_info *this_frame, > > + struct trad_frame_cache *this_cache, > > + CORE_ADDR func, LONGEST offset, > > + int bias) > > +{ > > + LONGEST backchain; > > + CORE_ADDR base, frame_base, base_orig; > > + CORE_ADDR regs; > > + CORE_ADDR gpregs; > > + CORE_ADDR fpregs; > > + int i; > > + struct gdbarch *gdbarch =3D get_frame_arch (this_frame); > > + struct gdbarch_tdep *tdep =3D gdbarch_tdep (gdbarch); > > + int wordsize =3D tdep->wordsize; > > + enum bfd_endian byte_order =3D gdbarch_byte_order (gdbarch); > > + > > + base =3D get_frame_register_unsigned (this_frame, > > + gdbarch_sp_regnum (gdbarch)); > > + base_orig =3D base; > > + if (wordsize =3D=3D 4) { > > + safe_read_memory_integer (base_orig + SIG_FRAME_FP_OFFSET + 8, > > + wordsize, byte_order, &backchain); > > + base =3D (CORE_ADDR)backchain; > > + } else { > > + safe_read_memory_integer (base_orig + SIG_FRAME_FP_OFFSET64, > > + wordsize, byte_order, &backchain); > > + base =3D (CORE_ADDR)backchain; > > + } >=20 > I'm wondering if the cast might be avoided by using > read_memory_typed_address in place of safe_read_memory_integer? > That way you'd be able to assign directly to base and also be > able to eliminate the "backchain" variable. >=20 read_memory_typed_address requires the second parameter of *type. Need to check and confirm how easy to get type, and does it required more=20 function calls than just calling safe_read_memory_integer would be good.=20 > > + > > + trad_frame_set_reg_value (this_cache, gdbarch_pc_regnum (gdbarch),=20 func); > > + trad_frame_set_reg_value (this_cache, gdbarch_sp_regnum (gdbarch),=20 base); > > + > > + if (wordsize =3D=3D 4) { > > + trad_frame_set_reg_addr (this_cache, tdep->ppc_lr_regnum, > > + base_orig + offset + 52 + 8); > > + } else { > > + trad_frame_set_reg_addr (this_cache, tdep->ppc_lr_regnum, > > + base_orig + offset + 320); > > + }=20 > > + trad_frame_set_id (this_cache, frame_id_build (base, func)); > > +} > > + > > +static void > > +aix_sigtramp_cache_init (const struct tramp_frame *self, > > + struct frame_info *this_frame, > > + struct trad_frame_cache *this_cache, > > + CORE_ADDR func) > > +{ > > + struct gdbarch *gdbarch =3D get_frame_arch (this_frame); > > + struct gdbarch_tdep *tdep =3D gdbarch_tdep (gdbarch); > > + > > + if (tdep->wordsize =3D=3D 4) > > + aix_sigtramp_cache (this_frame, this_cache, func, > > + 0x38 /* Minimum stack size */, > > + 0); > > + else > > + aix_sigtramp_cache (this_frame, this_cache, func, > > + 0x70 /* Minimum stack size. */, > > + 0); > > +} > > + > > +static int > > +aix_validate_pc (const struct tramp_frame *self, > > + struct frame_info *this_frame, > > + CORE_ADDR *pc) > > +{ > > + struct gdbarch *gdbarch =3D get_frame_arch (this_frame); > > + struct gdbarch_tdep *tdep =3D gdbarch_tdep (gdbarch); > > + enum bfd_endian byte_order =3D gdbarch_byte_order (gdbarch); > > + CORE_ADDR dest, frame_base; > > + frame_base =3D get_frame_register_unsigned (this_frame, > > + gdbarch_sp_regnum (gdbarch)); > > + if (tdep->wordsize =3D=3D 4) { > > + if (*pc && *pc < AIX_TEXT_SEGMENT_BASE) { > > + *pc =3D read_memory_unsigned_integer > > + (frame_base + SIG_FRAME_PC_OFFSET + 8, > > + tdep->wordsize, byte_order); > > + } > > + } else { > > + if (*pc && *pc < AIX_TEXT_SEGMENT_BASE) { > > + *pc =3D read_memory_unsigned_integer > > + (frame_base + SIG_FRAME_LR_OFFSET64, > > + tdep->wordsize, byte_order); > > + } > > + } > > + return 1; > > +}=20 > > + > > +static struct tramp_frame aix_sighandler_tramp_frame =3D { > > + SIGTRAMP_FRAME, > > + 4, > > + { > > + { TRAMP_SENTINEL_INSN }, > > + }, > > + aix_sigtramp_cache_init, > > + aix_validate_pc > > +}; > >=20 > > /* Supply register REGNUM in the general-purpose register set REGSET > > from the buffer specified by GREGS and LEN to register cache > > @@ -1083,6 +1196,7 @@ > > set_gdbarch_auto_wide_charset (gdbarch,=20 rs6000_aix_auto_wide_charset); > >=20 > > set_solib_ops (gdbarch, &solib_aix_so_ops); > > + tramp_frame_prepend_unwinder (gdbarch,=20 &aix_sighandler_tramp_frame); > > } > >=20 > > void > > --- ./gdb/tramp-frame.c_orig 2018-08-27 03:25:49 +0000 > > +++ ./gdb/tramp-frame.c 2018-08-27 03:26:24 +0000 > > @@ -132,6 +132,12 @@ > > false on HPUX which has a signal trampoline that has a name; it=20 can > > also be false when using an alternative signal stack. */ > > func =3D tramp_frame_start (tramp, this_frame, pc); > > + #if defined (_AIX) > > + if (pc <=3D 0x10000000) { > > + tramp->validate (tramp, this_frame, &pc); > > + func =3D pc; > > + } > > + #endif >=20 > We don't want to be putting OS specific ifdefs here. It seems to me > that the pc <=3D 0x10000000 test could be put in the validate code if in > fact it's needed at all. The return value of that call to validate is > not being checked, so that means that you're calling it to obtain > func. But func should be correctly set by the call to > tramp_frame_start, earlier on. Note, too, that tramp_frame_start > calls the validate method, so it seems to me that it ought to be > possible to get it set as needed by some suitable definition of > validate. >=20 Yes. Thanks! Earlier code was calling validate function twice which wasn't required. We can remove that AIX ifdef and i have made the below changes. Rest all=20 are same. Let me know your view on this. # diff -u tramp-frame.c_orig tramp-frame.c --- tramp-frame.c_orig 2018-08-27 03:25:49 +0000 +++ tramp-frame.c 2018-09-07 10:20:09 +0000 @@ -86,11 +86,15 @@ struct gdbarch *gdbarch =3D get_frame_arch (this_frame); enum bfd_endian byte_order =3D gdbarch_byte_order (gdbarch); int ti; + CORE_ADDR old_pc =3D pc; =20 /* Check if we can use this trampoline. */ if (tramp->validate && !tramp->validate (tramp, this_frame, &pc)) return 0; - + if ((tramp->insn[0].bytes =3D=3D TRAMP_SENTINEL_INSN) && + (tramp->insn[1].bytes =3D=3D AIX_TRAMP_SENTINEL_INSN) && + (old_pc < 0x1000000)) + return pc; --- tramp-frame.h_orig 2018-09-07 10:03:34 +0000 +++ tramp-frame.h 2018-09-07 10:18:50 +0000 @@ -42,6 +42,7 @@ /* Magic instruction that to mark the end of the signal trampoline instruction sequence. */ #define TRAMP_SENTINEL_INSN ((LONGEST) -1) +#define AIX_TRAMP_SENTINEL_INSN ((LONGEST) -2) <=3D=3D=3D=3D=3D=3D New def= inition to=20 make sure this check is done only when running it on AIX. =20 static struct tramp_frame aix_sighandler_tramp_frame =3D { SIGTRAMP_FRAME, 4, { { TRAMP_SENTINEL_INSN, -1},=20 { AIX_TRAMP_SENTINEL_INSN, -2}, <=3D=3D=3D=3D=3D=3D New definition = to make=20 sure this check is done only when running it on AIX. }, aix_sigtramp_cache_init, aix_validate_pc };