From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29187 invoked by alias); 9 Jan 2013 16:28:50 -0000 Received: (qmail 29177 invoked by uid 22791); 9 Jan 2013 16:28:48 -0000 X-SWARE-Spam-Status: No, hits=-2.6 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,TW_QW X-Spam-Check-By: sourceware.org Received: from mel.act-europe.fr (HELO mel.act-europe.fr) (194.98.77.210) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 09 Jan 2013 16:28:34 +0000 Received: from localhost (localhost [127.0.0.1]) by filtered-smtp.eu.adacore.com (Postfix) with ESMTP id B3056290024; Wed, 9 Jan 2013 17:28:41 +0100 (CET) Received: from mel.act-europe.fr ([127.0.0.1]) by localhost (smtp.eu.adacore.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id LmMmAYfUjOU0; Wed, 9 Jan 2013 17:28:41 +0100 (CET) Received: from ulanbator.act-europe.fr (ulanbator.act-europe.fr [10.10.1.67]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by mel.act-europe.fr (Postfix) with ESMTP id 9A084290023; Wed, 9 Jan 2013 17:28:41 +0100 (CET) Subject: Re: [RFA/commit+doco 2/2] Windows x64 SEH unwinder. Mime-Version: 1.0 (Apple Message framework v1283) Content-Type: text/plain; charset=iso-8859-1 From: Tristan Gingold In-Reply-To: <50ED9221.1050504@redhat.com> Date: Wed, 09 Jan 2013 16:28:00 -0000 Cc: Joel Brobecker , gdb-patches@sourceware.org Content-Transfer-Encoding: quoted-printable Message-Id: <9E84DF2D-7AF8-4AA1-A5DF-171EF189A6E7@adacore.com> References: <1357728781-15073-1-git-send-email-brobecker@adacore.com> <1357728781-15073-3-git-send-email-brobecker@adacore.com> <50ED9221.1050504@redhat.com> To: Pedro Alves 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: 2013-01/txt/msg00168.txt.bz2 Partial answer: On Jan 9, 2013, at 4:52 PM, Pedro Alves wrote: > On 01/09/2013 10:53 AM, Joel Brobecker wrote: >=20 >> This is the main part of the patch series, which actually >> adds the unwinder. >>=20 >> One element worth mentioning, perhaps, is the fact that we prepend >> the unwinder, and the sniffer is the default_frame_sniffer which >> always returns 1 (absence of SEH info is normal and means it is >> a leaf function). >=20 > Meaning you want to use this unwinder for leaf functions > too, right? Yes, and we need that. Unwinding lead functions is defined and handled by this unwinder. >> This effectively means that all other unwinders >> effectively get shunted. And in particular, I do not think that >> the DWARF unwinder will kick in even if DWARF unwind info is found. >>=20 >> The problem is that we want to be ahead of the default amd64-tdep >> unwinders, which is kind of a last-resort unwinder doing a good >> job under limited situations only. We'd like to be behind the DWARF >> unwinder if we could, but I don't think there is a way to ensure >> that yet. >>=20 >> In practice, this shouldn't be a problem, since this unwinder >> has been very reliable for us so far. But it does assume that >> the compiler is recent enough to generate native SEH data which, >> for GCC, means GCC 4.7 (I have been told). On the other hand, >> it looks like the first GCC release to support x64-windows was >> GCC 4.6. >>=20 >> I don't really see a real way of supporting both old and new versions >> of GCC, unless we have a way of more finely ordering the unwinders. >=20 > What specific finer order where you considering would be needed to > fix this? Joel once proposed to activate this unwinder if the CU is compiled by gcc 4.6 or older. >> Worse case scenario, we stop supporting code generated by GCC 4.6. >=20 > Yuck. >=20 >> Or an alternative option is to provide a setting to disable this >> unwinder. >=20 > That'd be my worse case scenario. :-) >=20 > Maybe detect if the whole module (exe/dll) the PC points at > contains any SEH (even if not for PC), and skip the SEH unwinder > if not? Is that possible? Yes, but useless. There are SEH info in crt0. >> +struct amd64_windows_frame_cache >> +{ >> + /* ImageBase for the module. */ >> + CORE_ADDR image_base; >> + >> + /* Function start rva. */ >> + CORE_ADDR start_rva; >> + >> + /* Next instruction to be executed. */ >> + CORE_ADDR pc; >> + >> + /* Current sp. */ >> + CORE_ADDR sp; >> + >> + /* Address of saved integer and xmm registers. */ >> + CORE_ADDR prev_reg_addr[16]; >> + CORE_ADDR prev_xmm_addr[16]; >> + >> + /* These two next fields are set only for machine info frames. */ >> + >> + /* Likewise for RIP. */ >=20 > "next two fields", and then immediately "likewise" > makes be believe there used to be two fields here that > have since been removed? You're correct. That concerns only prev_rsp_addr. >> + CORE_ADDR prev_rip_addr; >> + >> + /* Likewise for RSP. */ >> + CORE_ADDR prev_rsp_addr; >> + >> + /* Address of the previous frame. */ >> + CORE_ADDR prev_sp; >> +}; >> + >=20 >=20 >=20 >> +/* Try to recognize and decode an epilogue sequence. >> + >> + Return -1 if we fail to read the instructions for any reason. >> + Return 1 if an epilogue sequence was recognized, 0 otherwise. */ >> + >> +static int >> +amd64_windows_frame_decode_epilogue (struct frame_info *this_frame, >> + struct amd64_windows_frame_cache *cache) >> +{ >> + /* Not in a prologue, so maybe in an epilogue. For the rules, cf >=20 > OOW, what does "cf" stand for? Confere. >> + http://msdn.microsoft.com/en-us/library/tawsa7cb.aspx >> + Furthermore, according to RtlVirtualUnwind, the complete list of >> + epilog marker is: >> + - ret [c3] >> + - ret n [c2 imm16] >> + - rep ret [f3 c3] >> + - jmp imm8 | imm32 [eb rel8] or [e9 rel32] >> + - jmp qword ptr imm32 - not handled >> + - rex jmp reg [4X ff eY] >> + I would add: >> + - pop reg [41 58-5f] or [58-5f] >=20 > If you add "pop", and you'd add "add" and "lea" as well, > right? It's not super clear what "marker" means, but all > the instructions listed by RtlVirtualUnwind's docs are > control flow instructions. And then, in the url you point > above, we see: >=20 > "These are the only legal forms for an epilog. It must consist of either > an add RSP,constant or lea RSP,constant[FPReg], followed by a series > of zero or more 8-byte register pops and a return or a jmp. (Only > a subset of jmp statements are allowable in the epilog." >=20 > So from both docs it seems to me that "marker" is always the > last instruction of the epilogue, and that "pop" is not called > a marker. This is in fact an optimization. If we found a pop, followed by an epilog marker, there is not need to decode unwind info. > Furthermore, >=20 >> + >> + We don't care about the instruction deallocating the frame: >> + if it hasn't been executed, we can safely decode the insns; >> + if it has been executed, the following epilog decoding will >> + work. */ >> + CORE_ADDR pc =3D cache->pc; >> + CORE_ADDR cur_sp =3D cache->sp; >> + struct gdbarch *gdbarch =3D get_frame_arch (this_frame); >> + enum bfd_endian byte_order =3D gdbarch_byte_order (gdbarch); >> + >> + while (1) >> + { >> + gdb_byte op; >> + gdb_byte rex; >> + >> + if (target_read_memory (pc, &op, 1) !=3D 0) >> + return -1; >> + >> + if (op =3D=3D 0xc3) >> + { >> + /* Ret. */ >> + cache->prev_rip_addr =3D cur_sp; >> + cache->prev_sp =3D cur_sp + 8; >> + return 1; >> + } >> + else if (op =3D=3D 0xeb) >> + { >> + /* jmp rel8 */ >> + gdb_byte rel8; >> + >> + if (target_read_memory (pc + 1, &rel8, 1) !=3D 0) >> + return -1; >> + pc =3D pc + 2 + (signed char) rel8; >=20 > this implementation follows jumps, and keeps looping > at the jump destination. Right. > I see no hint that such thing > as an epilogue with a jump is a valid epilogue, only that > a jmp is only valid as replacement for ret. > Can you show an example where following the jmp until > you see a ret is necessary? Interesting remark. First attempt to answer is the case of a jump to an epilogue. But I may miss your point. >> +/* Decode and execute unwind insns at UNWIND_INFO. */ >> + >> +static void >> +amd64_windows_frame_decode_insns (struct frame_info *this_frame, >> + struct amd64_windows_frame_cache *cache, >> + CORE_ADDR unwind_info) >> +{ > ... >=20 >> + if (unwind_debug) >> + fprintf_unfiltered >> + (gdb_stdlog, >> + "amd64_windows_frame_play_insn: " >=20 > Is this supposed to be a function name? If so, it's already > out of sync. Correct. >> + "%s: ver: %02x, plgsz: %02x, cnt: %02x, frame: %02x\n", >> + paddress (gdbarch, unwind_info), >> + ex_ui.Version_Flags, ex_ui.SizeOfPrologue, >> + ex_ui.CountOfCodes, ex_ui.FrameRegisterOffset); >=20 >=20 >=20 >> + switch (PEX64_UNWCODE_CODE (p[1])) >> + { >> + case UWOP_PUSH_NONVOL: >> + /* Push pre-decrements RSP. */ >> + reg =3D amd64_windows_w2gdb_regnum[PEX64_UNWCODE_INFO (p[1])]; >> + cache->prev_reg_addr[reg] =3D cur_sp; >> + cur_sp +=3D 8; >> + break; >> + case UWOP_ALLOC_LARGE: >> + if (PEX64_UNWCODE_INFO (p[1]) =3D=3D 0) >> + cur_sp +=3D >=20 > Operator goes on next line (multiple instances in the patch, > including with '=3D'). >=20 >> + 8 * extract_unsigned_integer (p + 2, 2, byte_order); >> + else if (PEX64_UNWCODE_INFO (p[1]) =3D=3D 1) >> + cur_sp +=3D extract_unsigned_integer (p + 2, 4, byte_order); >> + else >> + return; >> + break; >=20 >> + >> + if (unwind_debug) >> + fprintf_unfiltered >> + (gdb_stdlog, >> + "amd64_windows_find_unwind_data: image_base=3D%s, unwind_data= =3D%s\n", >> + paddress (gdbarch, base), paddress (gdbarch, *unwind_info)); >=20 > Another stale function name. >=20 >> + >> + if (*unwind_info & 1) >> + { >> + /* Unofficially documented unwind info redirection, when UNWIND_I= NFO >> + address is odd. */ >=20 > What's "unofficially documented"? Documented in some > blog? Yes, or some papers not coming from MS. >=20 >> + struct external_pex64_runtime_function d; >> + CORE_ADDR sa, ea; >> + >> + if (target_read_memory (base + (*unwind_info & ~1), >> + (gdb_byte *) &d, sizeof (d)) !=3D 0) >> + return -1; >> + >> + *start_rva =3D >> + extract_unsigned_integer (d.rva_BeginAddress, 4, byte_order); >> + *unwind_info =3D >> + extract_unsigned_integer (d.rva_EndAddress, 4, byte_order); >> + } >> + return 0; >> +} >> + >> +/* Fill THIS_CACHE using the native amd64-windows unwinding data >> + for THIS_FRAME. */ >> + >> +static struct amd64_windows_frame_cache * >> +amd64_windows_frame_cache (struct frame_info *this_frame, void **this_c= ache) >> +{ >> + struct gdbarch *gdbarch =3D get_frame_arch (this_frame); >> + enum bfd_endian byte_order =3D gdbarch_byte_order (gdbarch); >> + struct amd64_windows_frame_cache *cache; >> + char buf[8]; >=20 > gdb_byte. >=20 >=20 >> +static struct value * >> +amd64_windows_frame_prev_register (struct frame_info *this_frame, >> + void **this_cache, int regnum) >> +{ >> + struct gdbarch *gdbarch =3D get_frame_arch (this_frame); >> + enum bfd_endian byte_order =3D gdbarch_byte_order (gdbarch); >> + struct amd64_windows_frame_cache *cache =3D >> + amd64_windows_frame_cache (this_frame, this_cache); >> + struct value *val; >> + CORE_ADDR prev; >> + >> + if (unwind_debug) >> + fprintf_unfiltered (gdb_stdlog, >> + "amd64_windows_frame_prev_register %s for sp=3D%s\n", >> + gdbarch_register_name (gdbarch, regnum), >> + paddress (gdbarch, cache->prev_sp)); >> + >> + if (regnum >=3D AMD64_XMM0_REGNUM && regnum <=3D AMD64_XMM0_REGNUM + = 15) >> + prev =3D cache->prev_xmm_addr[regnum - AMD64_XMM0_REGNUM]; >=20 > Indentation looks odd. >=20 >> + else if (regnum =3D=3D AMD64_RSP_REGNUM) >> + { >> + prev =3D cache->prev_rsp_addr; >> + if (prev =3D=3D 0) >> + return frame_unwind_got_constant (this_frame, regnum, cache->prev_sp); >> + } >> + else if (regnum >=3D AMD64_RAX_REGNUM && regnum <=3D AMD64_R15_REGNUM) >> + prev =3D cache->prev_reg_addr[regnum - AMD64_RAX_REGNUM]; >> + else if (regnum =3D=3D AMD64_RIP_REGNUM) >> + prev =3D cache->prev_rip_addr; >> + else >> + prev =3D 0; >> + >> + if (prev && unwind_debug) >> + fprintf_unfiltered (gdb_stdlog, " -> at %s\n", paddress (gdbarch, = prev)); >> + >> + if (prev) >=20 > ('prev' is not really a boolean. I'd prefer 'prev !=3D 0' > in these cases.) >=20 >=20 >> +/* Implement the "skip_prologue" gdbarch method. */ >> + >> +static CORE_ADDR >> +amd64_windows_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc) >> +{ >> + CORE_ADDR func_addr; >> + CORE_ADDR unwind_info =3D 0; >> + CORE_ADDR image_base, start_rva; >> + struct external_pex64_unwind_info ex_ui; >=20 > ex_ui could move to the inner scope. >=20 >> + >> + /* Use prologue size from unwind info. */ >> + if (amd64_windows_find_unwind_info (gdbarch, pc, &unwind_info, >> + &image_base, &start_rva) =3D=3D 0) >> + { >> + if (unwind_info =3D=3D 0) >> + { >> + /* Leaf function. */ >> + return pc; >> + } >> + else if (target_read_memory (image_base + unwind_info, >> + (gdb_byte *) &ex_ui, sizeof (ex_ui)) =3D=3D 0 >> + && PEX64_UWI_VERSION (ex_ui.Version_Flags) =3D=3D 1) >> + return max (pc, image_base + start_rva + ex_ui.SizeOfPrologue); >> + } >> + >> + /* See if we can determine the end of the prologue via the symbol >> + table. If so, then return either the PC, or the PC after >> + the prologue, whichever is greater. */ >> + if (find_pc_partial_function (pc, NULL, &func_addr, NULL)) >> + { >> + CORE_ADDR post_prologue_pc >> + =3D skip_prologue_using_sal (gdbarch, func_addr); >> + >> + if (post_prologue_pc !=3D 0) >> + return max (pc, post_prologue_pc); >> + } >> + >> + return pc; >> +} >> + >> static void >> amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarc= h) >> { >> @@ -225,6 +910,9 @@ amd64_windows_init_abi (struct gdbarch_info info, st= ruct gdbarch *gdbarch) >>=20 >> set_gdbarch_auto_wide_charset (gdbarch, amd64_windows_auto_wide_charse= t); >>=20 >> + frame_unwind_prepend_unwinder (gdbarch, &amd64_windows_frame_unwind); >=20 > I think a comment here referring to issues you > mentioned in the mail would be good. >=20 >> + set_gdbarch_skip_prologue (gdbarch, amd64_windows_skip_prologue); >> + >> set_solib_ops (gdbarch, &solib_target_so_ops); >> } >=20 > --=20 > Pedro Alves Tristan.