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

[Luis Machado <luis.machado@linaro.org>]

On 2/14/20 11:07 AM, Hannes Domani via gdb-patches wrote:
>   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.  */

The comments are fine. It's just the location of those, in local 
variables in a particular function, that seemed to me could be improved.

But like i said, I'm fine keeping it this way if it is deemed 
appropriate for the windows target.

I have no further comments on the patch. I'll defer to the maintainers 
for approvals.