Bug 1557788 - Remove OBJS_VAR_SUFFIX & .i_o suffix for instrumented builds; r=firefox-build-system-reviewers,chmanchester

In 1-tier PGO builds that shared the objdir between the instrumented and
profile-use builds, the instrumented build objects used a different
suffix (.i_o) to separate them from the profile-use build (which uses
the default .o suffix). These builds are now always in separate objdirs,
and don't need special suffix rules anymore.

As a bonus, this helps fix an issue with buildid.cpp continually
rebuilding because libxul_so.list always lists the inputs as *.o, which
don't exist if we're using a .i_o suffix. Make would always re-create
buildid.cpp and therefore libxul.so in the instrumented build even when
nothing has changed.

Differential Revision: https://phabricator.services.mozilla.com/D56115

--HG--
extra : moz-landing-system : lando

config/config.mk

@@ -435,23 +435,6 @@ endif # ! WINNT
 # this file
 OBJ_SUFFIX := $(_OBJ_SUFFIX)
 
-OBJS_VAR_SUFFIX := OBJS
-
-# PGO builds with GCC and clang-cl build objects with instrumentation in
-# a first pass, then objects optimized, without instrumentation, in a
-# second pass. If we overwrite the objects from the first pass with
-# those from the second, we end up not getting instrumentation data for
-# better optimization on incremental builds. As a consequence, we use a
-# different object suffix for the first pass.
-ifdef MOZ_PROFILE_GENERATE
-ifneq (,$(GNU_CC)$(CLANG_CL))
-OBJS_VAR_SUFFIX := PGO_OBJS
-ifndef NO_PROFILE_GUIDED_OPTIMIZE
-OBJ_SUFFIX := i_o
-endif
-endif
-endif
-
 PLY_INCLUDE = -I$(MOZILLA_DIR)/other-licenses/ply
 
 # Enable verbose logs when not using `make -s`

config/rules.mk

@@ -462,7 +462,7 @@ $(PROGRAM): $(PROGOBJS) $(STATIC_LIBS) $(EXTRA_DEPS) $(RESFILE) $(GLOBAL_DEPS) $
 	$(REPORT_BUILD)
 	@$(RM) $@.manifest
 ifeq (_WINNT,$(GNU_CC)_$(OS_ARCH))
-	$(LINKER) -NOLOGO -OUT:$@ -PDB:$(LINK_PDBFILE) -IMPLIB:$(basename $(@F)).lib $(WIN32_EXE_LDFLAGS) $(LDFLAGS) $(MOZ_PROGRAM_LDFLAGS) $($(notdir $@)_$(OBJS_VAR_SUFFIX)) $(RESFILE) $(STATIC_LIBS) $(SHARED_LIBS) $(OS_LIBS)
+	$(LINKER) -NOLOGO -OUT:$@ -PDB:$(LINK_PDBFILE) -IMPLIB:$(basename $(@F)).lib $(WIN32_EXE_LDFLAGS) $(LDFLAGS) $(MOZ_PROGRAM_LDFLAGS) $($(notdir $@)_OBJS) $(RESFILE) $(STATIC_LIBS) $(SHARED_LIBS) $(OS_LIBS)
 ifdef MSMANIFEST_TOOL
 	@if test -f $@.manifest; then \
 		if test -f '$(srcdir)/$(notdir $@).manifest'; then \
@@ -478,7 +478,7 @@ ifdef MSMANIFEST_TOOL
 	fi
 endif	# MSVC with manifest tool
 else # !WINNT || GNU_CC
-	$(call EXPAND_CC_OR_CXX,$@) -o $@ $(COMPUTED_CXX_LDFLAGS) $(PGO_CFLAGS) $($(notdir $@)_$(OBJS_VAR_SUFFIX)) $(RESFILE) $(WIN32_EXE_LDFLAGS) $(LDFLAGS) $(STATIC_LIBS) $(MOZ_PROGRAM_LDFLAGS) $(SHARED_LIBS) $(OS_LIBS)
+	$(call EXPAND_CC_OR_CXX,$@) -o $@ $(COMPUTED_CXX_LDFLAGS) $(PGO_CFLAGS) $($(notdir $@)_OBJS) $(RESFILE) $(WIN32_EXE_LDFLAGS) $(LDFLAGS) $(STATIC_LIBS) $(MOZ_PROGRAM_LDFLAGS) $(SHARED_LIBS) $(OS_LIBS)
 	$(call py_action,check_binary,--target $@)
 endif # WINNT && !GNU_CC
 
@@ -529,7 +529,7 @@ endif
 $(SIMPLE_PROGRAMS): %$(BIN_SUFFIX): %.$(OBJ_SUFFIX) $(STATIC_LIBS) $(EXTRA_DEPS) $(GLOBAL_DEPS)
 	$(REPORT_BUILD)
 ifeq (_WINNT,$(GNU_CC)_$(OS_ARCH))
-	$(LINKER) -nologo -out:$@ -pdb:$(LINK_PDBFILE) $($@_$(OBJS_VAR_SUFFIX)) $(WIN32_EXE_LDFLAGS) $(LDFLAGS) $(MOZ_PROGRAM_LDFLAGS) $(STATIC_LIBS) $(SHARED_LIBS) $(OS_LIBS)
+	$(LINKER) -nologo -out:$@ -pdb:$(LINK_PDBFILE) $($@_OBJS) $(WIN32_EXE_LDFLAGS) $(LDFLAGS) $(MOZ_PROGRAM_LDFLAGS) $(STATIC_LIBS) $(SHARED_LIBS) $(OS_LIBS)
 ifdef MSMANIFEST_TOOL
 	@if test -f $@.manifest; then \
 		$(MT) -NOLOGO -MANIFEST $@.manifest -OUTPUTRESOURCE:$@\;1; \
@@ -539,7 +539,7 @@ ifdef MSMANIFEST_TOOL
 	fi
 endif	# MSVC with manifest tool
 else
-	$(call EXPAND_CC_OR_CXX,$@) $(COMPUTED_CXX_LDFLAGS) $(PGO_CFLAGS) -o $@ $($@_$(OBJS_VAR_SUFFIX)) $(WIN32_EXE_LDFLAGS) $(LDFLAGS) $(STATIC_LIBS) $(MOZ_PROGRAM_LDFLAGS) $(SHARED_LIBS) $(OS_LIBS)
+	$(call EXPAND_CC_OR_CXX,$@) $(COMPUTED_CXX_LDFLAGS) $(PGO_CFLAGS) -o $@ $($@_OBJS) $(WIN32_EXE_LDFLAGS) $(LDFLAGS) $(STATIC_LIBS) $(MOZ_PROGRAM_LDFLAGS) $(SHARED_LIBS) $(OS_LIBS)
 	$(call py_action,check_binary,--target $@)
 endif # WINNT && !GNU_CC
 
@@ -568,7 +568,7 @@ endif
 $(LIBRARY): $(OBJS) $(STATIC_LIBS) $(EXTRA_DEPS) $(GLOBAL_DEPS)
 	$(REPORT_BUILD)
 	$(RM) $(REAL_LIBRARY)
-	$(AR) $(AR_FLAGS) $($@_$(OBJS_VAR_SUFFIX))
+	$(AR) $(AR_FLAGS) $($@_OBJS)
 
 $(WASM_ARCHIVE): $(CWASMOBJS) $(CPPWASMOBJS) $(STATIC_LIBS) $(EXTRA_DEPS) $(GLOBAL_DEPS)
 	$(REPORT_BUILD_VERBOSE)
@@ -609,7 +609,7 @@ $(SHARED_LIBRARY): $(OBJS) $(RESFILE) $(STATIC_LIBS) $(EXTRA_DEPS) $(GLOBAL_DEPS
 ifndef INCREMENTAL_LINKER
 	$(RM) $@
 endif
-	$(MKSHLIB) $($@_$(OBJS_VAR_SUFFIX)) $(RESFILE) $(LDFLAGS) $(STATIC_LIBS) $(SHARED_LIBS) $(EXTRA_DSO_LDOPTS) $(MOZ_GLUE_LDFLAGS) $(OS_LIBS)
+	$(MKSHLIB) $($@_OBJS) $(RESFILE) $(LDFLAGS) $(STATIC_LIBS) $(SHARED_LIBS) $(EXTRA_DSO_LDOPTS) $(MOZ_GLUE_LDFLAGS) $(OS_LIBS)
 	$(call py_action,check_binary,--target $@)
 
 ifeq (_WINNT,$(GNU_CC)_$(OS_ARCH))

python/mozbuild/mozbuild/backend/common.py

@@ -230,7 +230,6 @@ class CommonBackend(BuildBackend):
         shared_libs = []
         static_libs = []
         objs = []
-        no_pgo_objs = []
 
         seen_objs = set()
         seen_libs = set()
@@ -242,11 +241,6 @@ class CommonBackend(BuildBackend):
 
                 seen_objs.add(o)
                 objs.append(o)
-                # This is slightly odd, but for consistency with the
-                # recursivemake backend we don't replace OBJ_SUFFIX if any
-                # object in a library has `no_pgo` set.
-                if lib.no_pgo_objs or lib.no_pgo:
-                    no_pgo_objs.append(o)
 
         def expand(lib, recurse_objs, system_libs):
             if isinstance(lib, (HostLibrary, StaticLibrary,
@@ -289,7 +283,7 @@ class CommonBackend(BuildBackend):
                 seen_libs.add(lib)
                 os_libs.append(lib)
 
-        return (objs, no_pgo_objs, shared_libs, os_libs, static_libs)
+        return (objs, shared_libs, os_libs, static_libs)
 
     def _make_list_file(self, kind, objdir, objs, name):
         if not objs:

python/mozbuild/mozbuild/backend/recursivemake.py

@@ -1345,43 +1345,14 @@ class RecursiveMakeBackend(MakeBackend):
             return os.path.normpath(mozpath.join(mozpath.relpath(lib.objdir, obj.objdir),
                                                  name))
 
-        objs, no_pgo_objs, shared_libs, os_libs, static_libs = self._expand_libs(obj)
+        objs, shared_libs, os_libs, static_libs = self._expand_libs(obj)
 
         obj_target = obj.name
         if isinstance(obj, Program):
             obj_target = self._pretty_path(obj.output_path, backend_file)
 
-        profile_gen_objs = []
-
-        if obj.KIND == 'target':
-            is_unit_test = isinstance(obj, BaseProgram) and obj.is_unit_test
-
-            doing_pgo = self.environment.substs.get('MOZ_PGO')
-            obj_suffix_change_needed = (self.environment.substs.get('GNU_CC') or
-                                        self.environment.substs.get('CLANG_CL'))
-            if doing_pgo and obj_suffix_change_needed:
-                # We use a different OBJ_SUFFIX for the profile generate phase on
-                # systems where the pgo generate phase requires instrumentation
-                # that can only be removed by recompiling objects. These get
-                # picked up via OBJS_VAR_SUFFIX in config.mk.
-                if not is_unit_test and not isinstance(obj, SimpleProgram):
-                    profile_gen_objs = [o if o in no_pgo_objs else '%s.%s' %
-                                        (mozpath.splitext(o)[0], 'i_o') for o in objs]
-
-        def write_obj_deps(target, objs_ref, pgo_objs_ref):
-            if pgo_objs_ref:
-                backend_file.write('ifdef MOZ_PROFILE_GENERATE\n')
-                backend_file.write('%s: %s\n' % (target, pgo_objs_ref))
-                backend_file.write('else\n')
-                backend_file.write('%s: %s\n' % (target, objs_ref))
-                backend_file.write('endif\n')
-            else:
-                backend_file.write('%s: %s\n' % (target, objs_ref))
-
         objs_ref = ' \\\n    '.join(os.path.relpath(o, obj.objdir)
                                     for o in objs)
-        pgo_objs_ref = ' \\\n    '.join(os.path.relpath(o, obj.objdir)
-                                        for o in profile_gen_objs)
         # Don't bother with a list file if we're only linking objects built
         # in this directory or building a real static library. This
         # accommodates clang-plugin, where we would otherwise pass an
@@ -1393,10 +1364,7 @@ class RecursiveMakeBackend(MakeBackend):
             obj.no_expand_lib):
             backend_file.write_once('%s_OBJS := %s\n' % (obj.name,
                                                          objs_ref))
-            if profile_gen_objs:
+            backend_file.write('%s: %s\n' % (obj_target, objs_ref))
-                backend_file.write_once('%s_PGO_OBJS := %s\n' % (obj.name,
-                                                                 pgo_objs_ref))
-            write_obj_deps(obj_target, objs_ref, pgo_objs_ref)
         elif not isinstance(obj, (HostLibrary, StaticLibrary,
                                   SandboxedWasmLibrary)):
             list_file_path = '%s.list' % obj.name.replace('.', '_')
@@ -1405,16 +1373,7 @@ class RecursiveMakeBackend(MakeBackend):
             backend_file.write_once('%s_OBJS := %s\n' %
                                     (obj.name, list_file_ref))
             backend_file.write_once('%s: %s\n' % (obj_target, list_file_path))
-            if profile_gen_objs:
+            backend_file.write('%s: %s\n' % (obj_target, objs_ref))
-                pgo_list_file_path = '%s_pgo.list' % obj.name.replace('.', '_')
-                pgo_list_file_ref = self._make_list_file(obj.KIND, obj.objdir,
-                                                         profile_gen_objs,
-                                                         pgo_list_file_path)
-                backend_file.write_once('%s_PGO_OBJS := %s\n' %
-                                        (obj.name, pgo_list_file_ref))
-                backend_file.write_once('%s: %s\n' % (obj_target,
-                                                      pgo_list_file_path))
-            write_obj_deps(obj_target, objs_ref, pgo_objs_ref)
 
         for lib in shared_libs:
             assert obj.KIND != 'host'

python/mozbuild/mozbuild/backend/tup.py

@@ -416,7 +416,7 @@ class TupBackend(CommonBackend):
             ['-o', shlib.lib_name]
         )
 
-        objs, _, shared_libs, os_libs, static_libs = self._expand_libs(shlib)
+        objs, shared_libs, os_libs, static_libs = self._expand_libs(shlib)
         static_libs = self._lib_paths(backend_file.objdir, static_libs)
         shared_libs = self._lib_paths(backend_file.objdir, shared_libs)
 
@@ -470,7 +470,7 @@ class TupBackend(CommonBackend):
 
     def _gen_program(self, backend_file, prog):
         cc_or_cxx = 'CXX' if prog.cxx_link else 'CC'
-        objs, _, shared_libs, os_libs, static_libs = self._expand_libs(prog)
+        objs, shared_libs, os_libs, static_libs = self._expand_libs(prog)
 
         static_libs = self._lib_paths(backend_file.objdir, static_libs)
         shared_libs = self._lib_paths(backend_file.objdir, shared_libs)
@@ -522,7 +522,7 @@ class TupBackend(CommonBackend):
             self._gen_host_program(backend_file, p)
 
     def _gen_host_program(self, backend_file, prog):
-        _, _, _, extra_libs, _ = self._expand_libs(prog)
+        _, _, extra_libs, _ = self._expand_libs(prog)
         objs = prog.objs
 
         if isinstance(prog, HostSimpleProgram):
@@ -567,7 +567,7 @@ class TupBackend(CommonBackend):
             backend_file.environment.substs['AR_FLAGS'].replace('$@', '%o')
         ]
 
-        objs, _, shared_libs, _, static_libs = self._expand_libs(backend_file.static_lib)
+        objs, shared_libs, _, static_libs = self._expand_libs(backend_file.static_lib)
         static_libs = self._lib_paths(backend_file.objdir, static_libs)
         shared_libs = self._lib_paths(backend_file.objdir, shared_libs)
 

python/mozbuild/mozbuild/frontend/data.py

@@ -406,8 +406,6 @@ class Linkable(ContextDerived):
         'lib_defines',
         'linked_libraries',
         'linked_system_libs',
-        'no_pgo_sources',
-        'no_pgo',
         'sources',
     )
 
@@ -418,8 +416,6 @@ class Linkable(ContextDerived):
         self.linked_system_libs = []
         self.lib_defines = Defines(context, {})
         self.sources = defaultdict(list)
-        self.no_pgo_sources = []
-        self.no_pgo = False
 
     def link_library(self, obj):
         assert isinstance(obj, BaseLibrary)
@@ -468,10 +464,6 @@ class Linkable(ContextDerived):
         """Can be overridden by a base class for custom behavior."""
         return self.config.substs.get('OBJ_SUFFIX', '')
 
-    @property
-    def no_pgo_objs(self):
-        return self._get_objs(self.no_pgo_sources)
-
     @property
     def objs(self):
         return self._get_objs(self.source_files())

python/mozbuild/mozbuild/frontend/emitter.py

@@ -1032,10 +1032,6 @@ class TreeMetadataEmitter(LoggingMixin):
                 for target_var in ('SOURCES', 'UNIFIED_SOURCES'):
                     for suffix, srcs in ctxt_sources[target_var].items():
                         linkable.sources[suffix] += srcs
-                if no_pgo_sources:
-                    linkable.no_pgo_sources = no_pgo_sources
-                elif no_pgo:
-                    linkable.no_pgo = True
             for host_linkable in host_linkables:
                 for suffix, srcs in ctxt_sources['HOST_SOURCES'].items():
                     host_linkable.sources[suffix] += srcs