934984 - libvpx bustage when building on MSVC with /GL

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

libvpx apparently doesn't play nicely with optimizations. When PGO is enabled, asm_enc_offsets.c is specifically included. According to glandium, it would be useful if we were able to disable optimizations for those files in the non-PGO case as well, so filing :)

9:31.98 Finished generating code
9:32.05 Too many sections
9:32.05 c:\mozbuild\src\mozilla-central\objdir-fx-64\media\libvpx\Makefile:433:0: command './host_obj_int_extract.exe gas asm_enc_offsets.obj \
9:32.05 > asm_enc_offsets.asm' failed, return code 1

Comment 1

[Ryan VanderMeulen [:RyanVM]]

s/included/excluded *sigh*

Comment 2

[Mike Hommey [:glandium]]

To clarify what i meant, we have a bunch of files that require not being optimized, and that's information we should move in moz.build at some point. It would already be useful to have a config.mk/rules.mk helper for that, and that could be used for the two c files we build in libvpx only to get struct offsets.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

In case anybody's wondering, I checked today and this still happens.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #3)
> In case anybody's wondering, I checked today and this still happens.

You're explicitely adding -GL, right? (because for "normal" PGO, with MOZ_PGO, this is not supposed to happen)

Comment 6

[Mark Straver]

You should be able to build with LTCG without using PGO, though.
And won't this happen with PGO as well? -GL is implied for any PGO build.

Comment 7

[(Away)]

So apparently with -GL we get (according to dumpbin) "ANONYMOUS OBJECT" rather than "COFF OBJECT".

In the bytes where COFF would store the number of sections, the file has 0xFFFF instead. So I guess this is unrelated to the many-rdata issue seen in bug 1018402.

Comment 8

[(not currently active) Ted Mielczarek]

Yeah, LTCG-output obj files are not real obj files.

Comment 9

[(Away)]

(In reply to Mark Straver from comment #6)
> And won't this happen with PGO as well? -GL is implied for any PGO build.

I think what glandium means is that when you do a PGO build by setting MOZ_PGO, then that also triggers the fixup to not PGO this file.

Comment 10

[Dorian]

How does this fixup work? Can it be triggered without pgo build?

Comment 11

[gharpure.yash]

I managed to build Firefox with /GL by following the steps in bug 771588:

Modify Makefile.in for libvpx

ifdef VPX_NEED_OBJ_INT_EXTRACT

+# only for MSVC
+ifeq (WINNT_,$(OS_TARGET)_$(GNU_CC))
+vpx_scale_asm_offsets.$(OBJ_SUFFIX): CFLAGS += -GL- 
+endif


vpx_scale_asm_offsets.asm: vpx_scale_asm_offsets.$(OBJ_SUFFIX) $(HOST_PROGRAM)
	./$(HOST_PROGRAM) $(VPX_OIE_FORMAT) $< \
	    $(if $(VPX_AS_CONVERSION),| $(VPX_AS_CONVERSION)) > $@

# Filter out this object, because we don't want to link against it.
# It was generated solely so it could be parsed by obj_int_extract.
OBJS := $(filter-out vpx_scale_asm_offsets.$(OBJ_SUFFIX),$(OBJS))

+# only for MSVC
+ifeq (WINNT_,$(OS_TARGET)_$(GNU_CC))
+vp8_asm_enc_offsets.$(OBJ_SUFFIX): CFLAGS += -GL-
+endif

vp8_asm_enc_offsets.asm: vp8_asm_enc_offsets.$(OBJ_SUFFIX) $(HOST_PROGRAM)
	./$(HOST_PROGRAM) $(VPX_OIE_FORMAT) $< \
	    $(if $(VPX_AS_CONVERSION),| $(VPX_AS_CONVERSION)) > $@

# Filter out this object, because we don't want to link against it.
# It was generated solely so it could be parsed by obj_int_extract.
OBJS := $(filter-out vp8_asm_enc_offsets.$(OBJ_SUFFIX),$(OBJS))

endif

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Still broken with MSVC2013.

Comment 13

[(Away)]

As far as I can tell this issue was already fixed in bug 771588 but the fix was lost here: https://hg.mozilla.org/integration/mozilla-inbound/diff/0fc3f0660258/media/libvpx/Makefile.in
(That refactoring was sound for our main build, but it broke people who want to build GL without PGO).

So why don't we just reapply that fix -- that's what comment 11 did. Ryan since you can repro this, want to test it out and post a patch?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

I see where the no_pgo is set in moz.build, but I'm not sure how to make the change shown in the changeset you linked to.

Comment 15

[(Away)]

Well my proposal (which glandium might not like) is to leave the no_pgo stuff in moz.build, but do an additional -GL- in Makefile.in for good measure, just like the old days.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

What about something like SOURCES[s].flags += ['-GL-']?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Yep, that works locally. I'll push it off to try with and without PGO enabled to make sure our build infra doesn't puke either.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Attachment: Disable /GL for the libvpx asm files

I've verified that this works locally. I have Try runs in progress both with and without PGO enabled to confirm that it doesn't break anything. I'd like to be able to land this assuming they turn out green :)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8545647 [details] [diff] [review]
Disable /GL for the libvpx asm files

Green on Try. Can I get an rs to land please? :)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/b43ac7b3918b

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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