Closed Bug 1248020 Opened 4 years ago Closed 4 years ago

Add something to moz.build to use yasm as $(AS)

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox47 affected, firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: ted, Assigned: ted)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Right now we have a few places in the tree that want to use yasm as their assembler, but they have to set AS = $(SOMETHING) in their Makefile.in, like:
https://dxr.mozilla.org/mozilla-central/rev/576a6dcde5b68c2ea45324ed5ce1dabb7d833d09/media/ffvpx/libavcodec/x86/Makefile.in#6

We should add something like `USE_YASM = True` to moz.build which would force `AS := $(YASM)`. We could get rid of all those `FOO_AS` variables once we do that.

(bug 1248016 covers the other line in that Makefile, ASM_SUFFIX.)
So these patches got a little complicated. The first one is fairly straightforward--it adds `USE_YASM` to the moz.build context, which just sets a few passthrough variables to do what the existing Makefiles do--set AS:=$(YASM) and ASFLAGS:=$(YASM_ASFLAGS). It does copy a bit of logic from elsewhere in configure to get a usable set of default flags for yasm.

The second patch is more complicated because the configure logic for the things using yasm is complicated! First of all, we don't require yasm, and we don't use it on every platform (It only supports x86 and x86-64, for one thing), so we still have to keep some logic for falling back to the default assembler for some things. I tried to simplify things a little bit--nothing AC_SUBSTS a `FOO_AS` value anymore that points to yasm, they just either have a flag indicating they should use yasm (VPX, LIBJPEG_TURBO), or they only support yasm and nothing else (LIBAV_FFT, FFVPX). Each set of ASFLAGS also got pruned down to just the set of things that aren't in YASM_ASFLAGS. These could probably even just be moved into moz.build at this point, but I didn't want to make this patch even more complicated.

The config.mk changes are necessary because the ASFLAGS for yasm are getting set in backend.mk now, which comes before rules.mk and so that block was resetting them, which was no good. The AS_DASH_C bit is sent down as a passthrough variable in emitter.py, so that's not necessary either. The rest of the patch is just moz.build fiddling to add USE_YASM where needed and remove the no-longer-necessary Makefile.in files.
Attachment #8726295 - Flags: review?(mshal)
Comment on attachment 8726295 [details]
MozReview Request: bug 1248020 - add USE_YASM to moz.build. r?mshal

https://reviewboard.mozilla.org/r/37917/#review34575

::: old-configure.in:3410
(Diff revision 1)
> +      YASM_ASFLAGS="-f x64 -rnasm -pnasm"

From 'yasm -f help', it looks like 'win64' should also work for x64, and might make it a little more clear here. I see where this came from now in the second patch, so consider it a nit if you don't want to change the behavior :)

::: old-configure.in:3416
(Diff revision 1)
> +      if $CC -E -dM -</dev/null | grep -q __ELF__; then

If this check is false, do we need to unset YASM rather than just leaving YASM_ASFLAGS blank?

::: old-configure.in:3421
(Diff revision 1)
> +      if $CC -E -dM -</dev/null | grep -q __ELF__; then

Similar to above.
https://reviewboard.mozilla.org/r/37917/#review34581

::: old-configure.in:3410
(Diff revision 1)
> +      YASM_ASFLAGS="-f x64 -rnasm -pnasm"

On third thought, LIBAV_FFT_ASFLAGS used to use '-f win64', so maybe we should go with that to be a little more readable.
Comment on attachment 8726296 [details]
MozReview Request: bug 1248020 - convert a bunch of moz.build files to use USE_YASM. r?mshal

https://reviewboard.mozilla.org/r/37919/#review34579

I think all of the moz.build / configure changes look good. Is there a way you can double-check to make sure we didn't accidentally stop using asm where we should? Maybe compare the number of 'yasm' invocations in the before & after on each platform would suffice. I'd hate to lose performance over a build config change.

::: old-configure.in:5467
(Diff revision 1)
> +      VPX_ASFLAGS="-DPIC"

I think you are now adding -DPIC where it wasn't used before (WINNT:x86_64 and *:arm*). Does that matter?
Attachment #8726296 - Flags: review?(mshal)
Assignee: nobody → ted
https://reviewboard.mozilla.org/r/37919/#review35129

::: old-configure.in:5467
(Diff revision 1)
> +      VPX_ASFLAGS="-DPIC"

Good catch! I think this is only the Win64 case, the arm case doesn't use yasm. I'll fix it to match what we currently have. I don't know if it matters, but I'd rather not change it.
Comment on attachment 8726295 [details]
MozReview Request: bug 1248020 - add USE_YASM to moz.build. r?mshal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37917/diff/1-2/
Attachment #8726295 - Flags: review?(mshal)
Comment on attachment 8726296 [details]
MozReview Request: bug 1248020 - convert a bunch of moz.build files to use USE_YASM. r?mshal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37919/diff/1-2/
Attachment #8726296 - Flags: review?(mshal)
Comment on attachment 8726295 [details]
MozReview Request: bug 1248020 - add USE_YASM to moz.build. r?mshal

https://reviewboard.mozilla.org/r/37917/#review35135

::: old-configure.in:3430
(Diff revisions 1 - 2)
> +    # If we didn't get ASFLAGS, we shouldn't be using yasm.

nit: mind as well say YASM_ASFLAGS in the comment instead of ASFLAGS to line up exactly with the variable name.
Attachment #8726295 - Flags: review?(mshal) → review+
Comment on attachment 8726296 [details]
MozReview Request: bug 1248020 - convert a bunch of moz.build files to use USE_YASM. r?mshal

https://reviewboard.mozilla.org/r/37919/#review35137
Attachment #8726296 - Flags: review?(mshal) → review+
https://reviewboard.mozilla.org/r/37917/#review34581

> On third thought, LIBAV_FFT_ASFLAGS used to use '-f win64', so maybe we should go with that to be a little more readable.

This burned on try:
https://treeherder.mozilla.org/logviewer.html#?job_id=17757164&repo=try

presumably because this check is looking for exactly win64:
https://dxr.mozilla.org/mozilla-central/rev/05c087337043dd8e71cc27bdb5b9d55fd00aaa26/media/libvpx/vpx_config.asm#11

I don't know why yasm would implement two options for the same format but not expose them as the same thing internally, but I'm going to leave this as x64 because I don't want to touch any of the assembly code.
Actual successful Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e94afcfb3333

I downloaded all the logs and compared vs this try run of a stock m-c changeset:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4f33b054986&selectedJob=17757664

I did a quick sanity check "grep 'yasm -o' | wc -l" to make sure the number of yasm invocations was the same on both pushes on all platforms. Then I did some spot-checks to check that the yasm commandlines were the same. They were fine on everything *but* Win64. As it turns out, *some* of the Makefiles using yasm used '-f win64' where others used '-f x64'.

All the other asm files that check the output format seem to handle win64/x64 interchangably though:
https://dxr.mozilla.org/mozilla-central/rev/f0c0480732d36153e8839c7f17394d45f679f87d/media/libvpx/third_party/x86inc/x86inc.asm#51

So I think this should be fine.
rillian: FYI, I reworked the way various media bits invoke yasm in this patch. I verified that the yasm commandlines are the same (modulo the bit in comment 17 about Win64).
https://hg.mozilla.org/mozilla-central/rev/f8278f56b336
https://hg.mozilla.org/mozilla-central/rev/116882b45609
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.