From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19339 invoked by alias); 19 Nov 2012 05:18:50 -0000 Received: (qmail 19324 invoked by uid 22791); 19 Nov 2012 05:18:48 -0000 X-SWARE-Spam-Status: No, hits=-4.3 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE X-Spam-Check-By: sourceware.org Received: from mail-pa0-f41.google.com (HELO mail-pa0-f41.google.com) (209.85.220.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 19 Nov 2012 05:18:41 +0000 Received: by mail-pa0-f41.google.com with SMTP id fa10so3265014pad.0 for ; Sun, 18 Nov 2012 21:18:40 -0800 (PST) Received: by 10.69.1.1 with SMTP id bc1mr36024272pbd.102.1353302320295; Sun, 18 Nov 2012 21:18:40 -0800 (PST) MIME-Version: 1.0 Received: by 10.68.153.131 with HTTP; Sun, 18 Nov 2012 21:18:20 -0800 (PST) In-Reply-To: References: <20121119044750.GB9964@adacore.com> From: David Albert Date: Mon, 19 Nov 2012 05:18:00 -0000 Message-ID: Subject: Re: [PATCH] Scan for Mach-O start address in LC_MAIN and properly check bfd_mach_o_scan_start_address's return value To: Andrew Pinski Cc: Joel Brobecker , gdb-patches Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes 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 X-SW-Source: 2012-11/txt/msg00491.txt.bz2 On Mon, Nov 19, 2012 at 12:06 AM, Andrew Pinski wrote: > On Sun, Nov 18, 2012 at 9:03 PM, David Albert wrote: >> On Sun, Nov 18, 2012 at 11:47 PM, Joel Brobecker 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. Ah, good to know. Serves me right for using the git mirror then. Thanks for the info. Best, -David > > 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 >>>> + >>>> + * 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 >>>> >>>> * 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