This is the mail archive of the
mailing list for the binutils project.
Re: [PATCH] Scan for Mach-O start address in LC_MAIN and properly check bfd_mach_o_scan_start_address's return value
- From: Tristan Gingold <gingold at adacore dot com>
- To: David Albert <davidbalbert at gmail dot com>
- Cc: binutils at sourceware dot org
- Date: Wed, 21 Nov 2012 11:15:22 +0100
- Subject: Re: [PATCH] Scan for Mach-O start address in LC_MAIN and properly check bfd_mach_o_scan_start_address's return value
- References: <CAKYxTUYkjYQyPzOvTXQNJEyH10YJxy=H1YppPCDuVz=AT1Ln3g@mail.gmail.com>
On Nov 20, 2012, at 8:26 PM, David Albert wrote:
> Hi there,
> I sent this change to BFD to the gdb-patches list a couple of days ago
> by mistake. I've included the patch as well as an updated summary
> below. Joel Brobecker on the gdb-patches list suggested that I cc
> Tristan Gingold, which I've done.
> Last week there was a change committed to BFD that added support for
> the new Mach-O load commands in OS X 10.8. This patch includes two
> related changes.
> 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.
This part is ok.
> 2) A program's 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 a 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.
That part is ok. What should be the preference when both MAIN and
THREAD/UNIXTHREAD are present ? I'd prefer to slightly simplify
your change, and deal directly with LC_MAIN when found.
> I've never submitted a patch to binutils (or any other GNU project)
> 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.
I don't think there are any issues here, that's too simple.
> - 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 to `if (mdata->nsects > 0)`, but I wanted to
> get the opinion of someone who knows more about Mach-O than I do.
I don't see any reason not the compare with 0.
> - Currently, if no appropriate load command is found,
> bfd_mach_o_scan_start_address returns FALSE. I changed this to return
> TRUE because there are valid Mach-O files that don't have start
> addresses (shared libraries, etc). This bug wasn't causing problems
> before because the return value of bfd_mach_o_scan_start_address was
> always assumed to be true.
> - I ran `make check` in the binutils tree both before and after
> applying the patch and found no visible. I've included the results
> below. You can find the full output here:
> https://gist.github.com/4120289. Are there other tests I need to run?
No, that's fine.
Do you have an FSF assignment ? If no, I cannot commit your patch as is,
but I can write a simplified version of it and commit that version.