classification
Title: android compilation and link flags
Type: enhancement Stage: resolved
Components: Cross-Build Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: xdegaye Nosy List: Alex.Willmer, doko, martin.panter, python-dev, twouters, vstinner, xdegaye, yan12125
Priority: normal Keywords: patch

Created on 2016-04-26 12:20 by xdegaye, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
build-flags.patch xdegaye, 2016-04-26 12:20 review
build-flags_2.patch xdegaye, 2016-07-24 16:41 review
build-flags_3.patch xdegaye, 2016-07-25 09:31
build-flags_3.patch xdegaye, 2016-07-25 14:13 uploading twice the same file review
build-flags_4.patch xdegaye, 2016-07-26 09:43 review
build-flags_5.patch xdegaye, 2016-10-15 09:26 review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (19)
msg264266 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2016-04-26 12:20
The attached patch:

  * Sets the recommended android compilation flags, see: http://developer.android.com/ndk/guides/standalone_toolchain.html#abi.  Note that the android toolchains already set the -fpic flag, as shown with:

    arm-linux-androideabi-gcc -v -S main.c 2>&1 | grep main.c

  * Sets the Position independent executables (PIE) flag which is mandatory starting at API level 21 and supported starting with API level 16.
msg271169 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2016-07-24 16:41
The previous patch was using awkwardly, a 'host_cpu' configure argument to specify the arm ABI and missed setting LDFLAGS for the armv7 ABI.
This patch instead uses the __ARM_ARCH macro defined by each compiler of the Android toolchains:

    echo | ./clang -target armv7-none-linux-androideabi -dM -E - | grep ARM_ARCH
      #define __ARM_ARCH 7
      #define __ARM_ARCH_7A__ 1
    echo | ./clang -target armv5te-none-linux-androideabi -dM -E - | grep ARM_ARCH
      #define __ARM_ARCH 5
      #define __ARM_ARCH_5TE__ 1
    echo | ./arm-linux-androideabi-gcc -march=armv7-a -dM -E - | grep ARM_ARCH
      #define __ARM_ARCH 7
      #define __ARM_ARCH_7A__ 1
    echo | ./arm-linux-androideabi-gcc -dM -E - | grep ARM_ARCH
      #define __ARM_ARCH_5TE__ 1
      #define __ARM_ARCH 5

Python built with clang for the armv5te target crashes as reported in issue 27606.
msg271193 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-07-25 00:44
I left some suggestions and questions on the code review.
msg271246 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2016-07-25 09:31
Thanks for the review and the suggestions Martin :)
New patch.
msg271275 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2016-07-25 14:17
The first 'build-flags_3.patch' file that was uploaded did not get a review button because ssh://hg@hg.python.org/cpython was down at that time.

The second upload has been successfull !
msg271282 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-07-25 15:19
Note: build-flags_3.patch includes configure.ac and configure. Usually, we suggest to not included generated files (configure) in patches, but push them when the patch is ready.
msg271292 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2016-07-25 16:05
Thanks for the suggestion, I won't include them anymore in patches then.
msg271353 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2016-07-26 09:43
New patch. The sed commands used to evaluate ANDROID_API_LEVEL and _arm_arch do not need to discard comment lines and empty lines in the output of the preprocessor.
msg271518 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-07-28 11:01
All the bits that I understand look okay now. :)

I am still curious what configures the preprocessor to set __ARM_ARCH to 7 (I guess the clang -target argument?), and why we can’t set LDFLAGS at the same time or place. Is it just more convenient this way?
msg271530 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2016-07-28 11:37
> I am still curious what configures the preprocessor to set __ARM_ARCH to 7 (I guess the clang -target argument?)
Yes, the -target clang argument or the -march gcc argument.

> and why we can’t set LDFLAGS at the same time or place. Is it just more convenient this way?
I don't understand the question. LDFLAGS is set at the time we know that __ARM_ARCH is 7, just after the preprocessing is done. Would you set it elsewhere ?
LDFLAGS is set in configure.ac with always this same idiom: LDFLAGS="$LDFLAGS new_options..." and this setting is done in what seems to be random places in configure.ac.
CCSHARED is set in one case statement.
msg271543 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-07-28 13:03
Where is the code that sets the clang -target argument, or gcc -march? Is it hidden away somewhere in the autoconf code? Do you manually set it with ‘./configure CFLAGS="-target . . ." ’?
msg271552 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2016-07-28 14:21
Yes, the packager must use appropriately either CFLAGS CPPFLAGS [1] and LDFLAGS, or CC. I am using:
CC="clang --sysroot=$(SYSROOT) -target $(TARGET) -gcc-toolchain $(GCC_TOOLCHAIN)".

[1] See issue 27453, for the remaining problem upon using CPPFLAGS.
msg278708 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2016-10-15 08:56
See also the clang cross compilation documentation at http://clang.llvm.org/docs/CrossCompilation.html.
msg278712 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2016-10-15 09:26
This new patch fixes the following:
* Use BASECFLAGS instead of CCSHARED.
* '-march=armv7-a' is not set in BASECFLAGS anymore as this is redundant: this option is already set in the configure command line or is implicitly set by the the first component of the triple of the '-target' clang option.
* Do not set '-mthumb' as this is optional and may be set for the armv7 arch as well. Also this option crashes Python on armv5te when built with clang, issue 27606.
msg284667 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2017-01-04 20:54
New changeset fa2bc63e64c6 by Xavier de Gaye in branch '3.6':
Issue #26851: Set Android compilation and link flags.
https://hg.python.org/cpython/rev/fa2bc63e64c6

New changeset af363b5200ff by Xavier de Gaye in branch 'default':
Issue #26851: Merge 3.6.
https://hg.python.org/cpython/rev/af363b5200ff
msg284668 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-01-04 20:58
Latest patch committed with the following simplification: BASECFLAGS and LDFLAGS are now set for armv7 at the same place, as suggested by Martin in msg271518.
msg284739 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-01-05 12:18
In the code review Victor asked the following question:

> I'm a little bit surprised that you need to pass so much options which are specific to the platform. GCC doesn't have a generic option "hello, please compile for my architecture?"

ARM defines two incompatible floating point ABIS that specify the functions calling conventions, whether to use integer registers (soft floating point ABI) or the floating point registers (hard floating point ABI).  The compiler must be told which one to use and whether the hardware FPU may be used if present when the soft floating point ABI has been chosen. This last case is set with the command line option '-mfloat-abi=softfp'. The following two documents describe this feature in details:
  https://wiki.debian.org/ArmHardFloatPort/VfpComparison
  https://wiki.debian.org/ArmHardFloatPort

The following command prints the command line options, including implicitly predefined ones of the clang compiler in android-ndk-r13b and shows that the default is '-mfloat-abi=soft' for armv7:

$ ./clang -x c < /dev/null -dM -E - -target armv7-none-linux-androideabi -###
Android clang version 3.8.256229  (based on LLVM 3.8.256229)
Target: armv7-none-linux-android
Thread model: posix
InstalledDir: /opt/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/.
 "/home/opt.symlink/android-ndk-r13b/toolchains/llvm/prebuilt/linux-x86_64/bin/clang" "-cc1" "-triple" "armv7-none-linux-android" "-E" "-disable-free" "-disable-llvm-verifier" "-main-file-name" "-" "-mrelocation-model" "pic" "-pic-level" "1" "-mthread-model" "posix" "-mdisable-fp-elim" "-fmath-errno" "-masm-verbose" "-mconstructor-aliases" "-fuse-init-array" "-target-cpu" "cortex-a8" "-target-feature" "+soft-float-abi" "-target-abi" "aapcs-linux" "-mfloat-abi" "soft" "-target-linker-version" "2.24" "-dwarf-column-info" "-debugger-tuning=gdb" "-resource-dir" "/home/opt.symlink/android-ndk-r13b/toolchains/llvm/prebuilt/linux-x86_64/bin/../lib64/clang/3.8.256229" "-internal-isystem" "/usr/local/include" "-internal-isystem" "/home/opt.symlink/android-ndk-r13b/toolchains/llvm/prebuilt/linux-x86_64/bin/../lib64/clang/3.8.256229/include" "-internal-externc-isystem" "/include" "-internal-externc-isystem" "/usr/include" "-fdebug-compilation-dir" "/opt/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/bin" "-ferror-limit" "19" "-fmessage-length" "100" "-femulated-tls" "-fallow-half-arguments-and-returns" "-fno-signed-char" "-fobjc-runtime=gcc" "-fdiagnostics-show-option" "-fcolor-diagnostics" "-dM" "-o" "-" "-x" "c" "-"

Note that the system libffi must also be compiled with these same flags. The bundled libffi is deprecated in 3.6 by issue 27976 and has been removed in 3.7 by issue 27979.
msg284745 - (view) Author: Chih-Hsuan Yen (yan12125) * Date: 2017-01-05 13:45
> Note that the system libffi must also be compiled with these same flags

Just tried. With my packaging scripts, CPython on ARM is compiled with -mfloat-abi=softfp -mfpu=vfpv3-d16 while libffi not. test_ctypes pass as usual. Maybe ctypes test suite is not complete?

And by the way, if all packages on a system should be compiled with the same set of flags, shouldn't those flags specified in build/package scripts rather than Makefile of individual packages?
msg284752 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-01-05 15:40
> Just tried. With my packaging scripts, CPython on ARM is compiled with -mfloat-abi=softfp -mfpu=vfpv3-d16 while libffi not. test_ctypes pass as usual.

This works as expected, '-mfloat-abi=softfp' and the default '-mfloat-abi=soft' use the same ABI [1]: "Function calls are generated to pass FP arguments (float, double) in integer registers (one for float, a pair of registers for double)".

[1] https://wiki.debian.org/ArmHardFloatPort/VfpComparison
History
Date User Action Args
2017-03-31 16:36:19dstufftsetpull_requests: + pull_request932
2017-01-05 15:40:29xdegayesetmessages: + msg284752
2017-01-05 13:45:03yan12125setmessages: + msg284745
2017-01-05 12:18:37xdegayesetmessages: + msg284739
2017-01-04 20:58:21xdegayesetstatus: open -> closed
resolution: fixed
messages: + msg284668

stage: patch review -> resolved
2017-01-04 20:54:34python-devsetnosy: + python-dev
messages: + msg284667
2017-01-04 20:32:09xdegayesetversions: + Python 3.6
2016-10-15 09:26:21xdegayesetfiles: + build-flags_5.patch

messages: + msg278712
2016-10-15 08:56:29xdegayesetmessages: + msg278708
versions: + Python 3.7, - Python 3.6
2016-07-29 11:21:36yan12125setnosy: + yan12125
2016-07-28 14:21:57xdegayesetmessages: + msg271552
2016-07-28 13:03:52martin.pantersetmessages: + msg271543
2016-07-28 11:37:30xdegayesetmessages: + msg271530
2016-07-28 11:01:26martin.pantersetmessages: + msg271518
2016-07-26 09:43:19xdegayesetfiles: + build-flags_4.patch

messages: + msg271353
2016-07-25 16:05:04xdegayesetmessages: + msg271292
2016-07-25 15:19:21vstinnersetmessages: + msg271282
2016-07-25 14:17:14xdegayesetmessages: + msg271275
2016-07-25 14:13:46xdegayesetfiles: + build-flags_3.patch
2016-07-25 09:31:27xdegayesetfiles: + build-flags_3.patch

messages: + msg271246
2016-07-25 00:44:50martin.pantersetmessages: + msg271193
2016-07-24 16:41:12xdegayesetfiles: + build-flags_2.patch

nosy: + doko, vstinner, martin.panter
messages: + msg271169

assignee: xdegaye
stage: patch review
2016-05-01 07:11:18xdegayesetnosy: + twouters
2016-04-26 16:04:41zach.warelinkissue26865 dependencies
2016-04-26 12:20:11xdegayecreate