Closed Bug 1085599 Opened 7 years ago Closed 7 years ago

Bug 1063356 breaks (non-NEON?) (armv6?) builds

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(firefox35+ fixed, firefox36+ fixed, firefox37 fixed)

RESOLVED FIXED
2.1 S8 (7Nov)
Tracking Status
firefox35 + fixed
firefox36 + fixed
firefox37 --- fixed

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(2 files)

Not sure which yet, but this prevents gecko from building for Raspberry Pi and most likely any other armv6 devices.
You say you're using the android ICS compiler, gcc 4.4.3. What's the actual error?
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1085607
Sorry, wrong bug!
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: DUPLICATE → ---
I can most certainly confirm that this bug is occurring ;).

There are a lot of errors that indicate intrinsics aren't compiling.  For example

error: 'uint8x8_t' undeclared (first use in this function)

but I'm putting together a workaround atm, so haven't been able to dig into why.  We should be doing ICS builds on infra so I would have expected something to break if it's compiler-options-related.

However, that likely implies that we're dropped support for armv6 on ICS.
Status: UNCONFIRMED → NEW
Ever confirmed: true
> but I'm putting together a workaround atm, so haven't been able to dig into
> why.  We should be doing ICS builds on infra so I would have expected something
> to break if it's compiler-options-related.

Yes, I wondered if this would work at all on arm6. IIRC it's no longer tier 1?

> However, that likely implies that we're dropped support for armv6 on ICS.

How can I reproduce?
The error spew includes the problem

/home/cjones/mozilla/b2g/prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/../lib/gcc/arm-linux-androideabi/4.4.3/include/arm_neon.h:32:2: error: #error You must enable NEON instructions (e.g. -mfloat-abi=softfp -mfpu=neon) to use arm_neon.h

Note what the gcc docs say, however

  These built-in intrinsics for the ARM Advanced SIMD extension are available when the -mfpu=neon switch is used: 

With a minimal test program 
-----
#include <arm_neon.h>
void test(void) { uint8x8_t foo; }
-----

OK, so let's try

  /home/cjones/mozilla/b2g/prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/arm-linux-androideabi-gcc -o arm-neon.o -c -mfloat-api=softfp -mfpu=neon arm-neon.c
  cc1: error: unrecognized command line option "-mfloat-api=softfp"

Very cute.  The first error message lies.  Let's just try

  /home/cjones/mozilla/b2g/prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/arm-linux-androideabi-gcc -o arm-neon.o -c -mfpu=neon arm-neon.c
  /home/cjones/mozilla/b2g/prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/../lib/gcc/arm-linux-androideabi/4.4.3/include/arm_neon.h:32:2: error: #error You must enable NEON instructions (e.g. -mfloat-abi=softfp -mfpu=neon) to use arm_neon.h

Nope, same error as originally.  Try to stick one more thing to the wall

  /home/cjones/mozilla/b2g/prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/arm-linux-androideabi-gcc -o arm-neon.o -c -march=armv7-a -mfpu=neon arm-neon.c
  /home/cjones/mozilla/b2g/prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/../lib/gcc/arm-linux-androideabi/4.4.3/include/arm_neon.h:32:2: error: #error You must enable NEON instructions (e.g. -mfloat-abi=softfp -mfpu=neon) to use arm_neon.h

Nope, same error again.

I can't get this to compile at all with the ICS toolchain, so I don't know a minimal delta of command-line options would be.

Anyone happen to know what toolchain our ICS builders use?
We're currently using the same 4.4.x toolchain that all ICS devices use. Bug 1056337 is looking at upgrading to 4.8 though, so if 4.8 fixes your problems you can use that instead.
(In reply to Ralph Giles (:rillian) from comment #5)
> > but I'm putting together a workaround atm, so haven't been able to dig into
> > why.  We should be doing ICS builds on infra so I would have expected something
> > to break if it's compiler-options-related.
> 
> Yes, I wondered if this would work at all on arm6. IIRC it's no longer tier
> 1?

It's Tier II.

> 
> > However, that likely implies that we're dropped support for armv6 on ICS.
> 
> How can I reproduce?

(In reply to Ralph Giles (:rillian) from comment #5)
> > but I'm putting together a workaround atm, so haven't been able to dig into
> > why.  We should be doing ICS builds on infra so I would have expected something
> > to break if it's compiler-options-related.
> 
> Yes, I wondered if this would work at all on arm6. IIRC it's no longer tier
> 1?

Yes, apparently it's Tier II now.

> 
> > However, that likely implies that we're dropped support for armv6 on ICS.
> 
> How can I reproduce?

Well I'm trying to find a combo of build flags that get the test program above to even work.  Then I can see what we're *not* doing for the armv6 build.

But not even |-march=armv7-a -mfpu=neon| gets the two-line example to build.
(Whups, sorry for double post, had a bad tab switch.)
Another datum: I'm on 4.4.3, not sure if that's old for ICS ... I'm on 4.0.1 for reasons that quite unfortunate now.
(In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need me from comment #8)
> But not even |-march=armv7-a -mfpu=neon| gets the two-line example to build.

Turns out you need all of the |-march=armv7-a -mfloat-abi=softfp -mfpu=neon| magic words to get __ARM_NEON__ set.  Fyi.
The problem seems to be that build/autoconf/arch.m4 doesn't set a default MOZ_FLOAT_ABI.  (It sets MOZ_FLOAT_ABI=softfp by default for armv7-a target.)  That's easy enough for me to work around in BoardConfig.mk ... and I don't want to touch defaults in arch.m4 with a 50-foot pole ;).  I confirmed that |-march=armv6 -mfloat-abi=softfp -mfpu=vfp -mfpu=neon| indeed does build my testcase.

Not sure if this is going to disable any of the armv6 VFP optimizations that we so so need ... but I didn't confirm through perf testing that they're on either, just a code skim.
/home/cjones/mozilla/b2g/gecko/media/libvpx/vp8/common/arm/neon/mbloopfilter_neon.c: In function 'vp8_mbloop_filter_vertical_edge_uv_neon':
/home/cjones/mozilla/b2g/gecko/media/libvpx/vp8/common/arm/neon/mbloopfilter_neon.c:625: internal compiler error: in change_address_1, at emit-rtl.c:1954
Please submit a full bug report,

/me weeps
As best as I can tell:

 -march=armv6 -marm -mfpu=vfp -mfpu=neon (default in this code) --- doesn't define __ARM_NEON__, doesn't build
 -march=armv6 -marm -mfpu=vfp -mfloat-abi=softfp -mfpu=neon --- internal compiler error
 -march=armv6 -mthumb -mfpu=vfp -mfloat-abi=softfp -mfpu=neon (these options don't make sense) --- doesn't define __ARM_NEON__, doesn't build
 -march=armv7-a -mthumb -mfpu=vfp -mfloat-abi=softfp -mfpu=neon (default for armv7-a) --- builds (hooray!).  But won't run on armv6 because of thumb.
 -march=armv7-a -marm -mfpu=vfp -mfloat-abi=softfp -mfpu=neon --- same internal compiler error as above

So the relevant gcc bug is that |-marm -mfpu=neon| triggers the ICE here.  Sounds like a normal garden-variety dumb bug, but puts us in a difficult spot.

I'll see if there's a way to hack-enable |-march=armv7-a -mthumb| for this code.  If the NEON code ever ran anyway, it would crash, so generating thumb shouldn't be problematic unless I miss my guess about how this code is used ... if it dynamically checks *within the impl* if it has NEON, then we're screwed and have permanently broken armv6.
So it looks like we were already trying to do this through the |VPX_ASFLAGS| hack.  Seems like it happens to have worked for raw asm files, which don't seem to have NEON instructions gated on the same -mfloat-abi=softfp flag that the C intrinsics are gated on.  (Didn't confirm though.)

But now things look doubly-broken, because AFAICT we're using moz.build for libvpx?  It doesn't reference $VPX_ASFLAGS at all.  But I don't know moz.build so that guess may be wrong.  I'll see if I can cargo-cult something in.

We set VPX_ASFLAGS for several other configs, but maybe they're no longer needed there?
Have a patch that builds, but crashes early in startup.  Looking.
Wasn't able to quickly see what's crashing.  Starting up b2g under gdb takes minutes and minutes and minutes (not sure, maybe more than 10???), so that's not feasible for iterating.  But when it crashes, I can't attach gdb quickly enough.  Backed up my local changes to the most conservative (way faster clean-build then push for iteration), but still crashing.  So not going to be able to resolve this tonight.
(In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need me from comment #12)
> I confirmed that |-march=armv6 -mfloat-abi=softfp -mfpu=vfp -mfpu=neon|
> indeed does build my testcase.
> 
> Not sure if this is going to disable any of the armv6 VFP optimizations that
> we so so need ... but I didn't confirm through perf testing that they're on
> either, just a code skim.

What about -mfpu=neon-vfpv4 ? I'm not sure what multiple -mfpu options does, or whether neon implies vfp. Does it break if you pass -mfpu=neon first?

I did find in the manual, "floating-point operations are not generated by GCC's auto-vectorization pass unless -funsafe-math-optimizations is also specified" so it sounds like we are safe from -mfpu turning on non-portable code generation.

> But now things look doubly-broken, because AFAICT we're using moz.build
> for libvpx? It doesn't reference $VPX_ASFLAGS at all.

So moz.build doesn't do .asm files afaict, so VPX_ASFLAGS is for media/libvpx/Makefile.in, which has rules to run them through yasm.

The new code has some optimizations in .asm files, and some in .c files with intrinsics, so you need to mirror the VPX_ASFLAGS in CFLAGS, translating yasm syntax to gcc as appropriate. See the '-mfpu=neon' added in the moz.build. Maybe set a key in the config dict from configure.in and check it in the moz.build?
That's approximately what my patch does, except it also includes the gcc ICE workaround.

But the crash in the resulting artifact looks like it might be a different problem.  Trying to track that down but it's extremely slow to iterate.

(In reply to Ralph Giles (:rillian) from comment #18)
> CFLAGS, translating
> yasm syntax to gcc as appropriate. See the '-mfpu=neon' added in the
> moz.build

As I mentioned I don't know this new custom build system.  I hate to even ask because it sounds crazy if false, but can you confirm that mutating the CFLAGS variable in that file only affects the source files built in that target directory?  I.e. CFLAGS is "private" to that file?
(In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need me from comment #19)

> As I mentioned I don't know this new custom build system.  I hate to even
> ask because it sounds crazy if false, but can you confirm that mutating the
> CFLAGS variable in that file only affects the source files built in that
> target directory?  I.e. CFLAGS is "private" to that file?

Yes, that's correct. To quote './mach mozbuild-reference':

  CFLAGS: Flags passed to the C compiler for all the C source files declared in this directory.

So a directory has a moz.build file; the system generates a makefile in the corresponding obj directory, and variables like CFLAGS are expanded with the local value inside that makefile.

It's basically a python script with inherits a dict and sets some magic variables which the generator turns into an actual build description. Declarative-only though; build rules are in the tool's library.
Thanks, very helpful.
OK, so my RPi patches on top of

https://github.com/mozilla/gecko-dev/commit/b7b2dc40de355add3634cc972fc31521a8daeed3

(git mirror of what hg calls the parent of https://hg.mozilla.org/integration/mozilla-inbound/rev/7bca0cbbecf1 )

everything works.

If I apply my RPi patches to

https://github.com/mozilla/gecko-dev/commit/171fb5de2bd5bcdb70a10182ea85c6d3ea2f6f17

then the build errors out as described here.

If I apply this minimal fix (AFAICT)

diff --git a/media/libvpx/moz.build b/media/libvpx/moz.build
index d278748..23155be 100644
--- a/media/libvpx/moz.build
+++ b/media/libvpx/moz.build
@@ -53,17 +53,17 @@ if CONFIG['VPX_X86_ASM']:
         CFLAGS += ['-msse4.1']
 
     #postproc is only enabled on x86 with asm
     SOURCES += files['VP8_POSTPROC']
 
 arm_asm_files = []
 if CONFIG['VPX_ARM_ASM']:
     arm_asm_files += files['ARM_ASM']
-    CFLAGS += ['-mfpu=neon']
+    CFLAGS += ['-march=armv7-a', '-mthumb', '-mfloat-abi=softfp', '-mfpu=neon']
 
     if CONFIG['VPX_AS_CONVERSION']:
         GENERATED_SOURCES += [ "%s.%s" % (f, CONFIG['VPX_ASM_SUFFIX'])
             for f in sorted(arm_asm_files) if f.endswith('.asm')
         ]
         SOURCES += [
             f for f in sorted(arm_asm_files) if not f.endswith('.asm')
         ]

then we crash somewhere off in the weeds

Program received signal SIGSEGV, Segmentation fault.
defineAttribute (type=0x0, attId=0x0, isCdata=0 '\000', 
    isId=<value optimized out>, value=0x0, parser=0xb2782030)
    at /home/cjones/mozilla/b2g/gecko/parser/expat/lib/xmlparse.c:5393
5393	  if (type->nDefaultAtts == type->allocDefaultAtts) {
(gdb) bt
#0  defineAttribute (type=0x0, attId=0x0, isCdata=0 '\000', 
    isId=<value optimized out>, value=0x0, parser=0xb2782030)
    at /home/cjones/mozilla/b2g/gecko/parser/expat/lib/xmlparse.c:5393
#1  0xb57ea4c8 in doProlog (parser=0xb2782030, enc=0x0, 
    s=0xbe8a1934 "\210h\372\265\t", end=<value optimized out>, 
    tok=-1228712896, next=0xb6c04870 "20141021110153", nextPtr=0xb6142929, 
    haveMore=1 '\001')
    at /home/cjones/mozilla/b2g/gecko/parser/expat/lib/xmlparse.c:4094
#2  0xb6c04870 in ?? ()
Cannot access memory at address 0x21
#3  0xb6c04870 in ?? ()
Cannot access memory at address 0x21
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

On the bright side, the crash is reproducible, doesn't seem nondeterministic.  But on the unhappy side, something is confusing gdb, and it takes 10 minutes to get that point in execution anyway.  So I could literally run out of time debugging this.  Going to switch back to setting up RPi folks with instructions to work around this for now.

armv6 not looking good.
(In reply to Ralph Giles (:rillian) from comment #18)
> What about -mfpu=neon-vfpv4 ?

/home/cjones/mozilla/b2g/gecko/media/libvpx/vp8/common/rtcd.c:1: error: invalid floating point option: -mfpu=neon-vfpv4
Assembler messages:
Error: unknown floating point format `neon-vfpv4'

Error: unrecognized option -mfpu=neon-vfpv4
OK, duh!  Changing CFLAGS is going to apply those options to *all* C sources.

Unless I'm wrong, that has no chance of working because the compiler is free to generate NEON code for any arbitrary floating point expressions.  (And my workaround breaks that even harder because I let it generate thumb2, as well.)  It worked previously because we only enabled NEON for *hand-written assembly* in which we chose all the instructions.

If we could set CFLAGS just for a certain set of sources, maybe we could get that to work.  Anyone know if that's possible?

But either way we're going to break no-thumb2 builds because of the compiler bug.  So our options are (i) break armv6; (ii) if the per-targets CFLAGS hack works, break armv7-no-thumb.  I don't know who would own that kind of decision.  Does anyone else?
In particular, looks like iOS/ARM uses no-thumb mode by default.
.... which you already said and it went over my head :)

(In reply to Ralph Giles (:rillian) from comment #18)
> I did find in the manual, "floating-point operations are not generated by
> GCC's auto-vectorization pass unless -funsafe-math-optimizations is also
> specified" so it sounds like we are safe from -mfpu turning on non-portable
> code generation.

So because of the gcc bug, we're still forced to enable per-target CFLAGS for the files that use these intrinsics, and that forces us also to break the armv7-no-thumb configuration.
(In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need me from comment #22)

> -    CFLAGS += ['-mfpu=neon']
> +    CFLAGS += ['-march=armv7-a', '-mthumb', '-mfloat-abi=softfp',
> '-mfpu=neon']

Isn't rpi hard float?

> Error: unrecognized option -mfpu=neon-vfpv4

Ok, must need a newer gcc. That was from the gcc 4.8.3 manual.

> Unless I'm wrong, that has no chance of working because the compiler is free to generate NEON code for any arbitrary floating point expressions.

No, see comment 18 about -mfpu=neon.

However, you can't change the arch to something incompatible with arm6, or with the arch used for the rest of the build. I suspect that's responsible for at least some of the crashes.
(In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need me from comment #26)
> forces us also to break the armv7-no-thumb configuration.

Actually, I recall landing a patch that disabled thumb for a particular file to work around another bug.  So IIRC gcc can handle mixed-mode code in staticly-linked code.

So I think we're just down to the limitation of per-target CFLAGS for the neon-intrinsic-using code.
(In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need me from comment #25)
> In particular, looks like iOS/ARM uses no-thumb mode by default.

iOS support isn't actively being developed, so don't worry about it.
(In reply to Ralph Giles (:rillian) from comment #27)
> (In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need
> me from comment #22)
> 
> Isn't rpi hard float?
> 

Raspbian is hardfp.  FFOS/gonk is softfp for now.

> However, you can't change the arch to something incompatible with arm6, or
> with the arch used for the rest of the build. I suspect that's responsible
> for at least some of the crashes.

Do you mean for all the libvpx sources?  Yes, understood, that was initial "duh" realization.
*my initial "duh" realization
(In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need me from comment #30)

> Do you mean for all the libvpx sources?  Yes, understood, that was initial
> "duh" realization.

I mean you can't change the target arch for all the libvpx source to something incompatible. I suppose you could change the target for files which will never be invoked at runtime, assuming the linker can cope. But moz.build provides no way to do that, I think.

Is gcc 4.8 any better?
(In reply to Ralph Giles (:rillian) from comment #32)
> I suppose you could change the target for files
> which will never be invoked at runtime, assuming the linker can cope.

Yes, that seems to be the only option at this point, unless I'm missing something again.

> But
> moz.build provides no way to do that, I think.
>

I'd hate to drop support because of build system limitations ... is there anything we can do with building the NEON-intrinsic-using code in a subdir that has its own moz.build?
 
> Is gcc 4.8 any better?

That's a long story, but unfortunately compiler upgrade isn't on the table in the foreseeable future in my use case.  Anyone still trying to limp along with an armv6 build is most likely stuck with old gcc like I partially am.
(In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need me from comment #33)
> (In reply to Ralph Giles (:rillian) from comment #32)
> > I suppose you could change the target for files
> > which will never be invoked at runtime, assuming the linker can cope.
> 
> Yes, that seems to be the only option at this point, unless I'm missing
> something again.

Another option is to only build those files when mfpu >= "neon".  Then declare their exported symbols as __attribute__((weak)), and call them at runtime only when NEON is available *and* they're defined.  But we could never upstream that kind of hack, I suspect.
So apparently I was wrong. Per-source-file flags CAN be set in moz.build, it's just not documented. Try:

  SOURCES['foo.cpp'].cflags += ['-Dfoo']
Sweet!  That's what I would have guessed, even :).
The closest I was able to get was

----------
diff --git a/media/libvpx/moz.build b/media/libvpx/moz.build
index d278748..794992c 100644
--- a/media/libvpx/moz.build
+++ b/media/libvpx/moz.build
@@ -53,28 +53,31 @@ if CONFIG['VPX_X86_ASM']:
         CFLAGS += ['-msse4.1']
 
     #postproc is only enabled on x86 with asm
     SOURCES += files['VP8_POSTPROC']
 
 arm_asm_files = []
 if CONFIG['VPX_ARM_ASM']:
     arm_asm_files += files['ARM_ASM']
-    CFLAGS += ['-mfpu=neon']
-
     if CONFIG['VPX_AS_CONVERSION']:
         GENERATED_SOURCES += [ "%s.%s" % (f, CONFIG['VPX_ASM_SUFFIX'])
             for f in sorted(arm_asm_files) if f.endswith('.asm')
         ]
         SOURCES += [
             f for f in sorted(arm_asm_files) if not f.endswith('.asm')
         ]
     else:
         SOURCES += sorted(arm_asm_files)
 
+    # /** */
+    for f in arm_asm_files:
+        if 'neon' in f:
+            SOURCES[f].flags += ['-march=armv7-a', '-mthumb', '-mfloat-abi=softfp', '-mfpu=neon']
+
 # boolhuff_armv5te.asm defines the same functions as boolhuff.c instead of
 # using RTCD, so we have to make sure we only add one of the two.
 if 'vp8/encoder/arm/armv5te/boolhuff_armv5te.asm' not in arm_asm_files:
     SOURCES += [
         'vp8/encoder/boolhuff.c',
     ]
 
 MSVC_ENABLE_PGO = True
----------

which fails with

----------
The error was triggered on line 74 of this file:

    SOURCES[f].flags += ['-march=armv7-a', '-mthumb', '-mfloat-abi=softfp', '-mfpu=neon']

An error was encountered as part of executing the file itself. The error appears to be the fault of the script.

The error as reported by Python is:

    ['KeyError: u"\'vp8/encoder/arm/neon/fastquantizeb_neon.asm\'"\n']

----------

Is there perhaps a mozbuild person who could advise?
I expect cflags aren't allowed on asm files. Try:

  if 'neon' in f and f[:-2] == '.c'

CC'ing gps for moz.build expertise.
Good point, I didn't notice that a .asm instead of .c.  We'll need to set the assembler flags for those too, though.

I would also like to know if this a workaround that has a chance for r+.  If we're going to r- this over style/maintainability, let's just be up front about it and move on ;).  Who would review, rillian or gps or both?
I don't object to a patch like comment 37, which just does a little more more build flag decoration, but I wouldn't like to rearrange the whole build like we were talking about before.

Your patch would need media peer review (e.g. myself) and build peer review for moz.build/Makefile changes (Ted Mielczarek has been handling that for this code).
Note that not accepting a patch here would basically drop support for armv6, or drop it to Tier III.  Probably a discussion to have on moz.dev.(platform? not sure).
Yep.
I would r+ a patch like that, it's fairly reasonable. Style-wise I would probably write something like:
if 'neon' in f and f.endswith('.c'):
Blocks: 1087096
Duplicate of this bug: 1089411
in addition of trying to find the bug in the jungle of available ARM models and FPUs (this will probably take a long time to get it right for every possible combination), please give us a mozconfig option to *disable the ARM asm* completely, and to use the portable C/C++ code instead. thanks.
I tried to post about this (rather worrying) bug to dev-platform, but my post was caught in moderation and not passed through yet.
Sorry, I thought you had a patch. Did comment 37 & seq. not work? :vlui did try it on flatfish and said it didn't work (internal compiler error again). Are we back to toolchain bumps?
Flags: needinfo?(cjones.bugs)
No, if I had a patch I would have posted it ;).

See comment 37: |SOURCES[f]| where |f| is a string source filename throws the error
----------
The error was triggered on line 74 of this file:

    SOURCES[f].flags += ['-march=armv7-a', '-mthumb', '-mfloat-abi=softfp', '-mfpu=neon']

An error was encountered as part of executing the file itself. The error appears to be the fault of the script.

The error as reported by Python is:

    ['KeyError: u"\'vp8/encoder/arm/neon/fastquantizeb_neon.asm\'"\n']
----------

I couldn't make any progress debugging this further because it seems all the usual python introspection helpers are disabled in moz.build.

But even worse, I don't know if this hack (if it could be made to work) would be supported going forward.  So I'm stuck.  Hence the m.d.platform post.
Flags: needinfo?(cjones.bugs)
Note that this error you're getting contradicts what you wrote to dev-platform: you said there that the problem was that the code switched to using plain C files. fastquantizeb_neon.asm is not a C file.
Also, what this suggests, is that you're on the wrong side of the VPX_AS_CONVERSION check. Check media/libvpx/moz.build, you'll see that vp8/encoder/arm/neon/fastquantizeb_neon.asm is NOT in SOURCES when VPX_AS_CONVERSION is defined, but in GENERATED_SOURCES, with an additional suffix.
I think the intention is that only *_neon.c files are compiled with -mfpu=neon.
See "if 'ssse3' in f" in moz.build.

If CONFIG['BUILD_ARM_NEON'] is not set, then no neon files should be compiled, which I guess would require a vpx_config_armv6-android-gcc.h or similar
(I'm not sure about the neon / arm version correlation), but maybe that's an optional extra.
(In reply to Mike Hommey [:glandium] from comment #49)
> Note that this error you're getting contradicts what you wrote to
> dev-platform: you said there that the problem was that the code switched to
> using plain C files.

It doesn't, but I'm glad you took the time to catch me in a lie anyway!  Very helpful.

(In reply to Karl Tomlinson (:karlt) from comment #51)
> I think the intention is that only *_neon.c files are compiled with
> -mfpu=neon.
> See "if 'ssse3' in f" in moz.build.

OK perfect :).  That's exactly what I was looking for.  Thanks Karl!
Comment on attachment 8518508 [details] [diff] [review]
Enable NEON intrinsics for C files when even when building for ARMv6

Review of attachment 8518508 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me. Thanks!

Adding Ted for build peer review.
Attachment #8518508 - Flags: review?(ted)
Attachment #8518508 - Flags: review?(giles)
Attachment #8518508 - Flags: review+
Attachment #8518508 - Flags: review?(ted) → review+
Assignee: nobody → cjones.bugs
btw this also affects armv5tl with a different error (see bug 1089411)

i'm still searching for a way to turn all this "zomg optimized" asm off so i can build the raw, portable C code without running into errors when using an ARM fpu/cpu combination that wasn't tested yet...
(In reply to maillist-mozilla from comment #57)

> i'm still searching for a way to turn all this "zomg optimized" asm off so i
> can build the raw, portable C code without running into errors when using an
> ARM fpu/cpu combination that wasn't tested yet...

This isn't something mozilla's build system support directly. You'll need to add something like --disable-neon to the configure invocation in media/libvpx/update.py and rerun it to update the generated headers.
Blocks: 1089411
Duplicate of this bug: 1087359
[Tracking Requested - why for this release]: This is needed to fix WebM playback in Firefox for Android
https://hg.mozilla.org/mozilla-central/rev/ae75658cfabf
Status: NEW → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
This fix seems broke compilation on Mer SDK, with hardfp compiler
34:21.70 /opt/cross/bin/armv7hl-meego-linux-gnueabi-ld: error: /home/tatiana/build/xulrunner-package-jolla/objdir-mer/toolkit/library/../../media/libvpx/bilinearpredict_neon.o uses VFP register arguments, output does not
....
(In reply to Ralph Giles (:rillian) from comment #58)
> (In reply to maillist-mozilla from comment #57)
> 
> > i'm still searching for a way to turn all this "zomg optimized" asm off so i
> > can build the raw, portable C code without running into errors when using an
> > ARM fpu/cpu combination that wasn't tested yet...
> 
> This isn't something mozilla's build system support directly. You'll need to
> add something like --disable-neon to the configure invocation in
> media/libvpx/update.py and rerun it to update the generated headers.

hmm it was possible to activate the asm stuff from the mozilla build system, why is it not possible to deactivate it again given some switch ? (maybe a top-level "--disable-vpx-asm" which is then passed as --disable-neon to the vpx configure invocation)
It was compiling before, but now moz.build flags contain hardcoded =softfp - which is not good, because toolchain should specify defaults for that flag.
(In reply to Tatiana Meshkova (:tatiana) from comment #64)
> It was compiling before, but now moz.build flags contain hardcoded =softfp -
> which is not good, because toolchain should specify defaults for that flag.

It would. Please open a new bug with your specific build config.
Flags: needinfo?(tanya.meshkova)
If this is needed to fix WebM on Android, please nominate for beta uplift by Mon Dec 22 so it can be in next week's beta.
Flags: needinfo?(cjones.bugs)
Don't know what webm issue is being referred to, so no idea.
Flags: needinfo?(cjones.bugs)
(In reply to Chris Jones [:cjones] inactive; ni?/f?/r? if you need me from comment #67)
> Don't know what webm issue is being referred to, so no idea.

Please look at the duplication list.

https://bugzilla.mozilla.org/show_bug.cgi?id=1087359
Chris - Aaron pointed out what the issue was, we have one mobile beta remaining for 35.  If this patch is low risk to take, can you please nominate for uplift to aurora/beta before Monday Jan 5th?
Flags: needinfo?(cjones.bugs)
This patch is on the aurora branch, having landed on m-c when it was Firefox 36.
Approval Request Comment
[Feature/regressing bug #]: 1105858
[User impact if declined]: Cannot build for armv6 devices.
[Describe test coverage new/current, TBPL]: Landed on m-c, aurora.

[Risks and why]: This changes the compilation options for the neon code. It should generate correct code regardless of whether the target will use it at run time. Main risk is compatibility with older toolchains, but this has been fine on ff 36 and 37 builds.

[String/UUID change made/needed]: None.
Attachment #8544182 - Flags: approval-mozilla-beta?
Comment on attachment 8544182 [details] [diff] [review]
Backport to Firefox 35

Thanks for the nomination - let's get this into the final beta for 35.
Attachment #8544182 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(cjones.bugs)
Clearing old requests.
Flags: needinfo?(tanya.meshkova)
You need to log in before you can comment on or make changes to this bug.