I couldn't find a bug on file, but this is what mshal and Ted are suggesting in https://bugzilla.mozilla.org/show_bug.cgi?id=1042476#c10 and below, moving to a model where we dump symbols for a binary right after we link it, rather than doing them all at once during the automation tiers.

I'll start taking a look at this later in the week if nobody claims it.

My latest thinking on this is that we should just add extra dependencies on each (non-host) binary, like for Windows: foo.sym: foo.dll <run symbolstore.py on foo.pdb> for Linux it'd look more like: libfoo.so.sym: libfoo.so <run symbolstore.py on libfoo.so>

If we do it in make targets, presumably that means we can kill a lot of the multiprocess/process pool complexity in symbolstore.py. Good riddance. Also, we could leverage the separate process invocations by make to perform compression so when we go to produce the final archive all we have to do is splice bits inside a container instead of doing compression. With proper make rules, that would also save us from performing redundant compression when inputs haven't changed.

Compressing everything there would be a nice additional win, but as a start if we can at least do the symbol dumping and all the other things that are involved (dsymutil on OS X, makecab to compress PDB files on Windows, objcopy to split debug info on Linux) then buildsymbols could just load binaries.json, locate the generated files for each binary, and stuff them into a zip file.

I have a version of this going with the only notable difference in outputs that symbols for "firefox-bin" are omitted (it's not around during the compile tier). Is there any point to having symbols for "firefox-bin" in addition to "firefox"?

I don't think that matters, no. We only get `firefox` in crash reports because that's the actual binary.

Comment on attachment 8852228 [details] Bug 1337986 - Dump symbols during the compile tier. https://reviewboard.mozilla.org/r/124416/#review127216 This is a great start, thanks for taking this on! I have a few suggestions for ways we could do things differently in the interest of having less complex Makefile logic, but I'd love to hear your thoughts on them. I love the concept here--having less work to do during the packaging phase should be good, and letting make manage the number of concurrent jobs during the compile tier instead of having symbolstore.py do as much work as it can while other things are happening ought to help with contention. I'm interested to see what effect this has on overall build times! Obviously the compile tier will take a little longer, but hopefully getting the libxul symbol dumping done while we're linking the various little binaries that link after libxul should make things faster. ::: config/rules.mk:908 (Diff revision 1) > $(ASOBJS): > $(REPORT_BUILD_VERBOSE) > $(AS) $(ASOUTOPTION)$@ $(ASFLAGS) $($(notdir $<)_FLAGS) $(AS_DASH_C_FLAG) $(_VPATH_SRCS) > endif > > +ifeq ($(OS_ARCH),WINNT) While you're here, what do you think about moving all this logic into symbolstore.py, just getting all of these defaults from `buildconfig` values, rather than passing everything down as an argument from Makefiles? If you think this patch series is complicated enough as-is then let's file a followup to do that. ::: config/rules.mk:928 (Diff revision 1) > +endif > +MAKE_SYM_STORE_ARGS += --install-manifest=$(DEPTH)/_build_manifests/install/dist_include,$(DIST)/include > + > +SYM_STORE_SOURCE_DIRS := $(topsrcdir) > + > +define syms_template I've been thinking it would be nice to start removing content from rules.mk. Instead of doing this, what if instead we just wrote out thes rules in the recursive make backend directly into the Makefile? We could hide the logic behind a `py_action`, so the Makefile contents could be something simple like: ``` syms:: whatever.syms_track whatever.syms_track: whatever $(call py_action,dumpsymbols,$< $@) ``` It seems like the logic would be easier to follow in Python than the hoops you have to jump through here in Make, at the very least. ::: config/rules.mk:934 (Diff revision 1) > +syms:: $(2) > +$(2): $(1) > + @echo dumping symbols for $(1) > +# Our tracking file, if present, will contain path(s) to the previously generated > +# symbols. Remove them in this case so we don't simply accumulate old symbols > +# during incremental builds. So, as an alternate, possibly more involved proposal, we could make symbolstore simply put the `.sym` file and `.pd_` or whatever native debug symbol file in the current directory in the objdir, and then for the `buildsymbols` step instead of just running `zip`, write a small Python script that generates the zip by grabbing all the files from their objdir locations, reading the first line of the .sym file to figure out the right path inside the zip, and creating the zip file. We should have all the info we'd need in binaries.json. That would also let us tweak the compression per-file (since .sym files should be compressed but not the other files) which might speed up zip generation a little. What do you think? ::: config/rules.mk:951 (Diff revision 1) > + > +ifndef MOZ_PROFILE_GENERATE > +ifneq (,$(filter 1,$(MOZ_AUTOMATION_BUILD_SYMBOLS))) > +ifneq (,$(filter $(DIST)/bin%,$(FINAL_TARGET))) > +DUMP_SYMS_TARGETS := $(SHARED_LIBRARY) $(PROGRAM) $(SIMPLE_PROGRAMS) > +DUMP_SYMS_TARGETS := $(foreach file,$(DUMP_SYMS_TARGETS),$(or $(wildcard $(basename $(file)).pdb),$(file))) I don't know that the `$(wildcard)` here will work right--if the PDB file doesn't exist (because it's a clobber build) then it won't do the right thing. It's actually OK to pass a dll/exe to dump_syms instead of a pdb, so we can probably just do that instead. It might require tweaking symbolstore.py to know to munge the filename when it wants to copy+compress the PDB file. ::: toolkit/crashreporter/tools/symbolstore.py:580 (Diff revision 1) > def get_files_to_process(self, file_or_dir): > """Generate the files to process from an input.""" > if os.path.isdir(file_or_dir) and not self.ShouldSkipDir(file_or_dir): > for f in self.get_files_to_process_in_dir(file_or_dir): > yield f > - elif os.path.isfile(file_or_dir): > + elif os.path.isfile(file_or_dir) and self.ShouldProcess(os.path.abspath(file_or_dir)): Do we wind up passing files to the script that we don't actually want to dump symbols for?

Comment on attachment 8852228 [details] Bug 1337986 - Dump symbols during the compile tier. https://reviewboard.mozilla.org/r/124416/#review127252 So one thing I was thinking about but forgot to mention in my review--it would be good to preserve a simple way to invoke symbol dumping in developer builds, if only to make it easy to test this logic and test Breakpad updates etc. Having to set `MOZ_AUTOMATION=1` isn't the best since that has other effects on the build. Maybe we can configure things so that the syms targets get written out for every build, but they just aren't wired up to the compile dependency graph except in automation builds? Then you could make `buildsymbols` or `prepsymbolsarchive` or whatever depend on the `syms` target, which should be very quick to execute in automation (since all the files will be fresh) but would cause the actual symbol dumping work to happen for a developer build. ::: Makefile.in:256 (Diff revision 1) > ifdef MOZ_CRASHREPORTER > include $(topsrcdir)/toolkit/mozapps/installer/package-name.mk > > endif > > .PHONY: generatesymbols You renamed the target but not the `.PHONY` bit, FYI.

Comment on attachment 8852230 [details] Bug 1337986 - Remove dependencies between packaging steps and buildsymbols. https://reviewboard.mozilla.org/r/124420/#review127286 \o/

Comment on attachment 8852229 [details] Bug 1337986 - Remove code handling parallelism from symbolstore.py https://reviewboard.mozilla.org/r/124418/#review127256 This is good and useful, but I think we can go even farther here. We shouldn't need any of the code in `Process` or `get_files_to_process` if we're only taking a single file to dump on the commandline. Similarly, the `ProcessFilesWork` / `ProcessFilesFinished` stuff is leftover complexity from the multiprocess code--we should be able to simplify that all. Most of that exists because on OS X we have to run `dsymutil` before dumping symbols--maybe we should be doing that in the build backend instead? I'm not sure it would make much difference. In any event, I'm fine with landing this patch atop the other work, and if you don't want to rip everything out of symbolstore.py in one go at least file a followup to pare it down. ::: toolkit/crashreporter/tools/symbolstore.py:400 (Diff revision 1) > - cls.lock = manager.RLock() > - cls.srcdirRepoInfo = manager.dict() > - JobPool.init(executor(max_workers=num_cpus)) > - > def output(self, dest, output_str): > - """Writes |output_str| to |dest|, holding |lock|; > + """Writes |output_str| to |dest|, holding, terminates with a newline.""" As a second pass we could just replace all the `self.output` and `self.output_pid` with `print`.

Comment on attachment 8852230 [details] Bug 1337986 - Remove dependencies between packaging steps and buildsymbols. https://reviewboard.mozilla.org/r/124420/#review127302 ::: commit-message-5727f:3 (Diff revision 1) > +Bug 1337986 - Remove dependencies between packaging steps and buildsymbols. > + > +These dependecies are no longer relevant now that we're dumping symbols "dependencies" :)

Comment on attachment 8852228 [details] Bug 1337986 - Dump symbols during the compile tier. https://reviewboard.mozilla.org/r/124416/#review127252 Makes sense, will do.

Comment on attachment 8852228 [details] Bug 1337986 - Dump symbols during the compile tier. https://reviewboard.mozilla.org/r/124416/#review127216 > While you're here, what do you think about moving all this logic into symbolstore.py, just getting all of these defaults from `buildconfig` values, rather than passing everything down as an argument from Makefiles? If you think this patch series is complicated enough as-is then let's file a followup to do that. This sounds relatively straightforward, I'll look into it for this patch. > I've been thinking it would be nice to start removing content from rules.mk. Instead of doing this, what if instead we just wrote out thes rules in the recursive make backend directly into the Makefile? We could hide the logic behind a `py_action`, so the Makefile contents could be something simple like: > ``` > syms:: whatever.syms_track > whatever.syms_track: whatever > $(call py_action,dumpsymbols,$< $@) > ``` > > It seems like the logic would be easier to follow in Python than the hoops you have to jump through here in Make, at the very least. Yep, works for me! > So, as an alternate, possibly more involved proposal, we could make symbolstore simply put the `.sym` file and `.pd_` or whatever native debug symbol file in the current directory in the objdir, and then for the `buildsymbols` step instead of just running `zip`, write a small Python script that generates the zip by grabbing all the files from their objdir locations, reading the first line of the .sym file to figure out the right path inside the zip, and creating the zip file. We should have all the info we'd need in binaries.json. That would also let us tweak the compression per-file (since .sym files should be compressed but not the other files) which might speed up zip generation a little. > > What do you think? Avoiding wasted work when zipping things up sounds like a clear win, but this might be involved enough to warrant a follow-up. > Do we wind up passing files to the script that we don't actually want to dump symbols for? I believe this was an issue on the cross OS X builds, the clang-plugin library wasn't passing the ShouldProcess test there, I guess it calls itself a SHARED_LIBRARY but actually is built for the host.

Comment on attachment 8852225 [details] Bug 1337986 - Generate "syms" targets for directories containing programs or shared libraries. https://reviewboard.mozilla.org/r/124410/#review128808 ::: python/mozbuild/mozbuild/backend/recursivemake.py:739 (Diff revision 2) > > all_compile_deps = reduce(lambda x,y: x|y, > self._compile_graph.values()) if self._compile_graph else set() > compile_roots = set(self._compile_graph.keys()) - all_compile_deps > > + syms_targets = set(['%s/syms' % d for d in self._no_skip['syms']]) you can go with set('%s/syms' % d for d in ...) (without the intermediate list) ::: python/mozbuild/mozbuild/backend/recursivemake.py:740 (Diff revision 2) > + for d in self._no_skip['syms']: > + rule = root_deps_mk.create_rule(['%s/syms' % d]) > + rule.add_dependencies(['%s/target' % d]) You'd need less things written out if you added a syms_targets variable in root_mk (like compile_target), and added the following to config/recurse.mk: $(syms_targets): %/syms: %/target.

Comment on attachment 8852226 [details] Bug 1337986 - Build dump_syms before any syms target. https://reviewboard.mozilla.org/r/124412/#review128810 ::: config/recurse.mk:100 (Diff revision 2) > # nsinstall.py there. > ifdef COMPILE_ENVIRONMENT > ifneq (,$(filter config/host, $(compile_targets))) > $(addsuffix /$(CURRENT_TIER),$(CURRENT_DIRS)): config/host > endif > +# We need dump_syms during compile, so build it during export by making it depend The comment doesn't match what is actually done. ::: config/recurse.mk:103 (Diff revision 2) > $(addsuffix /$(CURRENT_TIER),$(CURRENT_DIRS)): config/host > endif > +# We need dump_syms during compile, so build it during export by making it depend > +# on an export target. > +ifneq (,$(filter toolkit/crashreporter/google-breakpad/src/tools/%/dump_syms/host,$(compile_targets))) > +$(addsuffix /$(CURRENT_TIER),$(lastword $(CURRENT_DIRS))): $(filter toolkit/crashreporter/google-breakpad/src/tools/%/dump_syms/host,$(compile_targets)) You should add those dependencies to the */syms targets only. Which also simplifies things since you don't need to filter out dump_syms/host.

Comment on attachment 8852227 [details] Bug 1337986 - Modify symbolstore.py to operate on dll/exe files. https://reviewboard.mozilla.org/r/124414/#review129008 ::: toolkit/crashreporter/tools/symbolstore.py:791 (Diff revision 2) > # Cache the corrected version to avoid future filesystem hits. > self.fixedFilenameCaseCache[file] = result > return result > > def CopyDebug(self, file, debug_file, guid, code_file, code_id): > + file = "%s.pdb" % os.path.splitext(file)[0] I would just write `+ '.pdb'`, but whatever. :) ::: toolkit/crashreporter/tools/symbolstore.py:840 (Diff revision 2) > else: > self.output(sys.stdout, rel_path) > > def SourceServerIndexing(self, debug_file, guid, sourceFileStream, vcs_root): > # Creates a .pdb.stream file in the mozilla\objdir to be used for source indexing > - debug_file = os.path.abspath(debug_file) > + debug_file = os.path.abspath("%s.pdb" % os.path.splitext(debug_file)[0]) If you change the call site of `SourceServerIndexing` to pass `debug_file` instead of `file` you'll already have the PDB file here. I guess it never mattered in the past because they were always the same.

Comment on attachment 8852228 [details] Bug 1337986 - Dump symbols during the compile tier. https://reviewboard.mozilla.org/r/124416/#review129014 This is great! Once we get this work landed I'd love to clean things up further so we're just calling into symbolstore.py as a Python module instead of doing all the commandline and environment wrangling you're forced to do here. ::: python/mozbuild/mozbuild/action/dumpsymbols.py:40 (Diff revision 2) > + if os_arch == 'WINNT': > + sym_store_args.extend(['-c', '--vcs-info']) > + if os.environ.get('PDBSTR_PATH'): > + sym_store_args.append('-i') > + if buildconfig.substs.get('MSVC_HAS_DIA_SDK'): > + dump_syms_bin = "%s.exe" % dump_syms_bin You could just tack on `substs['BIN_SUFFIX']` above. ::: python/mozbuild/mozbuild/action/dumpsymbols.py:44 (Diff revision 2) > + if buildconfig.substs.get('MSVC_HAS_DIA_SDK'): > + dump_syms_bin = "%s.exe" % dump_syms_bin > + else: > + dump_syms_bin = os.path.join(buildconfig.topsrcdir, 'toolkit', > + 'crashreporter', 'tools', 'win32', > + 'dump_syms_vc%s.exe' % buildconfig.substs['_MSC_VER']) You can actually just remove this, we don't support anything older than Visual C++ 2015 now and we don't have a dump_syms_vc1900.exe in-tree. ::: python/mozbuild/mozbuild/action/dumpsymbols.py:47 (Diff revision 2) > + dump_syms_bin = os.path.join(buildconfig.topsrcdir, 'toolkit', > + 'crashreporter', 'tools', 'win32', > + 'dump_syms_vc%s.exe' % buildconfig.substs['_MSC_VER']) > + elif os_arch == 'Darwin': > + sym_store_args.extend(['-c', '-a', buildconfig.substs['OS_TEST'], '--vcs-info']) > + elif os_arch in ('Linux', 'SunOS'): Drop the SunOS bit, we don't actually support Solaris anymore.

Comment on attachment 8854169 [details] Bug 1337986 - Convert unix paths to native paths for PDBSTR_PATH in mozharness. https://reviewboard.mozilla.org/r/126056/#review129020 I should just write a Rust version of pdbstr.exe at some point...

Comment on attachment 8852225 [details] Bug 1337986 - Generate "syms" targets for directories containing programs or shared libraries. https://reviewboard.mozilla.org/r/124410/#review129294 ::: config/recurse.mk:69 (Diff revision 3) > CURRENT_DIRS := $($(CURRENT_TIER)_dirs) > > # Need a list of compile targets because we can't use pattern rules: > # https://savannah.gnu.org/bugs/index.php?42833 > # Only recurse the paths starting with RECURSE_BASE_DIR when provided. > .PHONY: $(compile_targets) You should add $(syms_targets) to .PHONY too. ::: python/mozbuild/mozbuild/backend/recursivemake.py:741 (Diff revision 3) > + # Only hook symbols targets into the main compile graph in automation. > + if (self.environment.substs.get('MOZ_AUTOMATION') and > + self.environment.substs.get('MOZ_CRASHREPORTER')): > + compile_roots |= syms_targets Come to think of it, you could just do that in config/recurse.mk with: ifdef MOZ_AUTOMATION ifdef MOZ_CRASHREPORTER recurse_compile: $(syms_targets) endif endif As long as it's in the ifeq (.,$(DEPTH)) part, it's all good. ::: python/mozbuild/mozbuild/backend/recursivemake.py:766 (Diff revision 3) > + # Create a separate rule that depends on every 'syms' target so that > + # symbols can be dumped on demand locally. > + syms_rule = root_deps_mk.create_rule(['recurse_syms']) > + syms_rule.add_dependencies(['$(syms_targets)']) Same here, this could be done in recurse.mk.

Comment on attachment 8852226 [details] Bug 1337986 - Build dump_syms before any syms target. https://reviewboard.mozilla.org/r/124412/#review129296 ::: config/recurse.mk:77 (Diff revision 4) > > $(syms_targets): %/syms: %/target > > +# Ensure dump_syms gets built before any syms targets, all of which depend on it. > +ifneq (,$(filter toolkit/crashreporter/google-breakpad/src/tools/%/dump_syms/host,$(compile_targets))) > +$(syms_targets): $(filter toolkit/crashreporter/google-breakpad/src/tools/%/dump_syms/host,$(compile_targets)) With this, you're making all syms_targets depend on all compile_targets, which means dumping symbols won't actually happen until the very end of the compile tier. Use: $(syms_targets): %/syms: %/target

Comment on attachment 8852226 [details] Bug 1337986 - Build dump_syms before any syms target. https://reviewboard.mozilla.org/r/124412/#review129296 > With this, you're making all syms_targets depend on all compile_targets, which means dumping symbols won't actually happen until the very end of the compile tier. > > Use: > $(syms_targets): %/syms: %/target `$(syms_targets): %/syms: %/target` is in the prior commit. What I'm trying to do here is make every syms target depend on building dump_syms, I'm pretty sure that's what this is doing.

Mike, can you take another look at comment 46 based on comment 47? Thanks.

Err, I meant $(syms_targets): ..../dump_syms/host

(In reply to Mike Hommey [:glandium] from comment #49) > Err, I meant > > $(syms_targets): ..../dump_syms/host I don't see how this is going to work... the target name differs per-platform because of the directory layout of breakpad.

Comment on attachment 8852226 [details] Bug 1337986 - Build dump_syms before any syms target. https://reviewboard.mozilla.org/r/124412/#review129676 Doh, I misread the patch. Sorry.

Pushed by cmanchester@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/eb9139a092c0 Generate "syms" targets for directories containing programs or shared libraries. r=glandium https://hg.mozilla.org/integration/autoland/rev/e7a38558e8e2 Build dump_syms before any syms target. r=glandium https://hg.mozilla.org/integration/autoland/rev/dc1e93d94819 Modify symbolstore.py to operate on dll/exe files. r=ted https://hg.mozilla.org/integration/autoland/rev/17fc25af0768 Convert unix paths to native paths for PDBSTR_PATH in mozharness. r=ted https://hg.mozilla.org/integration/autoland/rev/994b97c9de37 Dump symbols during the compile tier. r=ted https://hg.mozilla.org/integration/autoland/rev/4b2125df04a8 Remove code handling parallelism from symbolstore.py r=ted https://hg.mozilla.org/integration/autoland/rev/7086da6987e0 Remove dependencies between packaging steps and buildsymbols. r=mshal,ted

https://hg.mozilla.org/mozilla-central/rev/eb9139a092c0 https://hg.mozilla.org/mozilla-central/rev/e7a38558e8e2 https://hg.mozilla.org/mozilla-central/rev/dc1e93d94819 https://hg.mozilla.org/mozilla-central/rev/17fc25af0768 https://hg.mozilla.org/mozilla-central/rev/994b97c9de37 https://hg.mozilla.org/mozilla-central/rev/4b2125df04a8 https://hg.mozilla.org/mozilla-central/rev/7086da6987e0