999348 - [flatfish] Build faild in gecko/tools/profiler/LulElf.cpp

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Iean Lin]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140317233623

Steps to reproduce:

When I build the images of flatfish, it ends with errors on compiling gecko/tools/profiler/LulElf.cpp

Build command:
GAIA_DISTRIBUTION_DIR=distribution_tablet B2G_SYSTEM_APPS=1 B2G_UPDATER=1 ./build.sh


Actual results:

Error messages:
gecko/tools/profiler/LulElf.cpp: In function 'int lul::ElfClass(void const*)':
gecko/tools/profiler/LulElf.cpp:791:9: error: 'ElfELFSIZE_Ehdr' does not name a type
gecko/tools/profiler/LulElf.cpp:794:10: error: 'elf_header' was not declared in this scope

Comment 1

[Iean Lin]

Add "DEFINES['ELFSIZE'] = 32" to gecko/tools/profiler/moz.build ?

Comment 2

[Julian Seward [:jseward]]

What hardware is flatfish, approximately?

Comment 3

[Wayne Chen [:xwaynec]]

I think LulElf.cpp issue fixed in bug 997700

Comment 4

[Wayne Chen [:xwaynec]]

My bad, looks different build error in same file.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Julian Seward from comment #2)
> What hardware is flatfish, approximately?

It's some type of 32-bit ARM, if that helps.

Comment 6

[Julian Seward [:jseward]]

(In reply to Iean Lin from comment #1)
> Add "DEFINES['ELFSIZE'] = 32" to gecko/tools/profiler/moz.build ?

This failure happened because ELFSIZE isn't defined.  It should
be defined in /usr/include/sys/exec_elf.h from the NDK, as a copy of
ARCH_ELFSIZE, which is defined in usr/include/machine/exec.h.  And
/usr/include/sys/exec_elf.h is included by /usr/include/elf.h, which
is included into LuLElfInt.h.

At least, that's the situation in
android-ndk-r8e/platforms/android-8/arch-arm/usr/include/machine/exec.h

So I'm inclined to think it's some kind of toolchain problem.

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Depends on bug 999902 because that new failure comes first.

Comment 8

[Alfredos-Panagiotis Damkalis [:fredy]]

I confirm this bug on Debian testing 64bit.

Workaround on comment 6 works for me too.

Comment 9

[Vincent Liu[:vliu]]

(In reply to Julian Seward from comment #6)
> (In reply to Iean Lin from comment #1)
> > Add "DEFINES['ELFSIZE'] = 32" to gecko/tools/profiler/moz.build ?
> 
> This failure happened because ELFSIZE isn't defined.  It should
> be defined in /usr/include/sys/exec_elf.h from the NDK, as a copy of
> ARCH_ELFSIZE, which is defined in usr/include/machine/exec.h.  And
> /usr/include/sys/exec_elf.h is included by /usr/include/elf.h, which
> is included into LuLElfInt.h.
> 
> At least, that's the situation in
> android-ndk-r8e/platforms/android-8/arch-arm/usr/include/machine/exec.h
> 
> So I'm inclined to think it's some kind of toolchain problem.

I also got the same build break on Vixen platform which is also based on Android JB 4.2.2. For the info till now, do you have any idea with that?

Comment 11

[Julian Seward [:jseward]]

An observation from Mike Hommey:
<glandium> the obvious fail is that there is no ndk path on the command line
<glandium> so it must be taking elf.h from bionic
<glandium> which is useless
<glandium> sewardj: so i'd suggest to look at the compile lines on b2g logs for other devices builds
<glandium> see what's different

Comment 12

[Julian Seward [:jseward]]

I compared the build command lines for LulElf.cpp for Nexus5 and Flatfish.
Nexus5 build succeeds, Flatfish doesn't.  There are no ndk-include related
differences.  The only differences are:

* Flatfish is built with gcc 4.6, Nexus5 with gcc 4.7:
* Flatfish has -I of external/dbus and external/bluetooth/bluez/lib,
  Nexus5 doesn't
* Nexus5 has -mno-unaligned-access, Flatfish doesn't.

So I'm not sure where that leads.  Is the 4.6 vs 4.7 difference
significant?


The full diff is:

< Flatfish
---
> Nexus5

< /home/sewardj/B2G_/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.6/bin/arm-linux-androideabi-g++
---
> /home/sewardj/B2G_/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.7/bin/arm-linux-androideabi-g++

< -I/home/sewardj/B2G_/external/dbus
< -I/home/sewardj/B2G_/external/bluetooth/bluez/lib

< -I/home/sewardj/B2G_/external/dbus
< -I/home/sewardj/B2G_/external/bluetooth/bluez/lib

> -mno-unaligned-access

Comment 13

[Julian Seward [:jseward]]

Attachment: LulElf.cpp build command lines for Flatfish and Nexus5

I'm pretty much lost and would appreciate some guidance.  I don't
understand how the build system works here.  In particular:

(1) which /usr/include/*.h is the build supposed to use?  Is it
       bionic/libc/include, 
    or prebuilds/ndk/*/platforms/android-*/arch-arm/usr/include
    or something else?

(2) How do I know which it is actually using?  Looking at the
    attached Flatfish command line, I see this
       -isystem
       /home/sewardj/B2G/bionic/libc/arch-arm/include
       -isystem
       /home/sewardj/B2G/bionic/libc/include/
    quite early on.  Does that mean it is using bionic includes
    for elf.h?  Should it be using ndk includes instead?

(3) If it is using bionic includes, then -- given comment 12 -- it
    seems likely that both Nexus5 and Flatfish are using the bionic
    includes.  In that case, why does the Nexus5 build work but the
    Flatfish build fail?  Do the two builds use different versions of
    the bionic headers?

Comment 14

[Mike Hommey [:glandium]]

Mwu, see comment 13

Comment 15

[Michael Wu [:mwu]]

1. Within the Android build system, there really isn't a /usr/include/ directory with all includes unless you're using the ndk include directory, which only gives you a small selection of headers. Usually the local build files (Android.mk, moz.build) will add additional paths to get all the necessary headers.

That being said, I think bionic/libc/include would be preferred over ndk include directories when doing a B2G/Gonk build.

2. Sounds like it's using bionic includes. You can verify for sure by checking the preprocessed output from GCC.

3. I think the bionic includes that comes with flatfish is broken. Flatfish is based on 4.2.2 while Nexus 5 is based on 4.4. I think they fixed this ELFSIZE issue sometime between 4.2.2 and 4.3, since I remember a similar build issue in gecko. Maybe bug 737190 ?

Comment 16

[Julian Seward [:jseward]]

Attachment: A possible fix

Mwu, thank you for the details in comment 15.  Attached is a proposed
fix.  I added the extra

  and CONFIG['CPU_ARCH'] == 'arm'

in an attempt to guard against a future scenario where we build Gonk
in 64 bits since in that case we definitely don't want to specify an
ELFSIZE of 32.

This fix works for both Flatfish and Nexus5.  In the non-broken cases
(eg Nexus5) I was concerned that the added -DELFSIZE=32 might cause
g++ to complain about redefinition of ELFSIZE that it picks up from
/usr/include/, or the other way round.  But that didn't appear to be a
problem, at least for a Nexus5 build.

Comment 18

[Juan Gomez [:_AtilA_] (CET/CEST)]

Micheal was right,
ELFSIZE is defined in the Bionic headers, and they were changed about year and a half ago (since android-4.3_r0.9) to better support ELF: https://android-review.googlesource.com/#/c/50780/

Upgrading bionic to refs/tags/android-4.3_r2.1 (which include ELFSIZE definition), solve this issue but causes compile errors later in other components so the approach in comment 16 looks fine so far.

Comment 19

[Michael Wu [:mwu]]

Comment on attachment 8419073 [details] [diff] [review]
A possible fix

Looks fine, though checking for ANDROID_VERSION <= 17 is probably useful than checking CPU_ARCH.

Comment 20

[Julian Seward [:jseward]]

Attachment: Revised fix, per comment 19

Verified with builds for both Flatfish and Nexus5.

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8419699 [details] [diff] [review]
Revised fix, per comment 19

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

::: tools/profiler/moz.build
@@ +96,5 @@
>      if CONFIG['ENABLE_TESTS']:
>          DIRS += ['tests/gtest']
>  
> +    if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk' and CONFIG['ANDROID_VERSION'] <= '17':
> +        DEFINES['ELFSIZE'] = 32

Why not do it in code?

Comment 22

[Julian Seward [:jseward]]

(In reply to Mike Hommey [:glandium] from comment #21)
> Why not do it in code?

I did it in the build system because that's how the same problem was
fixed in bug 737190 that Mwu refers to in comment #15.  Would you
prefer it to be fixed in code?

Comment 23

[Mike Hommey [:glandium]]

I don't care that much, but at least in code, you could do something like:
#ifndef ELFSIZE
#ifdef __LP64__
#define ELFSIZE 64
#else
#define ELFSIZE 32
#endif
#endif

Comment 24

[Francis Lee [:frlee]]

hi All,

as you know, for our TCP program, we still have Vixen to deliver. this issue actually blocks Vixen's progress. Set this issue to P1. please kindly resolve this issue as early as possible.

Comment 25

[Julian Seward [:jseward]]

https://hg.mozilla.org/integration/b2g-inbound/rev/59c2d4b0bb8b

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/59c2d4b0bb8b