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

[David Albert <davidbalbert at gmail dot com>]

On Wed, Nov 21, 2012 at 5:15 AM, Tristan Gingold <gingold@adacore.com> wrote:
> 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'm not totally sure about this, but I don't think you're going to
find both of them in one file. /usr/include/mach-o/loader.h describes
LC_MAIN as a replacement for LC_UNIXTHREAD and in a quick survey of my
/usr/bin on 10.8.2, there are 155 binaries with LC_UNIXTHREAD, 526
with LC_MAIN, and none with both. Same results (no overlap) in /bin
and /usr/local/bin.

Apple's fork prefers THREAD/UNIXTHREAD to MAIN.

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

Sounds good.

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

I do not have a copyright assignment. I'm happy to do whatever's
easiest, whether that's doing an assignment (I run my own business, so
permission is easy) or having you rewrite the patch. Let me know what
you need from me.

Best,
-David

>
> Tristan.
>