Closed Bug 999348 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S2 (23may)

People

(Reporter: iean.lin, Assigned: jseward)

References

Details

Attachments

(2 files, 1 obsolete file)

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
Add "DEFINES['ELFSIZE'] = 32" to gecko/tools/profiler/moz.build ?
Depends on: 938157
What hardware is flatfish, approximately?
I think LulElf.cpp issue fixed in bug 997700
My bad, looks different build error in same file.
(In reply to Julian Seward from comment #2)
> What hardware is flatfish, approximately?

It's some type of 32-bit ARM, if that helps.
(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.
Depends on bug 999902 because that new failure comes first.
Depends on: 999902
I confirm this bug on Debian testing 64bit.

Workaround on comment 6 works for me too.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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?
Flags: needinfo?(jseward)
Duplicate of this bug: 1005007
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
Flags: needinfo?(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
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?
Mwu, see comment 13
Flags: needinfo?(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 ?
Flags: needinfo?(mwu)
Attached patch A possible fix (obsolete) — Splinter Review
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.
Attachment #8419073 - Flags: feedback?(mwu)
Duplicate of this bug: 1006952
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 on attachment 8419073 [details] [diff] [review]
A possible fix

Looks fine, though checking for ANDROID_VERSION <= 17 is probably useful than checking CPU_ARCH.
Attachment #8419073 - Flags: feedback?(mwu) → feedback+
Verified with builds for both Flatfish and Nexus5.
Attachment #8419073 - Attachment is obsolete: true
Attachment #8419699 - Flags: review?(mwu)
Attachment #8419699 - Flags: review?(mwu) → review?(mh+mozilla)
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?
Attachment #8419699 - Flags: review?(mh+mozilla) → review+
(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?
Flags: needinfo?(mh+mozilla)
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
Flags: needinfo?(mh+mozilla)
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.
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/59c2d4b0bb8b
Assignee: nobody → jseward
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S2 (23may)
You need to log in before you can comment on or make changes to this bug.