Re: [PATCH v2] Rebase executable to match relocated base address

["Hannes Domani via gdb-patches" <gdb-patches@sourceware.org>]

 Am Freitag, 14. Februar 2020, 14:50:07 MEZ hat Luis Machado <luis.machado@linaro.org> Folgendes geschrieben:

> On 2/14/20 9:32 AM, Hannes Domani via gdb-patches wrote:
> >  Am Freitag, 14. Februar 2020, 12:02:03 MEZ hat Luis Machado <luis.machado@linaro.org> Folgendes geschrieben:
> >
> >> Hi,
> >>
> >> On 2/13/20 3:14 PM, Hannes Domani via gdb-patches wrote:
> >>> Windows executables linked with -dynamicbase get a new base address
> >>> when loaded, which makes debugging impossible if the executable isn't
> >>> also rebased in gdb.
> >>>
> >>> The new base address is read from the Process Environment Block.
> >>> ---
> >>> v2:
> >>> This version now no longer needs the fake auxv entry.
> >>> ---
> >>>    gdb/windows-tdep.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++
> >>>    1 file changed, 49 insertions(+)
> >>>
> >>
> >> Thanks. This version looks better.
> >>
> >>> diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
> >>> index 6eef3fbd96..29c0a828a7 100644
> >>> --- a/gdb/windows-tdep.c
> >>> +++ b/gdb/windows-tdep.c
> >>> @@ -34,6 +34,9 @@
> >>>    #include "solib.h"
> >>>    #include "solib-target.h"
> >>>    #include "gdbcore.h"
> >>> +#include "coff/internal.h"
> >>> +#include "libcoff.h"
> >>> +#include "solist.h"
> >>>
> >>>    /* Windows signal numbers differ between MinGW flavors and between
> >>>        those and Cygwin.  The below enumeration was gleaned from the
> >>> @@ -812,6 +815,50 @@ windows_get_siginfo_type (struct gdbarch *gdbarch)
> >>>      return siginfo_type;
> >>>    }
> >>>
> >>> +/* Implement the "solib_create_inferior_hook" target_so_ops method.  */
> >>> +
> >>> +static void
> >>> +windows_solib_create_inferior_hook (int from_tty)
> >>> +{
> >>> +  CORE_ADDR exec_base = 0;
> >>> +
> >>> +  /* Find base address of main executable in
> >>> +    TIB->process_environment_block->image_base_address.  */
> >>> +  struct gdbarch *gdbarch = target_gdbarch ();
> >>> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> >>> +  int ptr_bytes;
> >>> +  int peb_offset;  /* Offset of process_environment_block in TIB.  */
> >>> +  int base_offset; /* Offset of image_base_address in PEB.  */
> >>> +  if (gdbarch_ptr_bit (gdbarch) == 32)
> >>> +    {
> >>> +      ptr_bytes = 4;
> >>> +      peb_offset = 48;
> >>> +      base_offset = 8;
> >>> +    }
> >>> +  else
> >>> +    {
> >>> +      ptr_bytes = 8;
> >>> +      peb_offset = 96;
> >>> +      base_offset = 16;
> >>> +    }
> >>
> >> How about stashing the above offsets in windows_gdbarch_data, and then
> >> using them here?
> >
> > To be honest, that would seem a bit weird for me, since they are just these
> > simple numbers, and aren't used anywhere else.
> >
> >
>
> Fair enough. I don't have a strong opinion on this, but i usually try to
> avoid having these magic numbers in the code without some pointers to
> where those came from. Folks dealing with this code in the future may
> try to understand what it is doing and how they came to be.
>
> Having them at a single place, with some explanation, helps with that.
> That's my take on it, at least.

Then I guess I need to make better comments than this:
  /* Find base address of main executable in
     TIB->process_environment_block->image_base_address.  */

  int peb_offset;  /* Offset of process_environment_block in TIB.  */
  int base_offset; /* Offset of image_base_address in PEB.  */