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

[David Albert <davidbalbert@gmail.com>]

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?

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