1649691 - Make static assert less strict in MPSCQueue.h

Status: RESOLVED FIXED

(Core :: Audio/Video: cubeb, defect, P1)

Description

[Paul Adenot (:padenot)]

This is breaking downstream on linux armv7: https://bugzilla.mozilla.org/show_bug.cgi?id=1645416#c11

Comment 1

[Paul Adenot (:padenot)]

Attachment: Bug 1649691 - Make static assert less strict in MPSCQueue.h. r?achronop

Depends on D81593

Comment 2

[Paul Adenot (:padenot)]

Also I'm adjusting the comment because it's wrong with the version of jemalloc we're using (I had read documentation on a different version of jemalloc it seems), https://bugzilla.mozilla.org/show_bug.cgi?id=1649140 helps.

Comment 3

[tt_1]

Attachment: build.log.gz

hey, thank you so much for your fast answer.

So this is firefox-79.0_beta9 with patch from D81593 and patch from this bug, and it still fails with new errors at AsyncLogger.h and MPSCQueue.h

have a look at the full build log I've attached, the verbose error messages are starting at 15:15

Comment 4

[Paul Adenot (:padenot)]

I've updated the patch.

Comment 5

[tt_1]

Attachment: build.log.gz

I regret saying it, but that didn't have any effect :/

Comment 6

[Paul Adenot (:padenot)]

I'm having a hard time understanding why and I don't have a system to test this config (aarch64 + linux desktop?), so I'm disabling this for now on this configuration so it builds. Hopefully this fixes it.

Comment 7

[tt_1]

the machine I'm building for is a rpi2, or to be more specific armv7a-unknown-linux-gnueabihf

for testing the beta I'm always using the big machine and cross compile with cross-gcc, otherwise it would take up to two days for results plus my delay in answering here, it's too slow for working on patches like this.

I'm happy to test again the patch which disables the problematic code. Where can I get it from, please?

Comment 8

[Paul Adenot (:padenot)]

Same URL: https://phabricator.services.mozilla.com/D81818 (you can get a patch file from the top right in the menu, download raw diff).

Comment 9

[tt_1]

Attachment: 0001-firefox-79.0_beta2_D81593+D81818.patch

There is a conflict in the patches. I tried to sort it out and joined them (D81593+D81818) into one file. Can you please confirm this is what your intentions are for firefox-79.0_beta2? I'll test it when confirmed.

Comment 10

[Paul Adenot (:padenot)]

I think so yes. Only the first and last hunk are necessary however, but the one in the middle looks correct.

Comment 11

[tt_1]

Attachment: build.log.gz

It still fails with the joined patch. This is a bit of a bummer, no idea from where it is leaking? :(

Comment 12

[Paul Adenot (:padenot)]

Sorry my bad, this is a compile error on your merge, I misread. Newest version is a patch that applies on beta.

Comment 13

[tt_1]

It fails again with both patches. Are you sure about the dependency on D81593?

it's firefox-79.0_beta2 + D81593 + D81818 on my side

Sadly I don't really have to capacities to work directly with mozilla central

Comment 14

[Paul Adenot (:padenot)]

Just take the latest patch without dependency.

Comment 15

[Paul Adenot (:padenot)]

To be clear, your tree + D81818 should work. Or I don't really understand the configuration you build for, since D81818 disables the assert for arm (64 or 32) + !android + linux.

Comment 16

[tt_1]

ok, I'll test again. D81593 has been merged to mozilla-central only. Since I'm using the firefox-79 beta, I grabbed it before applying your patch on top of it. That doesn't work, now I'm going to retry, results coming soon.

Comment 17

[tt_1]

it might be that D81593 carries a dependency on D81592

however, with none of these and your patch D81818 only applied on top of firefox-79.0_beta2, the compile passes with flying colours.

I'm open for testing other fixes on the base of a now working compile.

Comment 18

[Paul Adenot (:padenot)]

Thanks for testing, I'll try to get this into beta, since it's juste a static_assert fix. And sorry for the hassle, I'm not well setup to reproduce this, so I was trying things blindly.

Comment 19

[tt_1]

I've tried again, this is a chain of depdendencies in regard to the patches: D81592 > D81593 > D81818

and it's either ( D81592 > D81593 ) > D81818

or only D81818

with firefox-79.0_beta2 as a base.

So from my perspective, D81818 should be applied to the 79.0_beta branch to fix the compile failure. And it should be uplifted (slightly rebased) to the mozilla-central branch too, so that the error doesn't return in firefox-80 in about a month of time. If D81592 and D81593 should be uplifted to the 79.0_beta branch I can't give advice on.

Does this need some more testing on other 32bit arches, i686 for instance?

Comment 20

[Paul Adenot (:padenot)]

Yes, this is what I said in comment 14. Latest iteration was made directly off of beta, and I'll fix in central and also beta, but I wanted to confirm it fixes the problem for you on beta.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/decb3b5cc1c5
Make static assert less strict in MPSCQueue.h. r=achronop

Comment 22

[Paul Adenot (:padenot)]

Comment on attachment 9160593 [details]
Bug 1649691 - Make static assert less strict in MPSCQueue.h. r?achronop

Beta/Release Uplift Approval Request

• User impact if declined: Compilation error on Firefox for Linux desktop running on ARM, which is a pain for downstream maintainers/packagers.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is disabling a static assert on a non-tier-1 platform, and has no impact on tier-1 platform. This has no impact at runtime whatsoever.
• String changes made/needed: none

Comment 23

[Dorel Luca [:dluca]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9160593 [details]
Bug 1649691 - Make static assert less strict in MPSCQueue.h. r?achronop

Makes life easier for non-Tier 1 platforms. Approved for 79.0b5.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c2fa58073ece

Comment 26

[tt_1]

I just wanted to leave feedback that firefox-79.0_beta6 compiles successfully and also no runtime issues. Thank you for fixing this! :-)