Issue 1124002: Create a new tool to upload Mac system library symbols. (Closed)
|
Created:
10 years, 9 months ago by rsesek Modified:
10 years, 9 months ago CC:
google-breakpad-dev_googlegroups.com Base URL:
http://google-breakpad.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionCreate a new tool to upload Mac system library symbols. R=andybons@chromium.org, mark@chromium.org Committed: https://code.google.com/p/google-breakpad/source/detail?r=1278 Patch Set 1 #
Total comments: 12
Patch Set 2 : #
Total comments: 27
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 8
Patch Set 5 : #
Total comments: 32
Patch Set 6 : #
Total comments: 4
Patch Set 7 : #
Total comments: 2
MessagesTotal messages: 18
Thanks in advance. This is a low-priority review since I've been sitting on this fiel for months. andybons: overall review/Go review mark: fyi/systems sanity review and approval
Sign in to reply to this message.
initial scan https://breakpad.appspot.com/1124002/diff/1/src/tools/mac/upload_system_symbo... File src/tools/mac/upload_system_symbols/upload_system_symbols.go (right): https://breakpad.appspot.com/1124002/diff/1/src/tools/mac/upload_system_symbo... src/tools/mac/upload_system_symbols/upload_system_symbols.go:33: any reason for the newlines between flag defs? https://breakpad.appspot.com/1124002/diff/1/src/tools/mac/upload_system_symbo... src/tools/mac/upload_system_symbols/upload_system_symbols.go:42: // Paths in the systemRoot that should be scanned for shared libraries. "pathsToScan contains..." From http://golang.org/doc/effective_go.html#commentary : "The first sentence should be a one-sentence summary that starts with the name being declared." https://breakpad.appspot.com/1124002/diff/1/src/tools/mac/upload_system_symbo... src/tools/mac/upload_system_symbols/upload_system_symbols.go:84: if *dumpOnly != "" { dumpOnly mapping to dump-to make me whiplash back to the flags to see what they mean. I would never guess that dumpOnly would point to a directory path. https://breakpad.appspot.com/1124002/diff/1/src/tools/mac/upload_system_symbo... src/tools/mac/upload_system_symbols/upload_system_symbols.go:87: fatal("Could not stat -dump-to location: %v", err) this error is going to look a bit redundant. You may be better off just fataling with just the error. Could not stat -dump-to location: stat dumpDir: no such file or directory https://breakpad.appspot.com/1124002/diff/1/src/tools/mac/upload_system_symbo... src/tools/mac/upload_system_symbols/upload_system_symbols.go:114: type WorkerPool struct { given that everything is within one package, you don't need to export any of this. also, why not define it as type workerPool sync.WaitGroup or type workerPool struct { sync.WaitGroup } to avoid having to write the Wait func below https://breakpad.appspot.com/1124002/diff/1/src/tools/mac/upload_system_symbo... src/tools/mac/upload_system_symbols/upload_system_symbols.go:175: fmt.Fprintf(os.Stderr, "Error running symupload(%s, %s), attempt %d: %v: %s\n", symfile, server, i, err, output) why not use the log package? you pipe a lot into stderr as it is and recreate log.Fatal below.
Sign in to reply to this message.
Thanks! https://breakpad.appspot.com/1124002/diff/1/src/tools/mac/upload_system_symbo... File src/tools/mac/upload_system_symbols/upload_system_symbols.go (right): https://breakpad.appspot.com/1124002/diff/1/src/tools/mac/upload_system_symbo... src/tools/mac/upload_system_symbols/upload_system_symbols.go:33: On 2014/01/31 02:24:24, andybons wrote: > any reason for the newlines between flag defs? No. Removed. https://breakpad.appspot.com/1124002/diff/1/src/tools/mac/upload_system_symbo... src/tools/mac/upload_system_symbols/upload_system_symbols.go:42: // Paths in the systemRoot that should be scanned for shared libraries. On 2014/01/31 02:24:24, andybons wrote: > "pathsToScan contains..." > > From http://golang.org/doc/effective_go.html#commentary : "The first sentence > should be a one-sentence summary that starts with the name being declared." Done. https://breakpad.appspot.com/1124002/diff/1/src/tools/mac/upload_system_symbo... src/tools/mac/upload_system_symbols/upload_system_symbols.go:84: if *dumpOnly != "" { On 2014/01/31 02:24:24, andybons wrote: > dumpOnly mapping to dump-to make me whiplash back to the flags to see what they > mean. > > I would never guess that dumpOnly would point to a directory path. Yeah, I knew I should have changed this before uploading. As written, it seems like a boolean value. Changed to dumpOnlyPath and uploadOnlyPath. https://breakpad.appspot.com/1124002/diff/1/src/tools/mac/upload_system_symbo... src/tools/mac/upload_system_symbols/upload_system_symbols.go:87: fatal("Could not stat -dump-to location: %v", err) On 2014/01/31 02:24:24, andybons wrote: > this error is going to look a bit redundant. > > You may be better off just fataling with just the error. > > Could not stat -dump-to location: stat dumpDir: no such file or directory Done, but kept a reference to the -dump-to flag. https://breakpad.appspot.com/1124002/diff/1/src/tools/mac/upload_system_symbo... src/tools/mac/upload_system_symbols/upload_system_symbols.go:114: type WorkerPool struct { On 2014/01/31 02:24:24, andybons wrote: > given that everything is within one package, you don't need to export any of > this. True, but in larger exes I like to note which symbols are public and which are internal to help structure the program better. It also means that if/when this gets too big and needs to be split up, it makes it obvious where the logic is. > also, why not define it as > > type workerPool sync.WaitGroup > > or type workerPool struct { > sync.WaitGroup > } > > to avoid having to write the Wait func below Both of those would make .Add and .Done public methods of WorkerPool. Those are details that shouldn't be exposed. https://breakpad.appspot.com/1124002/diff/1/src/tools/mac/upload_system_symbo... src/tools/mac/upload_system_symbols/upload_system_symbols.go:175: fmt.Fprintf(os.Stderr, "Error running symupload(%s, %s), attempt %d: %v: %s\n", symfile, server, i, err, output) On 2014/01/31 02:24:24, andybons wrote: > why not use the log package? you pipe a lot into stderr as it is and recreate > log.Fatal below. Done.
Sign in to reply to this message.
https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... File src/tools/mac/upload_system_symbols/upload_system_symbols.go (right): https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:1: /* ©? https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:33: breakpadTools = flag.String("breakpad-tools", "./out/Release/", "Path to the Breakpad tools directory, containing dump_syms and symupload.") The leading “./” shouldn’t be necessary. https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:36: systemRoot = flag.String("system-root", "", "Path to the root of the Mac OS X system whose symbols will be dumped.") Isn’t it reasonable to default to “/”? https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:45: "/System/Library/PrivateFrameworks", This is fine, but I thought you said you were scanning all of /Library and /System/Library. There are things like address book plug-ins, color pickers, etc. https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:51: "http://clients2.google.com/cr/symbol", Does https work? https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:57: regexp.MustCompile(`/System/Library/Frameworks/Python.framework/`), Since these are REs, backwhack the dot on this line and the next one. https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:93: if *dumpOnlyPath == "" { I don’t know Go, but since you just assigned *dumpOnlyPath to dumpPath, would you be able to avoid the * by testing dumpPath instead of *dumpOnlyPath here? And if so, would that be beneficial? https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:174: log.Printf("Error running symupload(%s, %s), attempt %d: %v: %s\n", symfile, server, i, err, output) (For when you’re not using -dump-to or -upload-from…) Something to leave the dumps for failed uploads around so that it’s possible to -upload-from the set of files that failed in the future, without having to redump everything, and reupload even the things that were already successfully uploaded? https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:373: // Skip line 1. ? https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:390: if strings.Contains(line, "Mach-O dynamically linked shared library i386") { You need to care about more than MH_DYLIB. At the very least, you want MH_BUNDLE too. “file” will show that as “Mach-O bundle i386”. Otherwise there’s no point of listing /Library/QuickTime, etc. https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:418: return magic == 0xfeedface || magic == 0xcefaedfe || magic == 0xfeedfacf || Since you only care about x86 and x86_64, you don’t have to check for other-endian magic here. You will only ever see MH_MAGIC (0xfeedface) and MH_MAGIC_64 (0xfeedfacf). MH_CIGAM and MH_CIGAM_64 would only be ppc, and this program doesn’t care about dumping ppc. https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:419: magic == 0xcffaedfe || magic == 0xcafebabe || magic == 0xbebafeca The FAT magic is not endian-sensitive. Since you always read a little-endian 32-bit quantity, the FAT magic will allays(1.0) show up as FAT_CIGAM (0xbebafeca) and allays(2.0) FAT_MAGIC (0xcafebabe).
Sign in to reply to this message.
https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... File src/tools/mac/upload_system_symbols/upload_system_symbols.go (right): https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:1: /* On 2014/01/31 16:55:44, Mark Mentovai wrote: > ©? Done. https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:33: breakpadTools = flag.String("breakpad-tools", "./out/Release/", "Path to the Breakpad tools directory, containing dump_syms and symupload.") On 2014/01/31 16:55:44, Mark Mentovai wrote: > The leading “./” shouldn’t be necessary. Done. https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:36: systemRoot = flag.String("system-root", "", "Path to the root of the Mac OS X system whose symbols will be dumped.") On 2014/01/31 16:55:44, Mark Mentovai wrote: > Isn’t it reasonable to default to “/”? The old script didn't, and I do like making this explicit so you don't accidentally start doing this. https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:45: "/System/Library/PrivateFrameworks", On 2014/01/31 16:55:44, Mark Mentovai wrote: > This is fine, but I thought you said you were scanning all of /Library and > /System/Library. There are things like address book plug-ins, color pickers, > etc. Not /Library since that mostly contains user stuff (QuickTime appears to be the exception). It turns out that only these directories in /System/Library are necessary, based on my querying the crash server. https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:51: "http://clients2.google.com/cr/symbol", On 2014/01/31 16:55:44, Mark Mentovai wrote: > Does https work? Done. https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:57: regexp.MustCompile(`/System/Library/Frameworks/Python.framework/`), On 2014/01/31 16:55:44, Mark Mentovai wrote: > Since these are REs, backwhack the dot on this line and the next one. Done. https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:93: if *dumpOnlyPath == "" { On 2014/01/31 16:55:44, Mark Mentovai wrote: > I don’t know Go, but since you just assigned *dumpOnlyPath to dumpPath, would > you be able to avoid the * by testing dumpPath instead of *dumpOnlyPath here? > And if so, would that be beneficial? Yeah, you could definitely do that (I had it written that way before), but I think it expresses the logic of the condition better to reference the flag. Perf should be negligible. https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:174: log.Printf("Error running symupload(%s, %s), attempt %d: %v: %s\n", symfile, server, i, err, output) On 2014/01/31 16:55:44, Mark Mentovai wrote: > (For when you’re not using -dump-to or -upload-from…) > > Something to leave the dumps for failed uploads around so that it’s possible to > -upload-from the set of files that failed in the future, without having to > redump everything, and reupload even the things that were already successfully > uploaded? This is something I thought about but didn't wire in. How useful do you think this is? I can probably add it without too much complexity, but since the upload servers are not accessible externally, I almost always have to do a split operation: dump on a lab machine with the new OS and upload on a corp machine. https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:373: // Skip line 1. On 2014/01/31 16:55:44, Mark Mentovai wrote: > ? Done. https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:390: if strings.Contains(line, "Mach-O dynamically linked shared library i386") { On 2014/01/31 16:55:44, Mark Mentovai wrote: > You need to care about more than MH_DYLIB. At the very least, you want MH_BUNDLE > too. “file” will show that as “Mach-O bundle i386”. > > Otherwise there’s no point of listing /Library/QuickTime, etc. Ooops. Good point. Did I mention I hate the output of file? https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:418: return magic == 0xfeedface || magic == 0xcefaedfe || magic == 0xfeedfacf || On 2014/01/31 16:55:44, Mark Mentovai wrote: > Since you only care about x86 and x86_64, you don’t have to check for > other-endian magic here. You will only ever see MH_MAGIC (0xfeedface) and > MH_MAGIC_64 (0xfeedfacf). MH_CIGAM and MH_CIGAM_64 would only be ppc, and this > program doesn’t care about dumping ppc. Done. Maybe we'll add it back if Apple ever goes big endian again. For the record, the old script did ppc and ppc64 (why?). https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:419: magic == 0xcffaedfe || magic == 0xcafebabe || magic == 0xbebafeca On 2014/01/31 16:55:44, Mark Mentovai wrote: > The FAT magic is not endian-sensitive. Since you always read a little-endian > 32-bit quantity, the FAT magic will allays(1.0) show up as FAT_CIGAM > (0xbebafeca) and allays(2.0) FAT_MAGIC (0xcafebabe). Done.
Sign in to reply to this message.
Where’s the test? :) https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... File src/tools/mac/upload_system_symbols/upload_system_symbols.go (right): https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:174: log.Printf("Error running symupload(%s, %s), attempt %d: %v: %s\n", symfile, server, i, err, output) rsesek wrote: > This is something I thought about but didn't wire in. How useful do you think > this is? I can probably add it without too much complexity, but since the upload > servers are not accessible externally, I almost always have to do a split > operation: dump on a lab machine with the new OS and upload on a corp machine. It’s not a must-have and it doesn’t have to be in v1.go. It’s just something that I thought might be useful. If the number of failures is small, you can just try reuploading those manually. But if you find that you’ve got to reupload manually every time you do this, it might be a nice addition. A sort of halfway approach would be to just print out all of the symuploads that failed, all together, so that if you were running with -upload-from, you could just copy all of those commands and paste them back into a shell to retry them. https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:390: if strings.Contains(line, "Mach-O dynamically linked shared library i386") { rsesek wrote: > On 2014/01/31 16:55:44, Mark Mentovai wrote: > > You need to care about more than MH_DYLIB. At the very least, you want > MH_BUNDLE > > too. “file” will show that as “Mach-O bundle i386”. > > > > Otherwise there’s no point of listing /Library/QuickTime, etc. > > Ooops. Good point. Did I mention I hate the output of file? You don’t have to use it if you don’t want to… https://breakpad.appspot.com/1124002/diff/30001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:418: return magic == 0xfeedface || magic == 0xcefaedfe || magic == 0xfeedfacf || rsesek wrote: > Done. Maybe we'll add it back if Apple ever goes big endian again. For the > record, the old script did ppc and ppc64 (why?). I can’t promise that all of Google never released anything built for ppc64. https://breakpad.appspot.com/1124002/diff/80001/src/tools/mac/upload_system_s... File src/tools/mac/upload_system_symbols/upload_system_symbols.go (right): https://breakpad.appspot.com/1124002/diff/80001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:384: // Test that this is either a dynamic library or a bundle (MH_DYLIB and MH_BUNDLE). If you’re testing for a dylib OR a bundle (emphasis on OR), you’re looking for MH_DYLIB OR MH_BUNDLE, not AND. https://breakpad.appspot.com/1124002/diff/80001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:385: var fileMachOPattern = regexp.MustCompile(`Mach-O (64-bit)? (bundle|dynamically linked shared library) (i386|x86_64)\n$`) This pattern is not correct. One of the spaces needs to be inside the “64-bit” group. https://breakpad.appspot.com/1124002/diff/80001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:385: var fileMachOPattern = regexp.MustCompile(`Mach-O (64-bit)? (bundle|dynamically linked shared library) (i386|x86_64)\n$`) And just for consistency’s sake, since the comment lists dylib first and bundle second, the order you list things here in the regular expression should match. https://breakpad.appspot.com/1124002/diff/80001/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:407: // e.g. "Mach-O universal binary with 2 architectures". My previous “?” comment must not have been descriptive enough, so let me clarify: This is correct for fat files, but not for thin files. “file --brief” on a fat file will produce “Mach-O universal binary with N architectures” on the first line, followed by N lines, one for each architecture. But on a thin file, it will only produce one line, something like “Mach-O dynamically linked shared library i386”. You will consume that line here, and there will be no more “file” output to look at in the loop beginning on line 416. We do need to be able to dump thin files from here.
Sign in to reply to this message.
Okay, I got sick of dealing with file(1)'s stupid output, so now this reads the Mach-O headers directly.
Sign in to reply to this message.
https://breakpad.appspot.com/1124002/diff/60003/src/tools/mac/upload_system_s... File src/tools/mac/upload_system_symbols/arch_reader.go (right): https://breakpad.appspot.com/1124002/diff/60003/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/arch_reader.go:57: Arch_i386 = "i386" mixing underscores with camelCase? bleh https://breakpad.appspot.com/1124002/diff/60003/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/arch_reader.go:78: f, err := os.Open(filepath) could you use debug/macho for any of the stuff you’re doing in this file? https://breakpad.appspot.com/1124002/diff/60003/src/tools/mac/upload_system_s... File src/tools/mac/upload_system_symbols/upload_system_symbols.go (right): https://breakpad.appspot.com/1124002/diff/60003/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:255: func (dq *DumpQueue) done() { depends on how defensive you want to be, but calling done twice on a dumpqueue without reinitting the queue will panic without a very useful message https://breakpad.appspot.com/1124002/diff/60003/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:276: os.Remove(symfile) do you want to ignore the error from os.Remove?
Sign in to reply to this message.
https://breakpad.appspot.com/1124002/diff/60003/src/tools/mac/upload_system_s... File src/tools/mac/upload_system_symbols/arch_reader.go (right): https://breakpad.appspot.com/1124002/diff/60003/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/arch_reader.go:57: Arch_i386 = "i386" On 2014/02/01 04:37:05, andybons wrote: > mixing underscores with camelCase? bleh Done. https://breakpad.appspot.com/1124002/diff/60003/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/arch_reader.go:78: f, err := os.Open(filepath) On 2014/02/01 04:37:05, andybons wrote: > could you use debug/macho for any of the stuff you’re doing in this file? I didn't know about that package, and it looked promising at first, but I found that it does not handle fat binaries. I could replace readThinXHeader with that, but it also reads the load commands and symbol tables, which I don't need. I filed https://code.google.com/p/go/issues/detail?id=7250 for this. https://breakpad.appspot.com/1124002/diff/60003/src/tools/mac/upload_system_s... File src/tools/mac/upload_system_symbols/upload_system_symbols.go (right): https://breakpad.appspot.com/1124002/diff/60003/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:255: func (dq *DumpQueue) done() { On 2014/02/01 04:37:05, andybons wrote: > depends on how defensive you want to be, but calling done twice on a dumpqueue > without reinitting the queue will panic without a very useful message I don't think I need to be defensive here. The error seems pretty adequate, too: http://play.golang.org/p/FhvVmSK34D https://breakpad.appspot.com/1124002/diff/60003/src/tools/mac/upload_system_s... src/tools/mac/upload_system_symbols/upload_system_symbols.go:276: os.Remove(symfile) On 2014/02/01 04:37:05, andybons wrote: > do you want to ignore the error from os.Remove? No, because this is already in an error handler.
Sign in to reply to this message.
lgtm Pending a final pass from Markus. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/arch_reader.go (right): https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:58: ArchX86_64 = "x86_64" i guess i'll let this _ slide. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/upload_system_symbols.go (right): https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/upload_system_symbols.go:78: uploadServers = []string{ going to guess that these won't change any time soon, but otherwise I'd put them in a flag as comma-separated values. nbd either way.
Sign in to reply to this message.
https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/arch_reader.go (right): https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:76: // images in the file (will be more than one if it is a fat image). A fat file may have only one architecture in it, so you should say “may be more than one,” not “will be.” https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:93: f.Seek(0, 0) I find it hard to believe that there’s no constant like SEEK_SET to use here. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:116: } Technically you should probably be considering the CPU subtype too. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:125: func readThin64Header(f *os.File) ([]ImageInfo, error) { This function is almost identical to readThin32Header. Can you combine the two functions, and have them do the two things they need to do differently determined by an extra function argument? https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:168: f.Seek(int64(header.offset), 0) Same comment as before regarding whatever SEEK_SET equivalent I’m sure is available. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:173: thinarch, err = readThin32Header(f) What if the fat header said to expect i386 but readThin32Header returned some other architecture because some joker added ARM support in some lame half-baked way? That should be just as errory as the default: case here. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:175: thinarch, err = readThin64Header(f) What if there’s a dupe, like when one fat file contains the same arch more than once? You should detect and handle that with an error. (Because if it happens, it probably doesn’t mean that the file really has the same arch more than once, it probably means that there’s a bug in your own code that has not discriminated different subtypes properly, or something like that. Not handling it could silently overwrite data.) https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:175: thinarch, err = readThin64Header(f) You optionally could make it an error for a fat file to have multiple architectures with different Mach-O types for each architecture (like i386 MH_EXECUTE and x86_64 MH_DYLIB). https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/arch_reader_test.go (right): https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader_test.go:57: expected := make(map[string]bool) You already have something named expected. Now you have another. That makes it hard to follow. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader_test.go:70: expected[ii.Arch] = true You should make sure that ii.Arch is already in the expected map, so that you can fail the test when an unexpected architecture is found. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/testdata/Makefile (right): https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/testdata/Makefile:1: LDFLAGS=-Xlinker -dylib This is confusing, because you don’t use it for every ld. I’d just write it inline for the links that need it. Also, you should tell the compiler driver “-dynamiclib”, which it understands and translates to -dylib when it invokes the linker. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/testdata/Makefile:5: maintest: main.c This is no good. You produce archtest.exe, so the target name should be archtest.exe, not maintest. Ideally you’d write this as archtest32.exe: executeable.c clang -m32 $< -o $@ archtest64.exe: executable.c clang -m64 $< -o $@ archtest.exe: archtest32.exe archtest64.exe lipo $^ -create -output $@ And similar for the dylib variant. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/testdata/Makefile:5: maintest: main.c The filenames should make their purpose clear if you’re using them differently. So you should have executable.c and dylib.c. But you don’t necessarily need a different test file. The executable needs a main(), but nothing prevents you from having a main() in a dylib.
Sign in to reply to this message.
https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/arch_reader_test.go (right): https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader_test.go:70: expected[ii.Arch] = true You should make sure that ii.Arch is already in the expected map (with value false), so that you can fail the test when an unexpected architecture is found.
Sign in to reply to this message.
https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/arch_reader.go (right): https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:76: // images in the file (will be more than one if it is a fat image). On 2014/02/03 18:43:13, Mark Mentovai wrote: > A fat file may have only one architecture in it, so you should say “may be more > than one,” not “will be.” Done. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:93: f.Seek(0, 0) On 2014/02/03 18:43:13, Mark Mentovai wrote: > I find it hard to believe that there’s no constant like SEEK_SET to use here. Done. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:116: } On 2014/02/03 18:43:13, Mark Mentovai wrote: > Technically you should probably be considering the CPU subtype too. I'm working on upstreaming fat, and the upstream MachO package already reads the CPU subtype. The subtype would matter for iOS but not OS X, and since I'm not yet doing iOS support, that shouldn't be an issue. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:125: func readThin64Header(f *os.File) ([]ImageInfo, error) { On 2014/02/03 18:43:13, Mark Mentovai wrote: > This function is almost identical to readThin32Header. Can you combine the two > functions, and have them do the two things they need to do differently > determined by an extra function argument? Not really since the header structs are different. It'd basically just be a single function with each of these existing functions as different branches of an if(). If I had generics, then yes. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:168: f.Seek(int64(header.offset), 0) On 2014/02/03 18:43:13, Mark Mentovai wrote: > Same comment as before regarding whatever SEEK_SET equivalent I’m sure is > available. Done. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:173: thinarch, err = readThin32Header(f) On 2014/02/03 18:43:13, Mark Mentovai wrote: > What if the fat header said to expect i386 but readThin32Header returned some > other architecture because some joker added ARM support in some lame half-baked > way? That should be just as errory as the default: case here. Done. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:175: thinarch, err = readThin64Header(f) On 2014/02/03 18:43:13, Mark Mentovai wrote: > You optionally could make it an error for a fat file to have multiple > architectures with different Mach-O types for each architecture (like i386 > MH_EXECUTE and x86_64 MH_DYLIB). I'll cross this bridge when Apple ships busted bits like that. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:175: thinarch, err = readThin64Header(f) On 2014/02/03 18:43:13, Mark Mentovai wrote: > What if there’s a dupe, like when one fat file contains the same arch more than > once? You should detect and handle that with an error. > > (Because if it happens, it probably doesn’t mean that the file really has the > same arch more than once, it probably means that there’s a bug in your own code > that has not discriminated different subtypes properly, or something like that. > Not handling it could silently overwrite data.) Done. Not going to handle subtypes for the reason stated above. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/arch_reader_test.go (right): https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader_test.go:57: expected := make(map[string]bool) On 2014/02/03 18:43:13, Mark Mentovai wrote: > You already have something named expected. Now you have another. That makes it > hard to follow. Done. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader_test.go:70: expected[ii.Arch] = true On 2014/02/03 18:43:13, Mark Mentovai wrote: > You should make sure that ii.Arch is already in the expected map, so that you > can fail the test when an unexpected architecture is found. Done. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/testdata/Makefile (right): https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/testdata/Makefile:1: LDFLAGS=-Xlinker -dylib On 2014/02/03 18:43:13, Mark Mentovai wrote: > This is confusing, because you don’t use it for every ld. I’d just write it > inline for the links that need it. > > Also, you should tell the compiler driver “-dynamiclib”, which it understands > and translates to -dylib when it invokes the linker. Done. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/testdata/Makefile:5: maintest: main.c On 2014/02/03 18:43:13, Mark Mentovai wrote: > This is no good. You produce archtest.exe, so the target name should be > archtest.exe, not maintest. > > Ideally you’d write this as > > archtest32.exe: executeable.c > clang -m32 $< -o $@ > > archtest64.exe: executable.c > clang -m64 $< -o $@ > > archtest.exe: archtest32.exe archtest64.exe > lipo $^ -create -output $@ > > And similar for the dylib variant. Done. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/testdata/Makefile:5: maintest: main.c On 2014/02/03 18:43:13, Mark Mentovai wrote: > The filenames should make their purpose clear if you’re using them differently. > So you should have executable.c and dylib.c. But you don’t necessarily need a > different test file. The executable needs a main(), but nothing prevents you > from having a main() in a dylib. Done. https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/upload_system_symbols.go (right): https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/upload_system_symbols.go:78: uploadServers = []string{ On 2014/02/03 16:34:19, andybons wrote: > going to guess that these won't change any time soon, but otherwise I'd put them > in a flag as comma-separated values. nbd either way. Correct. These are static.
Sign in to reply to this message.
LGTM https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/arch_reader.go (right): https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:125: func readThin64Header(f *os.File) ([]ImageInfo, error) { rsesek wrote: > On 2014/02/03 18:43:13, Mark Mentovai wrote: > > This function is almost identical to readThin32Header. Can you combine the two > > functions, and have them do the two things they need to do differently > > determined by an extra function argument? > > Not really since the header structs are different. It'd basically just be a > single function with each of these existing functions as different branches of > an if(). If I had generics, then yes. The whole bottom parts of the functions are 100% identical. If you refactored just the stuff that’s different into their own functions, like readThin32FiletypeAndArch and readThin64FiletypeAndArch, then your single readThinHeader function would be like (assuming that “expected_magic” is the new argument to the function) if (expected_magic == C.kMachHeaderMagic32) { filetype, cputype, err = readThin32FiletypeAndArch(f) } else { filetype, cputype, err = readThin64FiletypeAndArch(f) } /* common stuff goes here */ https://breakpad.appspot.com/1124002/diff/130010/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/arch_reader.go (right): https://breakpad.appspot.com/1124002/diff/130010/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:93: f.Seek(0, 0) You said “done” but you didn’t actually fix this. https://breakpad.appspot.com/1124002/diff/130010/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/testdata/Makefile (right): https://breakpad.appspot.com/1124002/diff/130010/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/testdata/Makefile:1: LDFLAGS=-Xlinker -dylib Unused. Remove.
Sign in to reply to this message.
Thanks all! https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/arch_reader.go (right): https://breakpad.appspot.com/1124002/diff/110002/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:125: func readThin64Header(f *os.File) ([]ImageInfo, error) { On 2014/02/03 22:01:46, Mark Mentovai wrote: > rsesek wrote: > > On 2014/02/03 18:43:13, Mark Mentovai wrote: > > > This function is almost identical to readThin32Header. Can you combine the > two > > > functions, and have them do the two things they need to do differently > > > determined by an extra function argument? > > > > Not really since the header structs are different. It'd basically just be a > > single function with each of these existing functions as different branches of > > an if(). If I had generics, then yes. > > The whole bottom parts of the functions are 100% identical. If you refactored > just the stuff that’s different into their own functions, like > readThin32FiletypeAndArch and readThin64FiletypeAndArch, then your single > readThinHeader function would be like > > (assuming that “expected_magic” is the new argument to the function) > if (expected_magic == C.kMachHeaderMagic32) { > filetype, cputype, err = readThin32FiletypeAndArch(f) > } else { > filetype, cputype, err = readThin64FiletypeAndArch(f) > } > /* common stuff goes here */ It's actually more LoC, but done because it eliminates redundancy. https://breakpad.appspot.com/1124002/diff/130010/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/arch_reader.go (right): https://breakpad.appspot.com/1124002/diff/130010/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:93: f.Seek(0, 0) On 2014/02/03 22:01:46, Mark Mentovai wrote: > You said “done” but you didn’t actually fix this. Done for realz. https://breakpad.appspot.com/1124002/diff/130010/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/testdata/Makefile (right): https://breakpad.appspot.com/1124002/diff/130010/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/testdata/Makefile:1: LDFLAGS=-Xlinker -dylib On 2014/02/03 22:01:46, Mark Mentovai wrote: > Unused. Remove. Done.
Sign in to reply to this message.
LGTM https://breakpad.appspot.com/1124002/diff/230001/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/arch_reader.go (right): https://breakpad.appspot.com/1124002/diff/230001/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:119: panic(fmt.Sprintf("Unexpected magic %d", magic)) hex me
Sign in to reply to this message.
https://breakpad.appspot.com/1124002/diff/230001/src/tools/mac/upload_system_... File src/tools/mac/upload_system_symbols/arch_reader.go (right): https://breakpad.appspot.com/1124002/diff/230001/src/tools/mac/upload_system_... src/tools/mac/upload_system_symbols/arch_reader.go:119: panic(fmt.Sprintf("Unexpected magic %d", magic)) On 2014/02/03 22:48:29, Mark Mentovai wrote: > hex me Done.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
