code review 60190043: debug/macho: Add support for opening fat/universal binaries.

debug/macho: Add support for opening fat/universal binaries. New testdata was created from existing using: $ lipo gcc-386-darwin-exec gcc-amd64-darwin-exec -create -output fat-gcc-386-amd64-darwin-exec Fixes issue 7250.

[Robert Sesek (Google), 2014-02-04 23:59:53]

Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go

[dave_cheney.net, 2014-02-05 00:43:40]

Very nice. A small set of comments, mainly around testing. https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/fat.go File src/pkg/debug/macho/fat.go (right): https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/fat.go... src/pkg/debug/macho/fat.go:44: ff := new(FatFile) var ff FatFile https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/fat.go... src/pkg/debug/macho/fat.go:118: return ff, nil return &ff, nil https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/fat.go... src/pkg/debug/macho/fat.go:134: return ff, nil why can't NewFatFile(f) do this directly ? https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... File src/pkg/debug/macho/file_test.go (right): https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... src/pkg/debug/macho/file_test.go:172: t.Error(err) t.Fatal(err) https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... src/pkg/debug/macho/file_test.go:173: return don't need the return https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... src/pkg/debug/macho/file_test.go:177: t.Errorf("OpenFat: uneunexpected magic number %#x, expected %#x", ff.Magic, MagicFat) unexpected should this be Fatalf? Can the test proceed if the magic number is wrong ? Please standardize on got/want, got/expected has fallen out of favor. https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... src/pkg/debug/macho/file_test.go:183: for i, _ := range ff.Arches { for i := ... { https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... src/pkg/debug/macho/file_test.go:193: continue drop the continue https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... src/pkg/debug/macho/file_test.go:200: _, err := OpenFat(filename) if _, err := ... ; err != nil { https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... src/pkg/debug/macho/file_test.go:202: t.Errorf("OpenFat %s: succeeded unexpectedly", filename) Should the be Fatalf ?

[Robert Sesek (Google), 2014-02-05 15:12:29]

Thanks for the review! One question about your comments, otherwise addressed all. https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/fat.go File src/pkg/debug/macho/fat.go (right): https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/fat.go... src/pkg/debug/macho/fat.go:44: ff := new(FatFile) On 2014/02/05 00:43:40, dfc wrote: > var ff FatFile What's the rationale behind this? I noticed that file.go does not take this approach. https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/fat.go... src/pkg/debug/macho/fat.go:134: return ff, nil On 2014/02/05 00:43:40, dfc wrote: > why can't NewFatFile(f) do this directly ? Done. https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... File src/pkg/debug/macho/file_test.go (right): https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... src/pkg/debug/macho/file_test.go:172: t.Error(err) On 2014/02/05 00:43:40, dfc wrote: > t.Fatal(err) Done. https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... src/pkg/debug/macho/file_test.go:173: return On 2014/02/05 00:43:40, dfc wrote: > don't need the return Done. https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... src/pkg/debug/macho/file_test.go:177: t.Errorf("OpenFat: uneunexpected magic number %#x, expected %#x", ff.Magic, MagicFat) On 2014/02/05 00:43:40, dfc wrote: > unexpected > > should this be Fatalf? Can the test proceed if the magic number is wrong ? > > Please standardize on got/want, got/expected has fallen out of favor. Done. https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... src/pkg/debug/macho/file_test.go:183: for i, _ := range ff.Arches { On 2014/02/05 00:43:40, dfc wrote: > for i := ... { Done. https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... src/pkg/debug/macho/file_test.go:193: continue On 2014/02/05 00:43:40, dfc wrote: > drop the continue Done. https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... src/pkg/debug/macho/file_test.go:200: _, err := OpenFat(filename) On 2014/02/05 00:43:40, dfc wrote: > if _, err := ... ; err != nil { Done. https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... src/pkg/debug/macho/file_test.go:202: t.Errorf("OpenFat %s: succeeded unexpectedly", filename) On 2014/02/05 00:43:40, dfc wrote: > Should the be Fatalf ? I think the test could still proceed if this failed.

[dave_cheney.net, 2014-02-05 22:55:26]

https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/fat.go File src/pkg/debug/macho/fat.go (right): https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/fat.go... src/pkg/debug/macho/fat.go:44: ff := new(FatFile) On 2014/02/05 15:12:30, Robert Sesek (Google) wrote: > On 2014/02/05 00:43:40, dfc wrote: > > var ff FatFile > > What's the rationale behind this? I noticed that file.go does not take this > approach. The std lib has a mixture of styles. This package, being one of the lesser used, shows this. v = new(T) is discouraged in favor of var v = T then returning &v if necessary. https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... File src/pkg/debug/macho/file_test.go (right): https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/file_t... src/pkg/debug/macho/file_test.go:202: t.Errorf("OpenFat %s: succeeded unexpectedly", filename) On 2014/02/05 15:12:30, Robert Sesek (Google) wrote: > On 2014/02/05 00:43:40, dfc wrote: > > Should the be Fatalf ? > > I think the test could still proceed if this failed. Fair enough. The basic idea is: if the test cannot continue successfully, then t.Fatal{,f}. If it can, t.Error{,f}. The exception to this is if the error is handled in a new goroutine, not the one the test is running from, in which case t.Fatalf should not be called, use t.Error; return;

[Robert Sesek (Google), 2014-02-05 23:00:48]

https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/fat.go File src/pkg/debug/macho/fat.go (right): https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/fat.go... src/pkg/debug/macho/fat.go:44: ff := new(FatFile) On 2014/02/05 22:55:26, dfc wrote: > On 2014/02/05 15:12:30, Robert Sesek (Google) wrote: > > On 2014/02/05 00:43:40, dfc wrote: > > > var ff FatFile > > > > What's the rationale behind this? I noticed that file.go does not take this > > approach. > > The std lib has a mixture of styles. This package, being one of the lesser used, > shows this. > > v = new(T) > > is discouraged in favor of > > var v = T > > then returning &v if necessary. Done, thanks. Is there some deeper reason behind this or is it just preferred style? https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/fat.go... src/pkg/debug/macho/fat.go:118: return ff, nil On 2014/02/05 00:43:40, dfc wrote: > return &ff, nil Done.

[dave_cheney.net, 2014-02-05 23:03:34]

 > On 6 Feb 2014, at 10:00, rsesek@google.com wrote: > > > https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/fat.go > File src/pkg/debug/macho/fat.go (right): > > https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/fat.go... > src/pkg/debug/macho/fat.go:44: ff := new(FatFile) >> On 2014/02/05 22:55:26, dfc wrote: >> On 2014/02/05 15:12:30, Robert Sesek (Google) wrote: >> > On 2014/02/05 00:43:40, dfc wrote: >> > > var ff FatFile >> > >> > What's the rationale behind this? I noticed that file.go does not > take this >> > approach. > >> The std lib has a mixture of styles. This package, being one of the > lesser used, >> shows this. > >> v = new(T) > >> is discouraged in favor of > >> var v = T > >> then returning &v if necessary. > > Done, thanks. Is there some deeper reason behind this or is it just > preferred style? nee tends to upset people. I don't know the serious reason. > > https://codereview.appspot.com/60190043/diff/60001/src/pkg/debug/macho/fat.go... > src/pkg/debug/macho/fat.go:118: return ff, nil >> On 2014/02/05 00:43:40, dfc wrote: >> return &ff, nil > > Done. > > https://codereview.appspot.com/60190043/ 

[Robert Sesek (Google), 2014-02-06 01:58:43]

On 2014/02/05 23:03:34, dfc wrote: > nee tends to upset people. I don't know the serious reason. Got it, I'll leave it be then ;). I think this is ready for submit. I'm not sure how this instance of Rietveld will handle the binary file, though (our Chromium instance doesn't AFAIK).

[josharian, 2014-02-06 02:07:31]

On 2014/02/06 01:58:43, Robert Sesek (Google) wrote: > On 2014/02/05 23:03:34, dfc wrote: > > nee tends to upset people. I don't know the serious reason. > > Got it, I'll leave it be then ;). > > I think this is ready for submit. I'm not sure how this instance of Rietveld > will handle the binary file, though (our Chromium instance doesn't AFAIK). If the binary file is an executable, it might be better to hex encode it. See https://code.google.com/p/go/source/detail?r=29156b17bdb7.

[Robert Sesek (Google), 2014-02-07 20:40:22]

Ping? On 2014/02/06 02:07:31, josharian wrote: > On 2014/02/06 01:58:43, Robert Sesek (Google) wrote: > > On 2014/02/05 23:03:34, dfc wrote: > > > nee tends to upset people. I don't know the serious reason. > > > > Got it, I'll leave it be then ;). > > > > I think this is ready for submit. I'm not sure how this instance of Rietveld > > will handle the binary file, though (our Chromium instance doesn't AFAIK). > > If the binary file is an executable, it might be better to hex encode it. See > https://code.google.com/p/go/source/detail?r=29156b17bdb7. That wasn't done for the other test files in this directory.

[dave_cheney.net, 2014-02-08 02:42:28]

On 2014/02/07 20:40:22, Robert Sesek (Google) wrote: > Ping? I'm sorry, I let myself get distracted for a ... Squirrel!! I'll do a final check and submit this today. > > On 2014/02/06 02:07:31, josharian wrote: > > On 2014/02/06 01:58:43, Robert Sesek (Google) wrote: > > > On 2014/02/05 23:03:34, dfc wrote: > > > > nee tends to upset people. I don't know the serious reason. > > > > > > Got it, I'll leave it be then ;). > > > > > > I think this is ready for submit. I'm not sure how this instance of Rietveld > > > will handle the binary file, though (our Chromium instance doesn't AFAIK). > > > > If the binary file is an executable, it might be better to hex encode it. See > > https://code.google.com/p/go/source/detail?r=29156b17bdb7. > > That wasn't done for the other test files in this directory. I think this is ok as is. We don't want a whole bunch of unrelated changes to the modes of existing files lumped in with this change so it is good to be consistent with the prevailing style of the package. If someone wants to tackle this cleanup in a following CL that would be fine.

[dave_cheney.net, 2014-02-08 02:45:19]

Opps, still need that CLA paperwork. I'll chase people about that now. On Sat, Feb 8, 2014 at 1:42 PM, <dave@cheney.net> wrote: > On 2014/02/07 20:40:22, Robert Sesek (Google) wrote: > >> Ping? >> > > I'm sorry, I let myself get distracted for a ... Squirrel!! > > I'll do a final check and submit this today. > > > > On 2014/02/06 02:07:31, josharian wrote: >> > On 2014/02/06 01:58:43, Robert Sesek (Google) wrote: >> > > On 2014/02/05 23:03:34, dfc wrote: >> > > > nee tends to upset people. I don't know the serious reason. >> > > >> > > Got it, I'll leave it be then ;). >> > > >> > > I think this is ready for submit. I'm not sure how this instance >> > of Rietveld > >> > > will handle the binary file, though (our Chromium instance doesn't >> > AFAIK). > >> > >> > If the binary file is an executable, it might be better to hex >> > encode it. See > >> > https://code.google.com/p/go/source/detail?r=29156b17bdb7. >> > > That wasn't done for the other test files in this directory. >> > > I think this is ok as is. We don't want a whole bunch of unrelated > changes to the modes of existing files lumped in with this change so it > is good to be consistent with the prevailing style of the package. If > someone wants to tackle this cleanup in a following CL that would be > fine. > > https://codereview.appspot.com/60190043/ > 

[dave_cheney.net, 2014-02-10 00:01:27]

Hmm, odessa(~/go/src) % hg clpatch 60190043 add : patch did not apply cleanly Robert, could you please try `hg mail 60190043` again

[Robert Sesek (Google), 2014-02-10 16:04:14]

Hello golang-codereviews@googlegroups.com, dave@cheney.net, josharian@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.

[Robert Sesek (Google), 2014-02-10 16:04:55]

On 2014/02/10 00:01:27, dfc wrote: > Hmm, > > odessa(~/go/src) % hg clpatch 60190043 > add : patch did not apply cleanly > > > Robert, could you please try `hg mail 60190043` again Done. Also, I think Google Inc. should cover my CLA requirements.

[dave_cheney.net, 2014-02-10 20:53:32]

Yup, brad has done the paperwork for you as a google employee. I think the submit failure was related to the test fixture, i'll try again today. On Tue, Feb 11, 2014 at 3:04 AM, <rsesek@google.com> wrote: > On 2014/02/10 00:01:27, dfc wrote: > >> Hmm, >> > > odessa(~/go/src) % hg clpatch 60190043 >> add : patch did not apply cleanly >> > > > Robert, could you please try `hg mail 60190043` again >> > > Done. Also, I think Google Inc. should cover my CLA requirements. > > https://codereview.appspot.com/60190043/ > 

[dave_cheney.net, 2014-02-10 22:31:18]

 Crap. Hg doesn't like the binary diff. Andrew/Brad are you able to assist? This one might need to be submitted directly. > On 11 Feb 2014, at 7:53, Dave Cheney <dave@cheney.net> wrote: > > Yup, brad has done the paperwork for you as a google employee. > > I think the submit failure was related to the test fixture, i'll try again today. > > >> On Tue, Feb 11, 2014 at 3:04 AM, <rsesek@google.com> wrote: >>> On 2014/02/10 00:01:27, dfc wrote: >>> Hmm, >> >>> odessa(~/go/src) % hg clpatch 60190043 >>> add : patch did not apply cleanly >> >> >>> Robert, could you please try `hg mail 60190043` again >> >> Done. Also, I think Google Inc. should cover my CLA requirements. >> >> https://codereview.appspot.com/60190043/ > 

[Robert Sesek (Google), 2014-02-10 23:04:58]

I uploaded the binary file here: https://drive.google.com/a/google.com/file/d/0B6CbNbqSn1CxeXBBWHpFckFGZGs/edi... MD5 hash is ffa108d6667d7d630d8a7081c9de2e6b On 2014/02/10 22:31:18, dfc wrote: > Crap. Hg doesn't like the binary diff. > > Andrew/Brad are you able to assist? This one might need to be submitted > directly. > > > On 11 Feb 2014, at 7:53, Dave Cheney <mailto:dave@cheney.net> wrote: > > > > Yup, brad has done the paperwork for you as a google employee. > > > > I think the submit failure was related to the test fixture, i'll try again > today. > > > > > >> On Tue, Feb 11, 2014 at 3:04 AM, <mailto:rsesek@google.com> wrote: > >>> On 2014/02/10 00:01:27, dfc wrote: > >>> Hmm, > >> > >>> odessa(~/go/src) % hg clpatch 60190043 > >>> add : patch did not apply cleanly > >> > >> > >>> Robert, could you please try `hg mail 60190043` again > >> > >> Done. Also, I think Google Inc. should cover my CLA requirements. > >> > >> https://codereview.appspot.com/60190043/ > >

[bradfitz, 2014-02-10 23:45:35]

That file can't be accessed. I don't think the google.com domain lets you share outside of it, by domain settings or something. Anyway, I also don't think we check in binary files to the repo anymore, since Debian got all fussy about it. We now just hex-encode it instead, to make it SUPER OBVIOUS that it's testdata instead of a malicious corporate binary blob without source. Then the test code can de-hex it (writing it to a temp directory if necessary). I'm not sure whether that applies here or not, but it might also solve whatever hg problems you're having. On Mon, Feb 10, 2014 at 3:04 PM, <rsesek@google.com> wrote: > I uploaded the binary file here: > https://drive.google.com/a/google.com/file/d/0B6CbNbqSn1CxeXBBWHpFckFGZGs/ > edit?usp=sharing > > MD5 hash is ffa108d6667d7d630d8a7081c9de2e6b > > > On 2014/02/10 22:31:18, dfc wrote: > >> Crap. Hg doesn't like the binary diff. >> > > Andrew/Brad are you able to assist? This one might need to be >> > submitted > >> directly. >> > > > On 11 Feb 2014, at 7:53, Dave Cheney <mailto:dave@cheney.net> wrote: >> > >> > Yup, brad has done the paperwork for you as a google employee. >> > >> > I think the submit failure was related to the test fixture, i'll try >> > again > >> today. >> >> > >> > >> >> On Tue, Feb 11, 2014 at 3:04 AM, <mailto:rsesek@google.com> wrote: >> >>> On 2014/02/10 00:01:27, dfc wrote: >> >>> Hmm, >> >> >> >>> odessa(~/go/src) % hg clpatch 60190043 >> >>> add : patch did not apply cleanly >> >> >> >> >> >>> Robert, could you please try `hg mail 60190043` again >> >> >> >> Done. Also, I think Google Inc. should cover my CLA requirements. >> >> >> >> https://codereview.appspot.com/60190043/ >> > >> > > > https://codereview.appspot.com/60190043/ > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. > 

[Robert Sesek (Google), 2014-02-11 00:01:23]

On 2014/02/10 23:45:35, bradfitz wrote: > That file can't be accessed. I don't think the http://google.com domain lets you > share outside of it, by domain settings or something. It's shared to google.com and Dave. You cannot share outside the domain, unfortunately. > Anyway, I also don't think we check in binary files to the repo anymore, > since Debian got all fussy about it. We now just hex-encode it instead, to > make it SUPER OBVIOUS that it's testdata instead of a malicious corporate > binary blob without source. Then the test code can de-hex it (writing it > to a temp directory if necessary). > > I'm not sure whether that applies here or not, but it might also solve > whatever hg problems you're having. > I don't know either, but the other testdata/ in this package is also binary. It would also make testing Open and OpenFat impracticable, since those functions open the file itself by path name. > > On Mon, Feb 10, 2014 at 3:04 PM, <mailto:rsesek@google.com> wrote: > > > I uploaded the binary file here: > > https://drive.google.com/a/google.com/file/d/0B6CbNbqSn1CxeXBBWHpFckFGZGs/ > > edit?usp=sharing > > > > MD5 hash is ffa108d6667d7d630d8a7081c9de2e6b > > > > > > On 2014/02/10 22:31:18, dfc wrote: > > > >> Crap. Hg doesn't like the binary diff. > >> > > > > Andrew/Brad are you able to assist? This one might need to be > >> > > submitted > > > >> directly. > >> > > > > > On 11 Feb 2014, at 7:53, Dave Cheney <mailto:dave@cheney.net> wrote: > >> > > >> > Yup, brad has done the paperwork for you as a google employee. > >> > > >> > I think the submit failure was related to the test fixture, i'll try > >> > > again > > > >> today. > >> > >> > > >> > > >> >> On Tue, Feb 11, 2014 at 3:04 AM, <mailto:rsesek@google.com> wrote: > >> >>> On 2014/02/10 00:01:27, dfc wrote: > >> >>> Hmm, > >> >> > >> >>> odessa(~/go/src) % hg clpatch 60190043 > >> >>> add : patch did not apply cleanly > >> >> > >> >> > >> >>> Robert, could you please try `hg mail 60190043` again > >> >> > >> >> Done. Also, I think Google Inc. should cover my CLA requirements. > >> >> > >> >> https://codereview.appspot.com/60190043/ > >> > > >> > > > > > > https://codereview.appspot.com/60190043/ > > > > -- > > You received this message because you are subscribed to the Google Groups > > "golang-codereviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to mailto:golang-codereviews+unsubscribe@googlegroups.com. > > For more options, visit https://groups.google.com/groups/opt_out. > >

[bradfitz, 2014-02-11 00:14:46]

macho.NewFile takes an io.ReaderAt. Or you could always just de-dex it and right it to out to a tempdir, like I mentioned before. On Mon, Feb 10, 2014 at 4:01 PM, <rsesek@google.com> wrote: > On 2014/02/10 23:45:35, bradfitz wrote: > >> That file can't be accessed. I don't think the http://google.com >> > domain lets you > >> share outside of it, by domain settings or something. >> > > It's shared to google.com and Dave. You cannot share outside the domain, > unfortunately. > > > Anyway, I also don't think we check in binary files to the repo >> > anymore, > >> since Debian got all fussy about it. We now just hex-encode it >> > instead, to > >> make it SUPER OBVIOUS that it's testdata instead of a malicious >> > corporate > >> binary blob without source. Then the test code can de-hex it (writing >> > it > >> to a temp directory if necessary). >> > > I'm not sure whether that applies here or not, but it might also solve >> whatever hg problems you're having. >> > > > I don't know either, but the other testdata/ in this package is also > binary. It would also make testing Open and OpenFat impracticable, since > those functions open the file itself by path name. > > > > On Mon, Feb 10, 2014 at 3:04 PM, <mailto:rsesek@google.com> wrote: >> > > > I uploaded the binary file here: >> > >> > https://drive.google.com/a/google.com/file/d/0B6CbNbqSn1CxeXBBWHpFckFGZGs/ > >> > edit?usp=sharing >> > >> > MD5 hash is ffa108d6667d7d630d8a7081c9de2e6b >> > >> > >> > On 2014/02/10 22:31:18, dfc wrote: >> > >> >> Crap. Hg doesn't like the binary diff. >> >> >> > >> > Andrew/Brad are you able to assist? This one might need to be >> >> >> > submitted >> > >> >> directly. >> >> >> > >> > > On 11 Feb 2014, at 7:53, Dave Cheney <mailto:dave@cheney.net> >> > wrote: > >> >> > >> >> > Yup, brad has done the paperwork for you as a google employee. >> >> > >> >> > I think the submit failure was related to the test fixture, i'll >> > try > >> >> >> > again >> > >> >> today. >> >> >> >> > >> >> > >> >> >> On Tue, Feb 11, 2014 at 3:04 AM, <mailto:rsesek@google.com> >> > wrote: > >> >> >>> On 2014/02/10 00:01:27, dfc wrote: >> >> >>> Hmm, >> >> >> >> >> >>> odessa(~/go/src) % hg clpatch 60190043 >> >> >>> add : patch did not apply cleanly >> >> >> >> >> >> >> >> >>> Robert, could you please try `hg mail 60190043` again >> >> >> >> >> >> Done. Also, I think Google Inc. should cover my CLA >> > requirements. > >> >> >> >> >> >> https://codereview.appspot.com/60190043/ >> >> > >> >> >> > >> > >> > https://codereview.appspot.com/60190043/ >> > >> > -- >> > You received this message because you are subscribed to the Google >> > Groups > >> > "golang-codereviews" group. >> > To unsubscribe from this group and stop receiving emails from it, >> > send an > >> > email to mailto:golang-codereviews+unsubscribe@googlegroups.com. >> >> > For more options, visit https://groups.google.com/groups/opt_out. >> > >> > > > https://codereview.appspot.com/60190043/ > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. > 

[Robert Sesek (Google), 2014-02-11 00:21:37]

On 2014/02/11 00:14:46, bradfitz wrote: > macho.NewFile takes an io.ReaderAt. Yeah, but Open/OpenFat are also tested. > Or you could always just de-dex it and right it to out to a tempdir, like I > mentioned before. True. I don't see a clear benefit to doing this, but I defer. (Personally I think it will make using nm, otool, and lipo more inconvenient on test files in the future). If this data should be HEX encoded, it seems better to do so in a separate CL, before this, for the existing data, and then land this with new HEX data. Let me know if you want me to do that.

[bradfitz, 2014-02-11 00:23:25]

I actually have no preferences here. I was just throwing it out there. I'll defer to somebody who cares. On Feb 10, 2014 4:21 PM, <rsesek@google.com> wrote: > On 2014/02/11 00:14:46, bradfitz wrote: > >> macho.NewFile takes an io.ReaderAt. >> > > Yeah, but Open/OpenFat are also tested. > > Or you could always just de-dex it and right it to out to a tempdir, >> > like I > >> mentioned before. >> > > True. I don't see a clear benefit to doing this, but I defer. > (Personally I think it will make using nm, otool, and lipo more > inconvenient on test files in the future). If this data should be HEX > encoded, it seems better to do so in a separate CL, before this, for the > existing data, and then land this with new HEX data. Let me know if you > want me to do that. > > https://codereview.appspot.com/60190043/ > 

[dave_cheney.net, 2014-02-11 03:24:07]

https://codereview.appspot.com/61720044 appears to work. Robert - can you please do `hg file -d 60190043 $GOROOT/src/pkg/debug/macho/testdata/fat-gcc-386-amd64-darwin-exec', then 'hg mail 60190043' to remove the fixture from your CL. On Tue, Feb 11, 2014 at 11:23 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > I actually have no preferences here. I was just throwing it out there. > > I'll defer to somebody who cares. > On Feb 10, 2014 4:21 PM, <rsesek@google.com> wrote: > >> On 2014/02/11 00:14:46, bradfitz wrote: >> >>> macho.NewFile takes an io.ReaderAt. >>> >> >> Yeah, but Open/OpenFat are also tested. >> >> Or you could always just de-dex it and right it to out to a tempdir, >>> >> like I >> >>> mentioned before. >>> >> >> True. I don't see a clear benefit to doing this, but I defer. >> (Personally I think it will make using nm, otool, and lipo more >> inconvenient on test files in the future). If this data should be HEX >> encoded, it seems better to do so in a separate CL, before this, for the >> existing data, and then land this with new HEX data. Let me know if you >> want me to do that. >> >> https://codereview.appspot.com/60190043/ >> > 

[Robert Sesek (Google), 2014-02-11 03:37:42]

Hello golang-codereviews@googlegroups.com, dave@cheney.net, josharian@gmail.com, bradfitz@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.

[dave_cheney.net, 2014-02-11 03:45:22]

On 2014/02/11 03:37:42, Robert Sesek (Google) wrote: > Hello mailto:golang-codereviews@googlegroups.com, mailto:dave@cheney.net, mailto:josharian@gmail.com, > mailto:bradfitz@golang.org (cc: mailto:golang-codereviews@googlegroups.com), > > Please take another look. LGTM. I'll take responsibility for landing this once the prereq with the fixture is approved. Thanks for sticking with us, normally things are a lot smoother.

[dave_cheney.net, 2014-02-12 01:02:31]

ping, https://codereview.appspot.com/61720044/ is needed to land this change.

[dave_cheney.net, 2014-02-13 00:05:52]

*** Submitted as https://code.google.com/p/go/source/detail?r=3e6f1641fda8 *** debug/macho: Add support for opening fat/universal binaries. New testdata was created from existing using: $ lipo gcc-386-darwin-exec gcc-amd64-darwin-exec -create -output fat-gcc-386-amd64-darwin-exec Fixes issue 7250. LGTM=dave R=golang-codereviews, dave, josharian, bradfitz CC=golang-codereviews https://codereview.appspot.com/60190043 Committer: Dave Cheney <dave@cheney.net>