Re: [PATCH] Scan for Mach-O start address in LC_MAIN and properly check bfd_mach_o_scan_start_address's return value

[Andrew Pinski <pinskia@gmail.com>]

On Sun, Nov 18, 2012 at 9:03 PM, David Albert <davidbalbert@gmail.com> wrote:
> On Sun, Nov 18, 2012 at 11:47 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>> On Sun, Nov 18, 2012 at 11:13:21PM -0500, David Albert wrote:
>>> Last week there was a change comitted to GDB that added support for
>>> the new Mach-O load commands in OS X 10.8. I have two related changes
>>> that I've included in a patch below.
>>>
>>> 1) On December 7th of last year, bfd_mach_o_scan_start_address was
>>> changed to return a bfd_boolean, but no change was made to the code
>>> that checks its return value in bfd_mach_o_scan. This means that
>>> bfd_mach_o_scan always assumes that bfd_mach_o_scan_start_address
>>> succeeds. This patch fixes that.
>>>
>>> 2) The start address can now be found in LC_MAIN in addition to
>>> LC_THREAD and LC_UNIXTHREAD. I've updated
>>> bfd_mach_o_scan_start_address to reflect this. I moved the call to
>>> bfd_mach_o_flatten_sections to before bfd_mach_o_scan_start_address,
>>> because the code that gets the start address from LC_MAIN relies on
>>> nsects being set.
>>>
>>> I've never submitted a patch to GDB before, so please let me know if
>>> there are things I can fix. I also have a few notes:
>>>
>>> - I used the latest version of Apple's GDB 6.3 fork (gdb-1822) as a
>>> reference for some of my research. The changes I made are quite small
>>> (most of the patch consists of placing a block of existing code inside
>>> an if statement), but parts of the patch are similar to what I found
>>> in Apple's fork. The code is simple enough that there are not really
>>> many ways to solve the problem, but I want to make sure there aren't
>>> issues with copyright or licensing.
>>>
>>> - The one thing I'm not sure of is line 4047 which reads `if
>>> (mdata->nsects > 1)`. This is the same check found in Apple's GDB
>>> fork, however it's unclear why we'd need two or more sections, given
>>> that we only use the first section to calculate the start address. I
>>> thought about changing it, but I wanted to get the opinion of someone
>>> who knows more about Mach-O than I do.
>>>
>>> - I didn't run the test suite because it wasn't clear what parts of
>>> the suite I should run. I tested it manually and didn't find any
>>> regressions.
>>
>> Hi David,
>>
>> It's nice to see people interested in hacking on Darwin. Changes
>> in bfd/ should be sent to binutils, however. You can also Cc:
>> Tristan Gingold, to make sure you catch his attention.
>
> Thanks, Joel. I'll do this in the morning. If this patch gets pulled
> into binutils, what's the process for getting it in GDB? Does the
> latest BFD just get merged into GDB every now and then or does it have
> to be shepherded through by someone?


Right now gdb and binutils share the same cvs repo so there is nothing
special to do.

Thanks,
Andrew

>
> Thanks,
> -David
>
>>
>>
>>>
>>> Best,
>>> -David
>>>
>>> diff --git a/bfd/ChangeLog b/bfd/ChangeLog
>>> index c25ceb9..4e5ff3a 100644
>>> --- a/bfd/ChangeLog
>>> +++ b/bfd/ChangeLog
>>> @@ -1,3 +1,10 @@
>>> +2012-11-18  David Albert  <davidbalbert@gmail.com>
>>> +
>>> +     * mach-o.c (bfd_mach_o_scan): Properly check return value of
>>> +     bfd_mach_o_scan_start_address.
>>> +     (bfd_mach_o_scan_start_address): Also search for start address in
>>> +     BFD_MACH_O_LC_MAIN.
>>> +
>>>  2012-11-16  Joey Ye  <joey.ye@arm.com>
>>>
>>>       * elf32-arm.c (elf32_arm_final_link_relocate,
>>> diff --git a/bfd/mach-o.c b/bfd/mach-o.c
>>> index e44cf6d..4546540 100644
>>> --- a/bfd/mach-o.c
>>> +++ b/bfd/mach-o.c
>>> @@ -3970,66 +3970,89 @@ bfd_mach_o_scan_start_address (bfd *abfd)
>>>  {
>>>    bfd_mach_o_data_struct *mdata = bfd_mach_o_get_data (abfd);
>>>    bfd_mach_o_thread_command *cmd = NULL;
>>> +  bfd_mach_o_main_command *main_cmd = NULL;
>>>    unsigned long i;
>>>
>>>    for (i = 0; i < mdata->header.ncmds; i++)
>>> -    if ((mdata->commands[i].type == BFD_MACH_O_LC_THREAD) ||
>>> -        (mdata->commands[i].type == BFD_MACH_O_LC_UNIXTHREAD))
>>> -      {
>>> -        cmd = &mdata->commands[i].command.thread;
>>> -        break;
>>> -      }
>>> -
>>> -  if (cmd == NULL)
>>> -    return FALSE;
>>> +    {
>>> +      if ((mdata->commands[i].type == BFD_MACH_O_LC_THREAD) ||
>>> +          (mdata->commands[i].type == BFD_MACH_O_LC_UNIXTHREAD))
>>> +        {
>>> +          cmd = &mdata->commands[i].command.thread;
>>> +          break;
>>> +        }
>>> +      else if (mdata->commands[i].type == BFD_MACH_O_LC_MAIN)
>>> +        {
>>> +          main_cmd = &mdata->commands[i].command.main;
>>> +          break;
>>> +        }
>>> +    }
>>>
>>> -  /* FIXME: create a subtarget hook ?  */
>>> -  for (i = 0; i < cmd->nflavours; i++)
>>> +  if (cmd)
>>>      {
>>> -      if ((mdata->header.cputype == BFD_MACH_O_CPU_TYPE_I386)
>>> -       && (cmd->flavours[i].flavour
>>> -           == (unsigned long) BFD_MACH_O_x86_THREAD_STATE32))
>>> -     {
>>> -       unsigned char buf[4];
>>> +      /* FIXME: create a subtarget hook ?  */
>>> +      for (i = 0; i < cmd->nflavours; i++)
>>> +        {
>>> +          if ((mdata->header.cputype == BFD_MACH_O_CPU_TYPE_I386)
>>> +              && (cmd->flavours[i].flavour
>>> +                  == (unsigned long) BFD_MACH_O_x86_THREAD_STATE32))
>>> +            {
>>> +              unsigned char buf[4];
>>>
>>> -       if (bfd_seek (abfd, cmd->flavours[i].offset + 40, SEEK_SET) != 0
>>> -              || bfd_bread (buf, 4, abfd) != 4)
>>> -         return FALSE;
>>> +              if (bfd_seek (abfd, cmd->flavours[i].offset + 40, SEEK_SET) != 0
>>> +                  || bfd_bread (buf, 4, abfd) != 4)
>>> +                return FALSE;
>>>
>>> -       abfd->start_address = bfd_h_get_32 (abfd, buf);
>>> -     }
>>> -      else if ((mdata->header.cputype == BFD_MACH_O_CPU_TYPE_POWERPC)
>>> -            && (cmd->flavours[i].flavour == BFD_MACH_O_PPC_THREAD_STATE))
>>> -     {
>>> -       unsigned char buf[4];
>>> +              abfd->start_address = bfd_h_get_32 (abfd, buf);
>>> +            }
>>> +          else if ((mdata->header.cputype == BFD_MACH_O_CPU_TYPE_POWERPC)
>>> +                   && (cmd->flavours[i].flavour ==
>>> BFD_MACH_O_PPC_THREAD_STATE))
>>> +            {
>>> +              unsigned char buf[4];
>>>
>>> -       if (bfd_seek (abfd, cmd->flavours[i].offset + 0, SEEK_SET) != 0
>>> -              || bfd_bread (buf, 4, abfd) != 4)
>>> -         return FALSE;
>>> +              if (bfd_seek (abfd, cmd->flavours[i].offset + 0, SEEK_SET) != 0
>>> +                  || bfd_bread (buf, 4, abfd) != 4)
>>> +                return FALSE;
>>>
>>> -       abfd->start_address = bfd_h_get_32 (abfd, buf);
>>> -     }
>>> -      else if ((mdata->header.cputype == BFD_MACH_O_CPU_TYPE_POWERPC_64)
>>> -               && (cmd->flavours[i].flavour == BFD_MACH_O_PPC_THREAD_STATE64))
>>> -        {
>>> -          unsigned char buf[8];
>>> +              abfd->start_address = bfd_h_get_32 (abfd, buf);
>>> +            }
>>> +          else if ((mdata->header.cputype == BFD_MACH_O_CPU_TYPE_POWERPC_64)
>>> +                   && (cmd->flavours[i].flavour ==
>>> BFD_MACH_O_PPC_THREAD_STATE64))
>>> +            {
>>> +              unsigned char buf[8];
>>>
>>> -          if (bfd_seek (abfd, cmd->flavours[i].offset + 0, SEEK_SET) != 0
>>> -              || bfd_bread (buf, 8, abfd) != 8)
>>> -            return FALSE;
>>> +              if (bfd_seek (abfd, cmd->flavours[i].offset + 0, SEEK_SET) != 0
>>> +                  || bfd_bread (buf, 8, abfd) != 8)
>>> +                return FALSE;
>>>
>>> -          abfd->start_address = bfd_h_get_64 (abfd, buf);
>>> -        }
>>> -      else if ((mdata->header.cputype == BFD_MACH_O_CPU_TYPE_X86_64)
>>> -               && (cmd->flavours[i].flavour == BFD_MACH_O_x86_THREAD_STATE64))
>>> -        {
>>> -          unsigned char buf[8];
>>> +              abfd->start_address = bfd_h_get_64 (abfd, buf);
>>> +            }
>>> +          else if ((mdata->header.cputype == BFD_MACH_O_CPU_TYPE_X86_64)
>>> +                   && (cmd->flavours[i].flavour ==
>>> BFD_MACH_O_x86_THREAD_STATE64))
>>> +            {
>>> +              unsigned char buf[8];
>>>
>>> -          if (bfd_seek (abfd, cmd->flavours[i].offset + (16 * 8),
>>> SEEK_SET) != 0
>>> -              || bfd_bread (buf, 8, abfd) != 8)
>>> -            return FALSE;
>>> +              if (bfd_seek (abfd, cmd->flavours[i].offset + (16 * 8),
>>> SEEK_SET) != 0
>>> +                  || bfd_bread (buf, 8, abfd) != 8)
>>> +                return FALSE;
>>>
>>> -          abfd->start_address = bfd_h_get_64 (abfd, buf);
>>> +              abfd->start_address = bfd_h_get_64 (abfd, buf);
>>> +            }
>>> +        }
>>> +    }
>>> +  else if (main_cmd)
>>> +    {
>>> +      bfd_vma text_address = 0;
>>> +
>>> +      if (mdata->nsects > 1)
>>> +        {
>>> +          bfd_mach_o_section *text_sect = mdata->sections[0];
>>> +          if (text_sect)
>>> +            {
>>> +              text_address = text_sect->addr - text_sect->offset;
>>> +              abfd->start_address = main_cmd->entryoff + text_address;
>>> +              return TRUE;
>>> +            }
>>>          }
>>>      }
>>>
>>> @@ -4121,10 +4144,11 @@ bfd_mach_o_scan (bfd *abfd,
>>>       }
>>>      }
>>>
>>> -  if (bfd_mach_o_scan_start_address (abfd) < 0)
>>> +  bfd_mach_o_flatten_sections (abfd);
>>> +
>>> +  if (!bfd_mach_o_scan_start_address (abfd))
>>>      return FALSE;
>>>
>>> -  bfd_mach_o_flatten_sections (abfd);
>>>    return TRUE;
>>>  }
>>
>> --
>> Joel