Fix cross-compiling for android with the NDK #252
Conversation
| static_assert((NUM_HEXSTR_OPTS - 1) <= (QUOTE_HEXSTR_MASK >> QUOTE_HEXSTR_SHIFT), | ||
| _Static_assert((NUM_HEXSTR_OPTS - 1) <= (QUOTE_HEXSTR_MASK >> QUOTE_HEXSTR_SHIFT), |
There was a problem hiding this comment.
Because the build will fail without it?
The android headers only define static_assert when in c11 mode, thence:
./mpers.sh m32 "-std=gnu99 " $f || exit; \
done
In file included from mpers-m32/struct_blk_user_trace_setup.c:10:
./defs.h:899:32: error: expected ')'
static_assert((NUM_HEXSTR_OPTS - 1) <= (QUOTE_HEXSTR_MASK >> QUOTE_HEXST...
There was a problem hiding this comment.
We have static_assert.h (included by defs.h) to handle this on various platforms.
Please have a look why it didn't work out in your case.
| /* The following function might be unused, hence the inline qualifier. */ | ||
| static inline void | ||
| static inline void __attribute__((unused)) |
There was a problem hiding this comment.
I suppose this change is not needed, the compiler shouldn't issue any warnings for static inline functions.
On the contrary, the compiler will issue a warning if a function marked as unused is actually used.
There was a problem hiding this comment.
Your supposition is false:
echo 'static inline void foo(){}' | clang -Wall -Werror -c -xc -
<stdin>:1:20: error: unused function 'foo'
[-Werror,-Wunused-function]
static inline void foo(){}
^
1 error generated.
There was a problem hiding this comment.
Then this is a compiler bug, a lot of software relies on this static inline thing, please report it to compiler people instead.
There was a problem hiding this comment.
According to gcc documentation,
-Wunused-function
Warn whenever a static function is declared but not defined or a non-inline static function is unused. This warning is enabled by -Wall.
Sorry, I'm not going to workaround incorrect warnings reported by non-compliant compilers.
|
On Tue, May 09, 2023 at 03:33:36AM -0700, Dmitry V. Levin wrote:
@ldv-alt commented on this pull request.
> -static_assert((NUM_HEXSTR_OPTS - 1) <= (QUOTE_HEXSTR_MASK
8000
>> QUOTE_HEXSTR_SHIFT),
+_Static_assert((NUM_HEXSTR_OPTS - 1) <= (QUOTE_HEXSTR_MASK >> QUOTE_HEXSTR_SHIFT),
We have static_assert.h (included by defs.h) to handle this on various platforms.
Please have a look why it didn't work out in your case.
I've already told you in the previous message:
Because the android's `assert.h` only defines `static_assert` while in c11 mode, and you're asking it to compile in gnu99 mode with `-std=gnu99`. From `sysroot/usr/include/assert.h`:
```
#if !defined(__cplusplus) && __STDC_VERSION__ >= 201112L
# undef static_assert
# define static_assert _Static_assert
#endif
|
|
On Tue, May 09, 2023 at 03:32:11AM -0700, Dmitry V. Levin wrote:
> /* The following function might be unused, hence the inline qualifier. */
-static inline void
+static inline void __attribute__((unused))
Then this is a compiler bug, a lot of software relies on this static inline thing, please report it to compiler people instead.
It's not a compiler bug -- you asked for warnings about unused functions,
and you got them.
If anything, you should remove the `inline`, it's only clutter.
Nobody is forcing you to accept my patch; if you don't care, just ignore it. Please don't make up spurious reasons for refusing it, it's embarassing.
|
This sounds like a clang bug. |
| # First try $CC with 'aarch64' replaced with 'arm' and 'android' | ||
| # replaced with 'androideabi' | ||
| cc=${CC%/aarch64-*}/armv7a-${CC##*/aarch64-} | ||
| cc=${cc%-android*}-androideabi${cc##*-android} |
There was a problem hiding this comment.
Why exactly should it take priority over all the other options? Also, since the CC differs from the one that ./configure would detect, I guess it is provided explicitly, why not provide CC_FOR_M32 explicitly as well?
| # First try $CC with 'aarch64' replaced with 'arm' and 'android' | ||
| # replaced with 'androideabi' | ||
| cc=${CC%/aarch64-*}/armv7a-${CC##*/aarch64-} | ||
| cc=${cc%-android*}-androideabi${cc##*-android} |
There was a problem hiding this comment.
I see androideabi is not present among the currently checked ABI options, would simply adding it to triplet3 set help?
|
On Tue, May 09, 2023 at 05:34:08AM -0700, esyr wrote:
- AC_CHECK_PROGS(CC_FOR_M32, arm_compat_compilers)
- AS_IF([test -n "$CC_FOR_M32"],
+ [# So far, only aarch64 needs a separate compiler for its compat
+ # personality (which is AArch32, that is more or less ARMv7 EABI)
+ # First try $CC with 'aarch64' replaced with 'arm' and 'android'
+ # replaced with 'androideabi'
+ cc=${CC%/aarch64-*}/armv7a-${CC##*/aarch64-}
+ cc=${cc%-android*}-androideabi${cc##*-android}
I see `androideabi` is not present among the currently checked ABI options, would simply adding it to `triplet3` set help?
No it would not help.
|
|
On Tue, May 09, 2023 at 05:32:10AM -0700, esyr wrote:
@esyr commented on this pull request.
> - # Fedora: arm-linux-gnu-gcc
- # ALT: armh-alt-linux-gnueabi-gcc
- m4_foreach([triplet1], [arm, arm7, arm7hl, armh], dnl
- [m4_foreach([triplet2], [, $host_vendor-], dnl
- [m4_foreach([triplet3], [gnu, gnueabi, gnueabihf], dnl
- [m4_foreach([triplet_cc], [gcc, cc], dnl
- [m4_append([arm_compat_compilers], dnl
- triplet1[-]triplet2[linux-]triplet3[-]triplet_cc)])])])])
- AC_CHECK_PROGS(CC_FOR_M32, arm_compat_compilers)
- AS_IF([test -n "$CC_FOR_M32"],
+ [# So far, only aarch64 needs a separate compiler for its compat
+ # personality (which is AArch32, that is more or less ARMv7 EABI)
+ # First try $CC with 'aarch64' replaced with 'arm' and 'android'
+ # replaced with 'androideabi'
+ cc=${CC%/aarch64-*}/armv7a-${CC##*/aarch64-}
+ cc=${cc%-android*}-androideabi${cc##*-android}
Why exactly should it take priority over all the other options?
Because it's the only one that makes sense anyway. Picking up whatever
arm cross compiler comes first in PATH does not make any sense whatsoever.
Also, since the `CC` differs from the one that ./configure would detect, I guess it is provided explicitly, why not provide `CC_FOR_M32` explicitly as well?
Because that does not work, AC_CHECK_PROGS does not accept absolute paths.
|
|
On Tue, May 09, 2023 at 05:10:56AM -0700, turistu wrote:
On Tue, May 09, 2023 at 03:33:36AM -0700, Dmitry V. Levin wrote:
> @ldv-alt commented on this pull request.
>
> > -static_assert((NUM_HEXSTR_OPTS - 1) <= (QUOTE_HEXSTR_MASK >> QUOTE_HEXSTR_SHIFT),
> +_Static_assert((NUM_HEXSTR_OPTS - 1) <= (QUOTE_HEXSTR_MASK >> QUOTE_HEXSTR_SHIFT),
>
> We have static_assert.h (included by defs.h) to handle this on various platforms.
> Please have a look why it didn't work out in your case.
I've already told you in the previous message:
Because the android's `assert.h` only defines `static_assert` while in c11 mode, and you're asking it to compile in gnu99 mode with `-std=gnu99`.
If the check is in c11 mode but the compilation is in gnu99 mode, then
it's a bug that has to be fixed. No need to paper it over with a patch
that breaks build on all platforms that do not provide _Static_assert,
please.
|
It makes sense in the most common case of an isolated build environment, as there's normally only one cross-compiler installed for the specific ABI, the one is that is needed for the build. I see value of having a heuristic for setting the
I'm not exactly sure how this is related to the variables passed to |
|
On Tue, May 09, 2023 at 06:11:09AM -0700, Dmitry V. Levin wrote:
On Tue, May 09, 2023 at 05:10:56AM -0700, turistu wrote:
> On Tue, May 09, 2023 at 03:33:36AM -0700, Dmitry V. Levin wrote:
> > @ldv-alt commented on this pull request.
> >
> > > -static_assert((NUM_HEXSTR_OPTS - 1) <= (QUOTE_HEXSTR_MASK >> QUOTE_HEXSTR_SHIFT),
> > +_Static_assert((NUM_HEXSTR_OPTS - 1) <= (QUOTE_HEXSTR_MASK >> QUOTE_HEXSTR_SHIFT),
> >
> > We have static_assert.h (included by defs.h) to handle this on various platforms.
> > Please have a look why it didn't work out in your case.
>
> I've already told you in the previous message:
>
> Because the android's `assert.h` only defines `static_assert` while in c11 mode, and you're asking it to compile in gnu99 mode with `-std=gnu99`.
If the check is in c11 mode but the compilation is in gnu99 mode, then
it's a bug that has to be fixed. No need to paper it over with a patch
that breaks build on all platforms that do not provide _Static_assert,
please.
What platforms does that break on? _Static_assert is a gcc keyword supported
since eons.
Looks, this is getting old already.
I will continue to make pull requests for any bugs or fixes I think necessary,
(just to give them more visibility, and help people with the same problem)
but I'll stop answering to bogus remarks.
If you don't like it, just go on and block me.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #252 +/- ##
==========================================
+ Coverage 87.89% 90.16% +2.27%
==========================================
Files 286 290 +4
Lines 24895 25031 +136
Branches 3624 0 -3624
==========================================
+ Hits 21881 22569 +688
- Misses 2049 2462 +413
+ Partials 965 0 -965 ☔ View full report in Codecov by Sentry. |
15622ec to
e847aa4
Compare
2c28f3a to
09c9f2d
Compare
ba1226c to
efeb7b4
Compare
bed4534 to
503f099
Compare
a711a20 to
b2bbeae
Compare
76ef3a7 to
2c6846c
Compare
64ea23f to
8c1b94a
Compare
af547d7 to
0a8a582
Compare
9bab8fe to
9ba681b
Compare
f1739c2 to
1004a13
Compare
…in/aarch64-linux-android21-clang ./configure .. when determining the compiler for -m32 on aarch64, first look for one similarly named with s/aarch64/armv7a/ and s/android/&eabi/
…in/aarch64-linux-android21-clang ./configure .. header include hell
…in/aarch64-linux-android21-clang ./configure .. fix the errors triggered by -Wundef and -Wunused- with -Werror
keep only m32 when called with the -mx32 option, the ndk's clang cross compiler driver ends up running the system's gcc YUCK
breaks the get_cpu_size() kludge. "fix" it with another kludge [1] https://android-review.googlesource.com/c/platform/bionic/+/2496358/3/libc/include/sched.h
use null_ptr instead
The only thing not quite obvious is the patch to configure.ac:
before looking for whatever armv7a compiler the user may have in her path (as the current "heuristics" does), look for a analogously named compiler in the same path as the aarch64 one.