Closed Bug 1319222 Opened 4 years ago Closed 3 years ago

Support compilation objects in the tup backend

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: mshal, Assigned: mshal)

References

Details

Attachments

(10 files)

Bug 1319222 - Remove srcdir from BackendTupfile;
59 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1319222 - Use relobjdir instead of relativedir to determine outputs;
59 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1319222 - Change --param flag to be multiple arguments;
59 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1319222 - Add an SFLAGS ComputedFlags variable for compiling *.S;
59 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1319222 - Compile the generated IPDL and WebIDL sources in the tup backend;
59 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1319222 - Enable compilation on all directories in the tup backend;
59 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1319222 - Add include flags to HOST_*;
59 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1319222 - Enable host compilation in the tup backend;
59 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1319222 - Support compiling *.s with yasm;
59 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1319222 - Remove INCLUDES from HOST_SIMPLE_PROGRAMS linking;
59 bytes, text/x-review-board-request
chmanchester
: review+
Details 
We'll need to compile all of the *Sources moz.build variables in the tup backend. I plan to share most of the CompilationDB command-line generation logic in the CommonBackend, and use that for the tup backend.
Blocks: 1319224
Depends on: 1319225
Depends on: 1319563
Depends on: 1416062
Depends on: 1417658
Assignee: nobody → mshal
With this set of patches, a build with the tup backend build will generate the same set of files as a recursive make build with an empty mozconfig. I haven't yet verified that the object files are identical, but progress! And actually linking them will be handled in other bugs.

I requested chmanchester for the flags patches since I'm really not sure if those are implemented correctly. They seemed to be required for host and asm compilation in a few places.
Attachment #8931140 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8931141 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8931142 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8931144 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8931145 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8931147 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8931148 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Comment on attachment 8931140 [details]
Bug 1319222 - Remove srcdir from BackendTupfile;

https://reviewboard.mozilla.org/r/202222/#review207582
Attachment #8931140 - Flags: review?(cmanchester) → review+
Comment on attachment 8931141 [details]
Bug 1319222 - Use relobjdir instead of relativedir to determine outputs;

https://reviewboard.mozilla.org/r/202224/#review207584

::: python/mozbuild/mozbuild/backend/tup.py:207
(Diff revision 1)
>                      BackendTupfile(objdir, self.environment,
>                                     self.environment.topsrcdir, self.environment.topobjdir)
>          return self._backend_files[objdir]
>  
>      def _get_backend_file_for(self, obj):
> -        return self._get_backend_file(obj.relativedir)
> +        return self._get_backend_file(obj.relobjdir)

Wow, this is sort of a footgun. I would say let's re-name `relativedir` to `relsrcdir`... in the constructor for ContextDerived we actually do `self.relativedir = context.relsrcdir`. If you'd rather not fix that here let's do it in a follow up.

I guess this explains all the times I've failed trying to find `relsrcdir` on an object in the build system.
Attachment #8931141 - Flags: review?(cmanchester) → review+
Comment on attachment 8931142 [details]
Bug 1319222 - Change --param flag to be multiple arguments;

https://reviewboard.mozilla.org/r/202226/#review207586
Attachment #8931142 - Flags: review?(cmanchester) → review+
Comment on attachment 8931143 [details]
Bug 1319222 - Add an SFLAGS ComputedFlags variable for compiling *.S;

https://reviewboard.mozilla.org/r/202228/#review207590

::: python/mozbuild/mozbuild/frontend/context.py:353
(Diff revision 1)
> +            ('DEFINES', None, ('ASFLAGS',)),
> +            ('LIBRARY_DEFINES', None, ('ASFLAGS',)),
> +            ('LOCAL_INCLUDES', None, ('ASFLAGS',)),

There are two potential issues here:

DEFINES and INCLUDES are added to the command line with ASFLAGS for SOBJS (made from `*.S` files), but not for ASOBJS (made from `*.s` files). Also, by adding them here but not deleting them from the rules.mk command line we'd end up with them duplicated in the Make backend. Probably we can split these off into a separate variable if this distinction is really significant.

The second potential issue is that in the Make backend DEFINES come before ASFLAGS, followed by per-source flags, followed by INCLUDES. Changing the order could be significant. I doubt it is, but we would want to check. If we want to preserve the order we could probably just cobble this together in the backend.

::: python/mozbuild/mozbuild/frontend/emitter.py:1237
(Diff revision 1)
>          if passthru.variables:
>              yield passthru
>  
>          if context.objdir in self._compile_dirs:
>              self._compile_flags[context.objdir] = computed_flags
> +            self._compile_as_flags[context.objdir] = computed_as_flags

This would make `_asm_compile_dirs` obsolete. Was that too restrictive for some reason?
Attachment #8931143 - Flags: review?(cmanchester)
Comment on attachment 8931144 [details]
Bug 1319222 - Compile the generated IPDL and WebIDL sources in the tup backend;

https://reviewboard.mozilla.org/r/202230/#review207592

::: python/mozbuild/mozbuild/backend/tup.py:569
(Diff revision 1)
>              cmd=cmd,
>              outputs=outputs,
>              extra_outputs=[self._installed_files],
>              check_unchanged=True,
>          )
> +        backend_file.sources['.cpp'].extend([u[0] for u in unified_ipdl_cppsrcs_mapping])

Intermediate list not needed?

::: python/mozbuild/mozbuild/backend/tup.py:602
(Diff revision 1)
>              inputs=webidls.all_non_static_basenames(),
>              outputs=outputs,
>              extra_outputs=[self._installed_files],
>              check_unchanged=True,
>          )
> +        backend_file.sources['.cpp'].extend([u[0] for u in unified_source_mapping])

Intermediate list not needed?
Attachment #8931144 - Flags: review?(cmanchester) → review+
Comment on attachment 8931145 [details]
Bug 1319222 - Enable compilation on all directories in the tup backend;

https://reviewboard.mozilla.org/r/202232/#review207594
Attachment #8931145 - Flags: review?(cmanchester) → review+
Comment on attachment 8931146 [details]
Bug 1319222 - Add include flags to HOST_*;

https://reviewboard.mozilla.org/r/202234/#review207600

::: python/mozbuild/mozbuild/frontend/context.py:332
(Diff revision 1)
> +            ('BASE_INCLUDES', ['-I%s' % main_src_dir, '-I%s' % context.objdir],
> +             ('HOST_CFLAGS', 'HOST_CXXFLAGS')),
> +            ('LOCAL_INCLUDES', None, ('HOST_CFLAGS', 'HOST_CXXFLAGS')),
> +            ('EXTRA_INCLUDES', ['-I%s/dist/include' % context.config.topobjdir],
> +             ('HOST_CFLAGS', 'HOST_CXXFLAGS')),

This has a sort of similar issue to `ASFLAGS`: the includes aren't passed every time we see `HOST_CFLAGS`, and we'd have to delete these from `rules.mk` if we were to add them here. Again we probably need to shunt these off to a separate variable or just stitch things together in the tup backend for now.
Attachment #8931146 - Flags: review?(cmanchester)
Comment on attachment 8931147 [details]
Bug 1319222 - Enable host compilation in the tup backend;

https://reviewboard.mozilla.org/r/202236/#review207602

::: python/mozbuild/mozbuild/backend/tup.py:122
(Diff revision 1)
> +        s = self.sources
> +        h = self.host_sources

Nit: one char variables are hard to grep for.

::: python/mozbuild/mozbuild/backend/tup.py:125
(Diff revision 1)
> -            ('.S', 'AS', 'ASFLAGS'),
> -            ('.cpp', 'CXX', 'CXXFLAGS'),
> -            ('.c', 'CC', 'CFLAGS'),
> +            (s['.S'], 'AS', 'ASFLAGS', ''),
> +            (s['.cpp'], 'CXX', 'CXXFLAGS', ''),
> +            (s['.c'], 'CC', 'CFLAGS', ''),
> +            (h['.cpp'], 'HOST_CXX', 'HOST_CXXFLAGS', 'host_'),
> +            (h['.c'], 'HOST_CC', 'HOST_CFLAGS', 'host_'),

I just noticed the recursivemake backend cares about the difference between `.s` and `.S`. Sorry, I probably should have caught this is a prior review :/
Attachment #8931147 - Flags: review?(cmanchester) → review+
Comment on attachment 8931147 [details]
Bug 1319222 - Enable host compilation in the tup backend;

https://reviewboard.mozilla.org/r/202236/#review207602

> I just noticed the recursivemake backend cares about the difference between `.s` and `.S`. Sorry, I probably should have caught this is a prior review :/

Oh, you take care of this in the next patch. Nevermind!
Comment on attachment 8931148 [details]
Bug 1319222 - Support compiling *.s with yasm;

https://reviewboard.mozilla.org/r/202238/#review207606

::: python/mozbuild/mozbuild/backend/tup.py:142
(Diff revision 1)
> -                cmd.extend(['-c', '%f', '-o', '%o'])
> +                if compiler == 'AS':
> +                    cmd.append(self.variables.get('AS_DASH_C_FLAG', '-c'))
> +                else:
> +                    cmd.append('-c')

Can this whole thing just be `if compile_value != self.environment.substs['YASM']: cmd.append('-c')`? 

That way we can avoid a reference to `AS_DASH_C_FLAG`.
Attachment #8931148 - Flags: review?(cmanchester) → review+
Comment on attachment 8931141 [details]
Bug 1319222 - Use relobjdir instead of relativedir to determine outputs;

https://reviewboard.mozilla.org/r/202224/#review207584

> Wow, this is sort of a footgun. I would say let's re-name `relativedir` to `relsrcdir`... in the constructor for ContextDerived we actually do `self.relativedir = context.relsrcdir`. If you'd rather not fix that here let's do it in a follow up.
> 
> I guess this explains all the times I've failed trying to find `relsrcdir` on an object in the build system.

Filed bug 1421038 as a followup.
Comment on attachment 8931148 [details]
Bug 1319222 - Support compiling *.s with yasm;

https://reviewboard.mozilla.org/r/202238/#review207606

> Can this whole thing just be `if compile_value != self.environment.substs['YASM']: cmd.append('-c')`? 
> 
> That way we can avoid a reference to `AS_DASH_C_FLAG`.

That does seem to work, though that makes this logic a little redundant with how AS_DASH_C_FLAG gets set in the emitter. I think it might be confusing to have that in the emitter and go unused in the tup backend.

I've re-worked it slightly to specify the dash_c setting in the compilers list so it no longer uses the weird if-statement. I'll ask for re-review on this one.
Comment on attachment 8931143 [details]
Bug 1319222 - Add an SFLAGS ComputedFlags variable for compiling *.S;

https://reviewboard.mozilla.org/r/202228/#review207590

> There are two potential issues here:
> 
> DEFINES and INCLUDES are added to the command line with ASFLAGS for SOBJS (made from `*.S` files), but not for ASOBJS (made from `*.s` files). Also, by adding them here but not deleting them from the rules.mk command line we'd end up with them duplicated in the Make backend. Probably we can split these off into a separate variable if this distinction is really significant.
> 
> The second potential issue is that in the Make backend DEFINES come before ASFLAGS, followed by per-source flags, followed by INCLUDES. Changing the order could be significant. I doubt it is, but we would want to check. If we want to preserve the order we could probably just cobble this together in the backend.

Ahh, good catch. I agree it's probably easiest to separate the flags for SOBJS out into a separate variable. I've re-worked this commit to go with that approach, and verified locally that the before & after commandlines in the make backend are identical for a .s and a .S file.

> This would make `_asm_compile_dirs` obsolete. Was that too restrictive for some reason?

I think this issue is no longer relevant with the new approach, since the original AsmFlags are the same as before. Let me know if I'm misunderstanding.
Comment on attachment 8931143 [details]
Bug 1319222 - Add an SFLAGS ComputedFlags variable for compiling *.S;

https://reviewboard.mozilla.org/r/202228/#review209980

Looks good, thanks for doing this!

::: python/mozbuild/mozbuild/frontend/context.py:2034
(Diff revision 2)
> -        """Recipe for linker flags for this context. Not to be manipulated
> -        directly.
> +        """Recipe for assembler flags for this context, without define or
> +        include flags since they are not supported by some assemblers.  Not to

This comment isn't quite right now (I don't think it needs to be updated). The `ASM_FLAGS` dict will have everything but decide which flags categories to pass on to `ASFLAGS` vs `SFLAGS` based on the `flag_variables` definition above.

::: python/mozbuild/mozbuild/frontend/emitter.py
(Diff revision 2)
>              passthru.variables['AS'] = yasm
>              passthru.variables['AS_DASH_C_FLAG'] = ''
>              computed_as_flags.resolve_flags('OS',
>                                              context.config.substs.get('YASM_ASFLAGS', []))
>  
> -

Stray line deletion.
Attachment #8931143 - Flags: review?(cmanchester) → review+
Comment on attachment 8931146 [details]
Bug 1319222 - Add include flags to HOST_*;

https://reviewboard.mozilla.org/r/202234/#review209990

::: config/config.mk:212
(Diff revision 2)
> +HOST_LINK_CFLAGS = $(COMPUTED_HOST_LINK_CFLAGS)
> +HOST_LINK_CXXFLAGS = $(COMPUTED_HOST_LINK_CXXFLAGS)

For `CompileFlags` we have `C{XX}_LDFLAGS` for the ldflags we also pass to the linked. I guess I'll say let's keep that naming convention, although it's not a perfect fit because for whatever reason we pass includes to link `HOST_SIMPLE_PROGRAMS`. Let's just add a comment or something to the commit message acknowledging the difference between the `HOST_SIMPLE_PROGRAM` and `HOST_PROGRAM` command lines.
Attachment #8931146 - Flags: review?(cmanchester) → review+
Comment on attachment 8931143 [details]
Bug 1319222 - Add an SFLAGS ComputedFlags variable for compiling *.S;

https://reviewboard.mozilla.org/r/202228/#review209980

> Stray line deletion.

Whoops, fixed!
Comment on attachment 8931146 [details]
Bug 1319222 - Add include flags to HOST_*;

https://reviewboard.mozilla.org/r/202234/#review209990

> For `CompileFlags` we have `C{XX}_LDFLAGS` for the ldflags we also pass to the linked. I guess I'll say let's keep that naming convention, although it's not a perfect fit because for whatever reason we pass includes to link `HOST_SIMPLE_PROGRAMS`. Let's just add a comment or something to the commit message acknowledging the difference between the `HOST_SIMPLE_PROGRAM` and `HOST_PROGRAM` command lines.

Ok, the name is updated.

As for the differences in flag usage between HOST_SIMPLE_PROGRAM and HOST_PROGRAM, it doesn't look like we need INCLUDES or DEFINES in the linker rule for HOST_SIMPLE_PROGRAM anymore (it's only used in layout/style/test for host_ListCSSProperties, and there is a separate compiler step anyway). How about just changing the HOST_SIMPLE_PROGRAM flags to use HOST_CXX_LDFLAGS instead of HOST_CXXFLAGS?
Flags: needinfo?(cmanchester)
Comment on attachment 8931146 [details]
Bug 1319222 - Add include flags to HOST_*;

https://reviewboard.mozilla.org/r/202234/#review209990

> Ok, the name is updated.
> 
> As for the differences in flag usage between HOST_SIMPLE_PROGRAM and HOST_PROGRAM, it doesn't look like we need INCLUDES or DEFINES in the linker rule for HOST_SIMPLE_PROGRAM anymore (it's only used in layout/style/test for host_ListCSSProperties, and there is a separate compiler step anyway). How about just changing the HOST_SIMPLE_PROGRAM flags to use HOST_CXX_LDFLAGS instead of HOST_CXXFLAGS?

By all means, if the difference isn't needed anymore let's get rid of it! Let's add this as a "pre" patch.
Replied in mozreview.
Flags: needinfo?(cmanchester)
Comment on attachment 8934596 [details]
Bug 1319222 - Remove INCLUDES from HOST_SIMPLE_PROGRAMS linking;

https://reviewboard.mozilla.org/r/205482/#review211140
Attachment #8934596 - Flags: review?(cmanchester) → review+
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05b9eb8ae66b
Remove srcdir from BackendTupfile; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/1c5d8c7e5ddd
Use relobjdir instead of relativedir to determine outputs; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/598cfad59614
Change --param flag to be multiple arguments; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/8e89b51b7f40
Add an SFLAGS ComputedFlags variable for compiling *.S; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/a35541618d92
Compile the generated IPDL and WebIDL sources in the tup backend; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/d115dc76ef73
Remove INCLUDES from HOST_SIMPLE_PROGRAMS linking; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/8b0115b2fed8
Add include flags to HOST_*; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/b6d367955ce6
Enable host compilation in the tup backend; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/7f12adb5e683
Support compiling *.s with yasm; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/a3160ab7a6bf
Enable compilation on all directories in the tup backend; r=chmanchester
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.