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

[Joel Brobecker <brobecker@adacore.com>]

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.


> 
> 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