Re: [RFA/commit+doco 2/2] Windows x64 SEH unwinder.

[Tristan Gingold <gingold at adacore dot com>]

Partial answer:

On Jan 9, 2013, at 4:52 PM, Pedro Alves wrote:

> On 01/09/2013 10:53 AM, Joel Brobecker wrote:
> 
>> This is the main part of the patch series, which actually
>> adds the unwinder.
>> 
>> 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).
> 
> 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.
>> 
>> 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.
>> 
>> 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.
>> 
>> 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.
> 
> 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.
> 
> Yuck.
> 
>> Or an alternative option is to provide a setting to disable this
>> unwinder.
> 
> That'd be my worse case scenario.  :-)
> 
> 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.  */
> 
> "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;
>> +};
>> +
> 
> 
> 
>> +/* 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
> 
> 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]
> 
> 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:
> 
> "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."
> 
> 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,
> 
>> +
>> +     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 = cache->pc;
>> +  CORE_ADDR cur_sp = cache->sp;
>> +  struct gdbarch *gdbarch = get_frame_arch (this_frame);
>> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> +
>> +  while (1)
>> +    {
>> +      gdb_byte op;
>> +      gdb_byte rex;
>> +
>> +      if (target_read_memory (pc, &op, 1) != 0)
>> +	return -1;
>> +
>> +      if (op == 0xc3)
>> +	{
>> +	  /* Ret.  */
>> +	  cache->prev_rip_addr = cur_sp;
>> +	  cache->prev_sp = cur_sp + 8;
>> +	  return 1;
>> +	}
>> +      else if (op == 0xeb)
>> +	{
>> +	  /* jmp rel8  */
>> +	  gdb_byte rel8;
>> +
>> +	  if (target_read_memory (pc + 1, &rel8, 1) != 0)
>> +	    return -1;
>> +	  pc = pc + 2 + (signed char) rel8;
> 
> 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)
>> +{
> ...
> 
>> +      if (unwind_debug)
>> +	fprintf_unfiltered
>> +	  (gdb_stdlog,
>> +	   "amd64_windows_frame_play_insn: "
> 
> 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);
> 
> 
> 
>> +	      switch (PEX64_UNWCODE_CODE (p[1]))
>> +		{
>> +		case UWOP_PUSH_NONVOL:
>> +		  /* Push pre-decrements RSP.  */
>> +		  reg = amd64_windows_w2gdb_regnum[PEX64_UNWCODE_INFO (p[1])];
>> +		  cache->prev_reg_addr[reg] = cur_sp;
>> +		  cur_sp += 8;
>> +		  break;
>> +		case UWOP_ALLOC_LARGE:
>> +		  if (PEX64_UNWCODE_INFO (p[1]) == 0)
>> +		    cur_sp +=
> 
> Operator goes on next line (multiple instances in the patch,
> including with '=').
> 
>> +		      8 * extract_unsigned_integer (p + 2, 2, byte_order);
>> +		  else if (PEX64_UNWCODE_INFO (p[1]) == 1)
>> +		    cur_sp += extract_unsigned_integer (p + 2, 4, byte_order);
>> +		  else
>> +		    return;
>> +		  break;
> 
>> +
>> +  if (unwind_debug)
>> +    fprintf_unfiltered
>> +      (gdb_stdlog,
>> +       "amd64_windows_find_unwind_data:  image_base=%s, unwind_data=%s\n",
>> +       paddress (gdbarch, base), paddress (gdbarch, *unwind_info));
> 
> Another stale function name.
> 
>> +
>> +  if (*unwind_info & 1)
>> +    {
>> +      /* Unofficially documented unwind info redirection, when UNWIND_INFO
>> +	 address is odd.  */
> 
> What's "unofficially documented"?  Documented in some
> blog?

Yes, or some papers not coming from MS.

> 
>> +      struct external_pex64_runtime_function d;
>> +      CORE_ADDR sa, ea;
>> +
>> +      if (target_read_memory (base + (*unwind_info & ~1),
>> +			      (gdb_byte *) &d, sizeof (d)) != 0)
>> +	return -1;
>> +
>> +      *start_rva =
>> +	extract_unsigned_integer (d.rva_BeginAddress, 4, byte_order);
>> +      *unwind_info =
>> +	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_cache)
>> +{
>> +  struct gdbarch *gdbarch = get_frame_arch (this_frame);
>> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> +  struct amd64_windows_frame_cache *cache;
>> +  char buf[8];
> 
> gdb_byte.
> 
> 
>> +static struct value *
>> +amd64_windows_frame_prev_register (struct frame_info *this_frame,
>> +				   void **this_cache, int regnum)
>> +{
>> +  struct gdbarch *gdbarch = get_frame_arch (this_frame);
>> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> +  struct amd64_windows_frame_cache *cache =
>> +    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=%s\n",
>> +			gdbarch_register_name (gdbarch, regnum),
>> +			paddress (gdbarch, cache->prev_sp));
>> +
>> +  if (regnum >= AMD64_XMM0_REGNUM && regnum <= AMD64_XMM0_REGNUM + 15)
>> +      prev = cache->prev_xmm_addr[regnum - AMD64_XMM0_REGNUM];
> 
> Indentation looks odd.
> 
>> +  else if (regnum == AMD64_RSP_REGNUM)
>> +    {
>> +      prev = cache->prev_rsp_addr;
>> +      if (prev == 0)
>> +	return frame_unwind_got_constant (this_frame, regnum, cache->prev_sp);
>> +    }
>> +  else if (regnum >= AMD64_RAX_REGNUM && regnum <= AMD64_R15_REGNUM)
>> +    prev = cache->prev_reg_addr[regnum - AMD64_RAX_REGNUM];
>> +  else if (regnum == AMD64_RIP_REGNUM)
>> +    prev = cache->prev_rip_addr;
>> +  else
>> +    prev = 0;
>> +
>> +  if (prev && unwind_debug)
>> +    fprintf_unfiltered (gdb_stdlog, "  -> at %s\n", paddress (gdbarch, prev));
>> +
>> +  if (prev)
> 
> ('prev' is not really a boolean.  I'd prefer 'prev != 0'
> in these cases.)
> 
> 
>> +/* 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 = 0;
>> +  CORE_ADDR image_base, start_rva;
>> +  struct external_pex64_unwind_info ex_ui;
> 
> ex_ui could move to the inner scope.
> 
>> +
>> +  /* Use prologue size from unwind info.  */
>> +  if (amd64_windows_find_unwind_info (gdbarch, pc, &unwind_info,
>> +				      &image_base, &start_rva) == 0)
>> +    {
>> +      if (unwind_info == 0)
>> +	{
>> +	  /* Leaf function.  */
>> +	  return pc;
>> +	}
>> +      else if (target_read_memory (image_base + unwind_info,
>> +				   (gdb_byte *) &ex_ui, sizeof (ex_ui)) == 0
>> +	       && PEX64_UWI_VERSION (ex_ui.Version_Flags) == 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
>> +	= skip_prologue_using_sal (gdbarch, func_addr);
>> +
>> +      if (post_prologue_pc != 0)
>> +	return max (pc, post_prologue_pc);
>> +    }
>> +
>> +  return pc;
>> +}
>> +
>> static void
>> amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>> {
>> @@ -225,6 +910,9 @@ amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>> 
>>   set_gdbarch_auto_wide_charset (gdbarch, amd64_windows_auto_wide_charset);
>> 
>> +  frame_unwind_prepend_unwinder (gdbarch, &amd64_windows_frame_unwind);
> 
> I think a comment here referring to issues you
> mentioned in the mail would be good.
> 
>> +  set_gdbarch_skip_prologue (gdbarch, amd64_windows_skip_prologue);
>> +
>>   set_solib_ops (gdbarch, &solib_target_so_ops);
>> }
> 
> -- 
> Pedro Alves

Tristan.