From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14844 invoked by alias); 19 Nov 2012 05:06:30 -0000 Received: (qmail 14832 invoked by uid 22791); 19 Nov 2012 05:06:29 -0000 X-SWARE-Spam-Status: No, hits=-5.1 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-wg0-f43.google.com (HELO mail-wg0-f43.google.com) (74.125.82.43) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 19 Nov 2012 05:06:14 +0000 Received: by mail-wg0-f43.google.com with SMTP id dq12so892309wgb.12 for ; Sun, 18 Nov 2012 21:06:13 -0800 (PST) MIME-Version: 1.0 Received: by 10.180.92.103 with SMTP id cl7mr7132739wib.16.1353301572957; Sun, 18 Nov 2012 21:06:12 -0800 (PST) Received: by 10.217.64.194 with HTTP; Sun, 18 Nov 2012 21:06:12 -0800 (PST) In-Reply-To: References: <20121119044750.GB9964@adacore.com> Date: Mon, 19 Nov 2012 05:06: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 From: Andrew Pinski To: David Albert Cc: Joel Brobecker , gdb-patches Content-Type: text/plain; charset=UTF-8 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/msg00490.txt.bz2 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. 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