Closed Bug 1659727 Opened 9 months ago Closed 8 months ago

mpcpucache.c invalid output constraint on Linux/ARM

Categories

(NSS :: Libraries, defect, P2)

3.55
ARM
Linux

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jcj, Assigned: congdanhqx)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

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.

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...

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?

Flags: needinfo?(congdanhqx)

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.

Flags: needinfo?(congdanhqx)

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

Flags: needinfo?(congdanhqx)
Attached patch avx2-new.patchSplinter Review
Flags: needinfo?(congdanhqx)

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.

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.

Attachment #9172558 - Flags: review?(kjacobs.bugzilla)
Assignee: nobody → congdanhqx
Severity: -- → S3
Priority: -- → P2
Attachment #9172558 - Attachment description: nss: Makefile: move avx2 detection to config.mk → Bug 1659727 - Move makefile avx2 detection to config.mk
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 3.57
Attachment #9172558 - Flags: review?(kjacobs.bugzilla)
Duplicate of this bug: 1660729
You need to log in before you can comment on or make changes to this bug.