[PATCH v2] Rebase executable to match relocated base address
Luis Machado
luis.machado@linaro.org
Fri Feb 14 14:41:00 GMT 2020
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.
More information about the Gdb-patches
mailing list