978594 - Port the per-source flags to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[(no longer active)]

Attachment: Port the per-source flags to moz.build

Comment 2

[(no longer active)]

Comment on attachment 8384304 [details] [diff] [review]
Port the per-source flags to moz.build

This seems to mostly work except for one gotcha.  If I use $(OBJ_SUFFIX) in backend.mk, I get bitten by http://mxr.mozilla.org/mozilla-central/source/config/baseconfig.mk#25.  For now I've used $(_OBJ_SUFFIX) to get away with this but obviously this will break Linux PGO builds.

glandium, any idea what's the best way to solve this?  The code in config/config.mk which sets OBJ_SUFFIX can't be reordered since it depends on NO_PROFILE_GUIDED_OPTIMIZE which is defined in backend.mk.  One possible approach would be to write this to a new file (postbackend.mk?) which will be included after that point in config.mk, but I'm not sure if that is the best solution.  Another trick which seems to work is to generate something:

define foo
Source.$(OBJ_SUFFIX): MOZBUILD_CXXFLAGS += -mfoo
endef

And then add $(foo) to config.mk after we set OBJ_SUFFIX.

Also, general feedback on the rest of the patch is appreciated.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8384304 [details] [diff] [review]
Port the per-source flags to moz.build

Review of attachment 8384304 [details] [diff] [review]:
-----------------------------------------------------------------

A simpler way to do this, and that solves both the backend.mk ordering problem and the fact that the current system is also broken in that the flags are not even used when building .s and .i, is to define per-file variables, such as foo_c_CFLAGS, and add them to the corresponding build rules for objects, assembly and preprocessed files, by deriving from $@, like $($(subst .,_,$(notdir $@))_CFLAGS). I even think the subst is not necessary, as make supports variable names with dots in them (although i don't know if pymake and mac make does)

Comment 4

[(no longer active)]

Attachment: Port the per-source flags to moz.build

Comment 5

[(no longer active)]

Comment on attachment 8385064 [details] [diff] [review]
Port the per-source flags to moz.build

This idea seems to work very well.  I ended up using @< instead because that way I can just use the source file name directly.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8385064 [details] [diff] [review]
Port the per-source flags to moz.build

Review of attachment 8385064 [details] [diff] [review]:
-----------------------------------------------------------------

Please land the Makefile.in/moz.build changes separately from the build system changes to support them.

::: config/rules.mk
@@ +971,5 @@
>  
>  $(COBJS):
>  	$(REPORT_BUILD)
>  	@$(MAKE_DEPS_AUTO_CC)
> +	$(ELOG) $(CC) $(OUTOPTION)$@ -c $(COMPILE_CFLAGS) $($(notdir $<)_FLAGS) $(TARGET_LOCAL_INCLUDES) $(_VPATH_SRCS)

If your concern with bug 979108 is that this rules.mk change doesn't trigger a rebuild, you can move the flags to COMPILE_*FLAGS in config.mk. That would also make the changes lighter, as that would impact the .s and .i rules at the same time.

::: gfx/cairo/libpixman/src/Makefile.in
@@ -14,5 @@
> -ifneq (,$(filter 1400 1500, $(_MSC_VER)))
> -# MSVC 2005 and 2008 generate code that breaks alignment
> -# restrictions in debug mode so always optimize.
> -# See bug 640250 for more info.
> -SSE2_CFLAGS=-O2

For clarity, can you remove this old cruft in a separate changeset when landing?

::: gfx/cairo/libpixman/src/moz.build
@@ +128,5 @@
>  
>  if use_mmx:
>      DEFINES['USE_MMX'] = True
>      SOURCES += ['pixman-mmx.c']
> +    SOURCES['pixman-mmx.c'].flags = mmx_flags

Not sure there's much value in those intermediate variables.

::: gfx/ycbcr/moz.build
@@ +26,5 @@
> +    SOURCES += ['yuv_convert_sse2.cpp']
> +    if CONFIG['GNU_CC']:
> +        SOURCES['yuv_convert_sse2.cpp'].flags += ['-msse2']
> +    if CONFIG['SOLARIS_SUNPRO_CXX']:
> +        SOURCES['yuv_convert_sse2.cpp'].flags += ['-xarch=sse2', '-xO4']

sounds like we should have a CONFIG['MMX_FLAGS'] and CONFIG['SSE2_FLAGS']. Followup?

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +313,5 @@
> +            '.c': 'CFLAGS',
> +            '.cpp': 'CXXFLAGS',
> +            '.cc': 'CXXFLAGS',
> +            '.m': 'CMFLAGS',
> +            '.mm': 'CMMFLAGS',

You're not using the variable names anymore, you can make source_flags a set of extensions instead, but...

@@ +319,5 @@
> +        sources_with_flags = [f for f in sources if sources[f].flags]
> +        for f in sources_with_flags:
> +            ext = mozpath.splitext(f)[1]
> +            if ext not in source_flags:
> +                raise SandboxValidationError('Per source flags are not supported for %s files' % ext)

You might as well add assembly files to the mix. I'm sure we have at least one per-file ASFLAGS adjustment around, which then makes all source types support flags, in which case you don't need to care about that at all.

Comment 7

[(no longer active)]

(In reply to Mike Hommey [:glandium] from comment #6)
> Please land the Makefile.in/moz.build changes separately from the build
> system changes to support them.

OK, will do.

> ::: config/rules.mk
> @@ +971,5 @@
> >  
> >  $(COBJS):
> >  	$(REPORT_BUILD)
> >  	@$(MAKE_DEPS_AUTO_CC)
> > +	$(ELOG) $(CC) $(OUTOPTION)$@ -c $(COMPILE_CFLAGS) $($(notdir $<)_FLAGS) $(TARGET_LOCAL_INCLUDES) $(_VPATH_SRCS)
> 
> If your concern with bug 979108 is that this rules.mk change doesn't trigger
> a rebuild, you can move the flags to COMPILE_*FLAGS in config.mk. That would
> also make the changes lighter, as that would impact the .s and .i rules at
> the same time.

I did things this way on purpose.  Doing this in config.mk would mess up the ordering of these flags for Objective-C++ code, since I'd have to add those flags at the end of COMPILE_CMMFLAGS which means that stuff in MOZBUILD_CMMFLAGS would potentially override them.  Need-info?ing on this mostly.

> ::: gfx/cairo/libpixman/src/Makefile.in
> @@ -14,5 @@
> > -ifneq (,$(filter 1400 1500, $(_MSC_VER)))
> > -# MSVC 2005 and 2008 generate code that breaks alignment
> > -# restrictions in debug mode so always optimize.
> > -# See bug 640250 for more info.
> > -SSE2_CFLAGS=-O2
> 
> For clarity, can you remove this old cruft in a separate changeset when
> landing?

https://hg.mozilla.org/integration/mozilla-inbound/rev/de9872bce666

> ::: gfx/cairo/libpixman/src/moz.build
> @@ +128,5 @@
> >  
> >  if use_mmx:
> >      DEFINES['USE_MMX'] = True
> >      SOURCES += ['pixman-mmx.c']
> > +    SOURCES['pixman-mmx.c'].flags = mmx_flags
> 
> Not sure there's much value in those intermediate variables.

I did that so that those flags and use_mmx etc. are in the same place.  Please confirm if you want me to change that?

> ::: gfx/ycbcr/moz.build
> @@ +26,5 @@
> > +    SOURCES += ['yuv_convert_sse2.cpp']
> > +    if CONFIG['GNU_CC']:
> > +        SOURCES['yuv_convert_sse2.cpp'].flags += ['-msse2']
> > +    if CONFIG['SOLARIS_SUNPRO_CXX']:
> > +        SOURCES['yuv_convert_sse2.cpp'].flags += ['-xarch=sse2', '-xO4']
> 
> sounds like we should have a CONFIG['MMX_FLAGS'] and CONFIG['SSE2_FLAGS'].
> Followup?

Sure.

> ::: python/mozbuild/mozbuild/frontend/emitter.py
> @@ +313,5 @@
> > +            '.c': 'CFLAGS',
> > +            '.cpp': 'CXXFLAGS',
> > +            '.cc': 'CXXFLAGS',
> > +            '.m': 'CMFLAGS',
> > +            '.mm': 'CMMFLAGS',
> 
> You're not using the variable names anymore, you can make source_flags a set
> of extensions instead, but...

Ah right.

> @@ +319,5 @@
> > +        sources_with_flags = [f for f in sources if sources[f].flags]
> > +        for f in sources_with_flags:
> > +            ext = mozpath.splitext(f)[1]
> > +            if ext not in source_flags:
> > +                raise SandboxValidationError('Per source flags are not supported for %s files' % ext)
> 
> You might as well add assembly files to the mix. I'm sure we have at least
> one per-file ASFLAGS adjustment around, which then makes all source types
> support flags, in which case you don't need to care about that at all.

We don't, I already checked.

$ git grep -w ASFLAGS | grep Makefile.in
media/libjpeg/Makefile.in:8:ASFLAGS=$(LIBJPEG_TURBO_ASFLAGS) -I$(topsrcdir)/media/libjpeg/simd/
media/libtheora/lib/Makefile.in:11:ASFLAGS = -march=armv7-a -mfpu=neon
media/libvpx/Makefile.in:7:ASFLAGS=$(VPX_ASFLAGS) -I. -I$(topsrcdir)/media/libvpx/ -I$(topsrcdir)/media/libvpx/vpx_ports/
xpcom/reflect/xptcall/src/md/unix/Makefile.in:40:ASFLAGS		+= -I$(DIST)/include
xpcom/reflect/xptcall/src/md/unix/Makefile.in:64:ASFLAGS		+= -xarch=v9
xpcom/reflect/xptcall/src/md/unix/Makefile.in:102:ASFLAGS += -xarch=amd64

Comment 8

[Mike Hommey [:glandium]]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #7)
> I did things this way on purpose.  Doing this in config.mk would mess up the
> ordering of these flags for Objective-C++ code, since I'd have to add those
> flags at the end of COMPILE_CMMFLAGS which means that stuff in
> MOZBUILD_CMMFLAGS would potentially override them.  Need-info?ing on this
> mostly.

You probably mean COMPILE_CXXFLAGS and COMPILE_CMMFLAGS. It's probably a big fail that we're giving cxxflags to .mm compilation, but it's not something i'm going to block you on. Ok with the rules.mk changes, I don't think there's any need for a CLOBBER anyways.

> > ::: gfx/cairo/libpixman/src/moz.build
> > @@ +128,5 @@
> > >  
> > >  if use_mmx:
> > >      DEFINES['USE_MMX'] = True
> > >      SOURCES += ['pixman-mmx.c']
> > > +    SOURCES['pixman-mmx.c'].flags = mmx_flags
> > 
> > Not sure there's much value in those intermediate variables.
> 
> I did that so that those flags and use_mmx etc. are in the same place. 
> Please confirm if you want me to change that?

Either way is fine. I'm just not convinced it buys much to have them there vs. here.

> We don't, I already checked.
> 
> $ git grep -w ASFLAGS | grep Makefile.in
> media/libjpeg/Makefile.in:8:ASFLAGS=$(LIBJPEG_TURBO_ASFLAGS)
> -I$(topsrcdir)/media/libjpeg/simd/
> media/libtheora/lib/Makefile.in:11:ASFLAGS = -march=armv7-a -mfpu=neon
> media/libvpx/Makefile.in:7:ASFLAGS=$(VPX_ASFLAGS) -I.
> -I$(topsrcdir)/media/libvpx/ -I$(topsrcdir)/media/libvpx/vpx_ports/
> xpcom/reflect/xptcall/src/md/unix/Makefile.in:40:ASFLAGS		+=
> -I$(DIST)/include
> xpcom/reflect/xptcall/src/md/unix/Makefile.in:64:ASFLAGS		+= -xarch=v9
> xpcom/reflect/xptcall/src/md/unix/Makefile.in:102:ASFLAGS += -xarch=amd64

I think we had some in the past. Not that I would encourage it in the future, but it seems to me we don't really need to be restrictive here, when those flags are easily added to rules.mk the same way, and when that simplifies the sandbox handling code.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de9872bce666

Comment 10

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0737df20477
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e3455c9d8da

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d0737df20477
https://hg.mozilla.org/mozilla-central/rev/6e3455c9d8da