Closed
Bug 999348
Opened 11 years ago
Closed 11 years ago
[flatfish] Build faild in gecko/tools/profiler/LulElf.cpp
Categories
(Firefox OS Graveyard :: General, defect, P1)
Firefox OS Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
2.0 S2 (23may)
People
(Reporter: iean.lin, Assigned: jseward)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
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 ?
Assignee | ||
Comment 2•11 years ago
|
||
What hardware is flatfish, approximately?
Comment 3•11 years ago
|
||
I think LulElf.cpp issue fixed in bug 997700
Comment 4•11 years ago
|
||
My bad, looks different build error in same file.
Comment 5•11 years ago
|
||
(In reply to Julian Seward from comment #2)
> What hardware is flatfish, approximately?
It's some type of 32-bit ARM, if that helps.
Assignee | ||
Comment 6•11 years ago
|
||
(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•11 years ago
|
||
Depends on bug 999902 because that new failure comes first.
Depends on: 999902
Comment 8•11 years ago
|
||
I confirm this bug on Debian testing 64bit.
Workaround on comment 6 works for me too.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 9•11 years ago
|
||
(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)
Assignee | ||
Comment 11•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(jseward)
Assignee | ||
Comment 12•11 years ago
|
||
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
Assignee | ||
Comment 13•11 years ago
|
||
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 15•11 years ago
|
||
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)
Assignee | ||
Comment 16•11 years ago
|
||
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)
Comment 18•11 years ago
|
||
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•11 years ago
|
||
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+
Assignee | ||
Comment 20•11 years ago
|
||
Verified with builds for both Flatfish and Nexus5.
Attachment #8419073 -
Attachment is obsolete: true
Attachment #8419699 -
Flags: review?(mwu)
Updated•11 years ago
|
Attachment #8419699 -
Flags: review?(mwu) → review?(mh+mozilla)
Comment 21•11 years ago
|
||
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+
Assignee | ||
Comment 22•11 years ago
|
||
(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?
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(mh+mozilla)
Comment 23•11 years ago
|
||
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)
Comment 24•11 years ago
|
||
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
Assignee | ||
Comment 25•11 years ago
|
||
Assignee: nobody → jseward
Status: NEW → RESOLVED
Closed: 11 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.
Description
•