850041 - B2G's Android (Gonk) build doesn't use CFLAGS and CXXFLAGS from .userconfig

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

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

I've been working on getting frame pointers for profiler stack walking (bug 831611 and bug 810526), and it would help to be able to set additional compiler flags for the Android build.  In the limit this can be accomplished with wrapper scripts or even by replacing parts of the prebuilt toolchain in place (the latter is what I'm currently doing), but it would be good if there were a less invasive way.  The flags in .userconfig work for the Gecko build, but not the Android bits, and I need the entire system to follow the frame-pointer ABI to get unbroken stacks.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Mike, Dave, any suggestions on how to move forward here?

Comment 2

[Dave Hylands [:dhylands]]

I took a quick look and for android, it looks like you might be able to use

TARGET_GLOBAL_CFLAGS and TARGET_GLOBAL_CPPFLAGS

I've added mwu in case he has any better ideas

Comment 3

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

TARGET_GLOBAL_CFLAGS does not seem to be it.  I built using a -include flag to force-include module-level asm to put a known string in the .rodata (to be reasonably unambiguous about whether my flags are actually being used), which works for Gecko via CFLAGS/CXXFLAGS, but nothing else.

Comment 4

[Dave Hylands [:dhylands]]

TARGET_GLOBAL_CFLAGS would only apply to .c files
TARGET_GLOBAL_CPPFLAGS applies to .cpp files.

I added:

export TARGET_GLOBAL_CPPFLAGS=-DDAVE_HYLANDS

to my .userconfig, and then did:

touch system/vold/VoldCommand.cpp
./build.sh showcommands

and I see:

prebuilt/linux-x86/ccache/ccache prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/arm-linux-androideabi-g++ -I bionic/libc/kernel/common -I bionic/libc/kernel/arch-arm -I system/extras/ext4_utils -I external/openssl/include -I system/vold -I out/target/product/unagi/obj/EXECUTABLES/vold_intermediates -I dalvik/libnativehelper/include/nativehelper -isystem system/core/include -isystem hardware/libhardware/include -isystem hardware/libhardware_legacy/include -isystem hardware/ril/include -isystem dalvik/libnativehelper/include -isystem frameworks/base/include -isystem frameworks/base/opengl/include -isystem frameworks/base/native/include -isystem external/skia/include -isystem out/target/product/unagi/obj/include -isystem bionic/libc/arch-arm/include -isystem bionic/libc/include -isystem bionic/libstdc++/include -isystem bionic/libm/include -isystem bionic/libm/include/arm -isystem bionic/libthread_db/include -c  -fno-exceptions -Wno-multichar -msoft-float -fpic -ffunction-sections -fdata-sections -funwind-tables -fstack-protector -Wa,--noexecstack -Werror=format-security -fno-short-enums -march=armv7-a -mfloat-abi=softfp -mfpu=neon -include system/core/include/arch/linux-arm/AndroidConfig.h -I system/core/include/arch/linux-arm/ -Wno-psabi -mthumb-interwork -DANDROID -fmessage-length=0 -W -Wall -Wno-unused -Winit-self -Wpointer-arith -Werror=return-type -Werror=non-virtual-dtor -Werror=address -Werror=sequence-point -DNDEBUG -g -Wstrict-aliasing=2 -fgcse-after-reload -frerun-cse-after-loop -frename-registers -DNDEBUG -UDEBUG -DDAVE_HYLANDS -fvisibility-inlines-hidden -DANDROID -fmessage-length=0 -W -Wall -Wno-unused -Winit-self -Wpointer-arith -Wsign-promo -Werror=return-type -Werror=non-virtual-dtor -Werror=address -Werror=sequence-point -DNDEBUG -UDEBUG -mthumb -Os -fomit-frame-pointer -fno-strict-aliasing  -fno-rtti      -MD -MF out/target/product/unagi/obj/EXECUTABLES/vold_intermediates/VoldCommand.d -o out/target/product/unagi/obj/EXECUTABLES/vold_intermediates/VoldCommand.o system/vold/VoldCommand.cpp

So it seems to be working. I noticed that it doesn't rebuild the world when you do this.

- make sure you export TARGET_GLOBAL_CPPFLAGS (and TARGET_LOCAL_CFLAGS) from .userconfig
- do "rm -rf out" to force everything to be rebuilt.

The showcommands target is a psuedo-target which will cause the android build commands to be shown.

Comment 5

[Dave Hylands [:dhylands]]

Hmm. I tried setting both using:

export TARGET_GLOBAL_CFLAGS=-DDAVE_HYLANDS_C
export TARGET_GLOBAL_CPPFLAGS=-DDAVE_HYLANDS_CPP

and then touched system/core/sdcard/sdcard.c and I don't see the cflags added.

Comment 6

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

From a cursory reading of the makefiles, I think that CPPFLAGS in this case might mean C preprocessor flags, as it usually does.

Comment 7

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

So, I ran the build under strace at -j1, and I can see that the top-level make process is forking and execing things with TARGET_GLOBAL_CFLAGS in the environment... and then the execve calls stop having TARGET_GLOBAL_CFLAGS in them.

Recursive grep on the makefiles isn't enlightening; nor is ltrace.

Comment 8

[Dave Hylands [:dhylands]]

It seems that androids build/core/select.mk contains these lines:

$(combo_target)GLOBAL_CFLAGS := -fno-exceptions -Wno-multichar
$(combo_target)RELEASE_CFLAGS := -O2 -g -fno-strict-aliasing
$(combo_target)GLOBAL_LDFLAGS :=
$(combo_target)GLOBAL_ARFLAGS := crsP

which causes anything which sets TARGET_GLOBAL_CFLAGS in the environment to be wiped out

Comment 9

[Dave Hylands [:dhylands]]

So I think that if you wanted to add something that could be controlled via .userconfig then you would need to create a new variable and modify build/core/combo/TARGET_linux-arm.mk to add that new variable to TARGET_GLOBAL_CFLAGS

As far as I can tell TARGET_GLOBAL_CFLAGS is passed to compile both C and C++ files, whereas TARGET_GLOBAL_CPPFLAGS is only passed to C++ files. See build/core/definitions.mk and search for transform-cpp-to-o and transform-c-or-s-to-o-no-deps

It appears that this file has already had modifications by us, so I would assume its ok to add some more.

Comment 10

[Dave Hylands [:dhylands]]

I went ahead and merged this:
https://github.com/mozilla-b2g/platform_build/pull/19
https://github.com/mozilla-b2g/platform_build/commit/7fce578c221291b1cbd3ccaae2838bf57a39cd7c

and afterwards, I realized it was v1-train which I think requires some type of approval which this bug doesn't have.

This is one of those extremely low-risk changes, so I'll ask for approval after the fact.

Comment 11

[Dave Hylands [:dhylands]]

Jed - can you prepare a PR for master as well?

Comment 12

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

Done: https://github.com/mozilla-b2g/platform_build/pull/20

Comment 13

[Dave Hylands [:dhylands]]

ok - merged into master:
https://github.com/mozilla-b2g/platform_build/commit/f9f0e383f25088d279e356ea4c36d60aa31fd46a