From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 80731 invoked by alias); 14 Feb 2020 14:41:29 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 80710 invoked by uid 89); 14 Feb 2020 14:41:28 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-20.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail-qt1-f195.google.com Received: from mail-qt1-f195.google.com (HELO mail-qt1-f195.google.com) (209.85.160.195) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 14 Feb 2020 14:41:27 +0000 Received: by mail-qt1-f195.google.com with SMTP id d9so7051413qte.12 for ; Fri, 14 Feb 2020 06:41:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-language:content-transfer-encoding; bh=YoaikQk5CnrGF/RGM2IvaP1Ceo2mIK+0+O9G8X0omo0=; b=ldVx4kB8+SO09FrvLxKDwI6ia/Uss+gv8HaFTXTK+/2QxowUSuzQQztJ8u5PMqQntS wYZUlhfmwLOMwvX7ne7ODvkIXQaoI6jqq/VIo7cThCy4OoW2gSOMgoaYBe7WiOHa7PI3 9rg+v0YECO+qzR30PheNNEMFON234h8XKhlBeCYiwFV5jvz+rXA4vyXcOuWJQoWTvo6P ZSfHAIfUiELX7DIjcHr+mwgrzmrdIVQcFijcjHdx1mWGqZk3KSPJT5uGHJJF0Y9jDagY KJgtmtwSogyu9H93Duxq61ft8R/Q37iCksBbSYefnPuyP9pMYAOHYvR1xPFRiWkHRlXL VhIg== Return-Path: Received: from [192.168.0.185] ([191.34.221.109]) by smtp.gmail.com with ESMTPSA id h34sm3494404qtc.62.2020.02.14.06.41.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 14 Feb 2020 06:41:24 -0800 (PST) Subject: Re: [PATCH v2] Rebase executable to match relocated base address To: Hannes Domani , Gdb-patches References: <20200213181430.11259-1-ssbssa.ref@yahoo.de> <20200213181430.11259-1-ssbssa@yahoo.de> <4854af6a-e0c4-7714-6ed4-20697e0282c0@linaro.org> <578612266.5100822.1581683522459@mail.yahoo.com> <3df92ab1-e080-1cac-7500-9333714cbd58@linaro.org> <854900713.5159847.1581689224439@mail.yahoo.com> From: Luis Machado Message-ID: <38c52ecf-ec0a-cf8a-d284-b0aafc23759a@linaro.org> Date: Fri, 14 Feb 2020 14:41:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <854900713.5159847.1581689224439@mail.yahoo.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2020-02/txt/msg00564.txt.bz2 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 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 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.