This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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?

>  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?

> 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?

>  
> +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?

> +  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?

> +     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.

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.  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?


> +/* 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.

> +	   "%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?

> +      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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]