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

[Tristan Gingold <gingold at adacore dot com>]

Hello,

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.

Ok.

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

Tristan.