1659727 - mpcpucache.c invalid output constraint on Linux/ARM

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[J.C. Jones [:jcj] (he/they)]

Originally from https://bugzilla.mozilla.org/show_bug.cgi?id=1646594#c8

mpi/mpcpucache.c:91:15: error: invalid output constraint '=a' in asm
: "=a"(*eax),
^

the compile command contains -Di386 which originates from:

OS_REL_CFLAGS = -Di386

this is because of the include Linux.mk added in the commit; at the time in arch.mk not all variables that affect Linux.mk have been initialized yet and so it sets x86 specific variables, which are not all overwritten when Linux.mk is included a second time.

the easiest fix for me would be to add ifneq ($(OS_TARGET),Android) around that, but it's possible that this breaks other non-Android configs too.

so Android.mk includes Linux.mk which sets CC and includes UNIX.mk which includes Werror.mk which sets GCC_VERSION...

i'm thinking the whole ifndef NSS_DISABLE_AVX2 block should be moved from arch.mk to a different file that is read after config.mk include coreconf/$(OS_TARGET).mk, but i'm not sure where to move it to.

Comment 1

[Michael Stahl]

thanks; i've added a work-around on our side for now in https://cgit.freedesktop.org/libreoffice/core/commit/?id=c7d1bb119d88faaafb53d65ad8907ede97d89c8c

but i guess this could break non-Android Linuxes as well, at least in theory, so a proper fix would be better...

Comment 2

[J.C. Jones [:jcj] (he/they)]

Danh, do you have a suggestion as to the best way to repair this without breaking your fix from Bug 1646594? Any chance you could give us a revised patch?

Comment 3

[Danh]

I think: "include Linux.mk" in coreconf.mk is fundamentally broken (because BSD people may also need avx2?).
(yes, the broken code written by me)

I think:

• we should remove that broken: "include Linux.mk"
• In the "ifndef NSS_DISABLE_AVX2" in "coreconf/arch.mk", remove "NSS_DISABLE_AVX2 = 1", so if "coreconf/arch.mk" is being visited multiple time can run into this block, (I'm not sure if this is the case).
• Modify lib/freebl/Makefile to set NSS_DISABLE_AVX2 if not defined, I'm not really sure what older (and other) make would do if a variable was set to 0, if those make implementation doesn't unset it, we should be fine with flip those variables.

I write this without building, just some foods for thought.
I can try with build and test this weekend, though.

Comment 4

[Kevin Jacobs [:kjacobs]]

Danh, were you able to test the patch you suggested?

Comment 5

[Danh]

Attachment: avx2-new.patch

Comment 6

[Danh]

That approach doesn't work, since we have a guard to not include an "*.mk" file twice.
Working through it, I think putting that code in "arch.mk" is broken.

I propose moving that block into "config.mk" instead.

Attach here is a patch.
I'll go away for my vacation for 5 days so please bear with me that I'm late to response to any feedback.

Comment 7

[Danh]

Attachment: Bug 1659727 - Move makefile avx2 detection to config.mk

Current code base use CPU_ARCH to detect if avx2 is supported in arch.mk
However, when arch.mk included, CPU_ARCH haven't been initialised, CPU_ARCH
will be initialised by the OS specific code later on.

Move the AVX2 detection to config.mk, after all other initialisation done.

Comment 8

[Kevin Jacobs [:kjacobs]]

https://hg.mozilla.org/projects/nss/rev/c6dcb99e61210c730ca7cae65dc03d83d7cbf7ff