Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check 'support_64bit_file_offsets' cross-file property #2996

Closed

Conversation

rib
Copy link
Contributor

@rib rib commented Jan 30, 2018

Since we're currently targeting some 32bit Android devices with API level 23 we can't rely on support for -D_FILE_OFFSET_BITS=64

(E.g. ref: android/ndk#442)

This patch adds a check for a 'support_64bit_file_offsets' cross-file property so we have a way to avoid meson passing -D_FILE_OFFSET_BITS=64 while compiling.

I can aim to look at writing tests if this seems reasonable.

Some (even relatively recent) versions of Android don't support 64bit
file offsets via -D_FILE_OFFSET_BITS=64, so it's now possible to avoid
the automatic definition by setting:

 support_64bit_file_offsets = False

in the [properties] section of a cross-file
@jpakkane
Copy link
Member

Ping @nirbheek, you probably know the most about this area.

@nirbheek
Copy link
Member

I think the proper way to do this is to detect that the platform in question is Android 32-bit, and then run a compiler check for it.

@Rondom
Copy link

Rondom commented Feb 11, 2018

MingW-W64 is also a candidate that would profit from a compile-time-check. It also supports _FILE_OFFSET_BITS, so the statement in the comments, that Windows does not support it is not 100% true.

@rib
Copy link
Contributor Author

rib commented Feb 12, 2018

Currently my standard way of detecting that I'm building for Android is to use a check like:

if compiler.get_define('__ANDROID__') != ''
endif

It feels conceptually like a chicken-egg problem to invoke the compiler to figure out arguments that we should pass whenever we invoke the compiler. At least when I was looking at this it wasn't clear to me how I should do something equivalent at this point within the compiler backend? For an option like this that affects the ABI it seems you'd want to be very careful about allowing invocations of the compiler with/without _FILE_OFFSET_BITS defined, especially if we inadvertently cached defines for the wrong ABI.

I think the situation might also be a little more messy than being able to detect 32bit arm and black listing that. This page describes some of the awkwardness: https://android.googlesource.com/platform/bionic/+/master/docs/32-bit-abi.md Also see here for some discussion about different behaviour with the NDK's recently unified headers: android/ndk#442

Notably NDK toolchains prior to r15 did silently let you compile for 32bit ARM with _FILE_OFFSET_BITS=64 by just ignoring the requirement for 64bit file offsets. A compile time check would be tricky for these versions of the NDK.

Further the NDK does semi-support _FILE_OFFSET_BITS=64 on 32bit arm depending on the API level being targeted, up until level 24 where afiu it does fully support _FILE_OFFSET_BITS=64 even on 32bit ARM. E.g. it has always support lseek64 and supports mmap64 from level 21 - except notably if you used Clang then they would inline an mmap64 shim on earlier versions.

For reference you can search for the 64; suffix within https://android.googlesource.com/platform/bionic/+/master/libc/libc.map.txt and see when various 64bit file IO apis were added.

Overall I think it could be nice if Meson had some better built-in understanding of the Android platform where it might encode some of this knowledge internally, but also that sounds like a fairly big piece of work which I'm not sure I could take on currently.

I think it might be too simplistic to just say we can just introspect the toolchain and black list 32bit ARM and without a more thorough understanding of the details I think a cross-file override something like this could still be a reasonable practical solution.

@nirbheek
Copy link
Member

Thanks for the details about Android, and sorry to hear that you can't help with it. You seem like the ideal person!

As for adding an option, projects can already undo that by passing -U_FILE_OFFSET_BITS in the cross file, so I am not sure what adding a custom option would gain besides adding API that we would have to support forever.

For completeness, can you confirm that -U_FILE_OFFSET_BITS also works?

@rib
Copy link
Contributor Author

rib commented Feb 12, 2018

A good suggestion, which I hadn't thought of trying before but unfortunately that work around doesn't Just Work™ currently:

[4/268] Compiling C++ object 'subprojects/boost_1_65_1/libs/thread/boost_thread@sha/src_pthread_thread.cpp.o'.
FAILED: subprojects/boost_1_65_1/libs/thread/boost_thread@sha/src_pthread_thread.cpp.o
arm-linux-androideabi-clang++ -mfpu=neon -fno-omit-frame-pointer -funwind-tables -U_FILE_OFFSET_BITS -Isubprojects/boost_1_65_1/libs/thread/boost_thread@sha -Isubprojects/boost_1_65_1/libs/thread -I../subproject
s/boost_1_65_1/libs/thread -Isubprojects/boost_1_65_1 -I../subprojects/boost_1_65_1 -Xclang -fcolor-diagnostics -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -std=c++14 -O0 -g -fPIC -pthrea
d -MD -MQ 'subprojects/boost_1_65_1/libs/thread/boost_thread@sha/src_pthread_thread.cpp.o' -MF 'subprojects/boost_1_65_1/libs/thread/boost_thread@sha/src_pthread_thread.cpp.o.d' -o 'subprojects/boost_1_65_1/libs
/thread/boost_thread@sha/src_pthread_thread.cpp.o' -c ../subprojects/boost_1_65_1/libs/thread/src/pthread/thread.cpp
In file included from ../subprojects/boost_1_65_1/libs/thread/src/pthread/thread.cpp:11:
In file included from ../subprojects/boost_1_65_1/boost/thread/thread_only.hpp:17:
In file included from ../subprojects/boost_1_65_1/boost/thread/pthread/thread_data.hpp:10:
In file included from ../subprojects/boost_1_65_1/boost/thread/exceptions.hpp:20:
In file included from /home/bob/local/android-arm-toolchain-23-r15c/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/string:440:
/home/bob/local/android-arm-toolchain-23-r15c/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/cstdio:137:9: error: no member named 'fgetpos' in the global namespace
using ::fgetpos;
      ~~^
/home/bob/local/android-arm-toolchain-23-r15c/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/cstdio:139:9: error: no member named 'fsetpos' in the global namespace
using ::fsetpos;
      ~~^
In file included from ../subprojects/boost_1_65_1/libs/thread/src/pthread/thread.cpp:34:
/home/bob/local/android-arm-toolchain-23-r15c/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/fstream:822:9: error: use of undeclared identifier 'fseeko'
    if (fseeko(__file_, __width > 0 ? __width * __off : 0, __whence))

I guess the order could somehow be changed to allow this to work, but it would be good if such a change also came with a unit test to guarantee that the order can't inadvertently be reverted in the future. Given how the order of arguments can be significant in different ways we could probably imagine other situations where you really do want the cross-file arguments to precede everything else - like they do currently.

I might be able to help contribute to adding an Android module/backend at some point, but at least in the short-term I have a milestone for our Glimpse mocap system that I'm working hard to meet so won't have much time to spare for a while.

Although I agree to some extent about the hassle of maintaining features that may be superseded later with smarter platform support; apart from plumbing through the environment to more parts of the backend this feature is fairly trivial. My current feeling is that it will always be worthwhile having a last-ditch override mechanism considering the difficulty of reliably/portability of introspecting whether this feature is fully supported. For Android I think you'd have to explicitly check sizeof(off_t) == 64 and then making assumptions about the 32bit ABI for 64bit file ops I suppose you could forward declare all the 64bit symbols like fstat64 and mmap64 and explicitly check you can link with those without letting the compiler potentially NOP the implementation like early versions of the NDK did. That wouldn't be a portable check though.

The alternative of requiring Meson to have platform specific support for any kind of cross-compiler that needs an override for this also sounds like it involves notable effort to maintain (perhaps more). I still agree that e.g. for Android it would be useful if Meson had more knowledge of the Platform, but there are lots of miscellaneous linux-ish platforms that cross compilers are created for and I'm not sure how reasonable it is to require Meson have built-in knowledge of them. E.g. It seems plausible people could have configurations of Yocto without LFS enabled in their libc + create a standalone toolchain for development. If you're unlucky enough to hit the limitation you might be stuck not being able to use Meson without patching it like I was.

@nirbheek
Copy link
Member

A good suggestion, which I hadn't thought of trying before but unfortunately that work around doesn't Just Work™ currently:

Hum, we should fix that and add a test for it.

I am not against adding a separate option for this, but I feel like people are going to miss it anyway unless they read the documentation, so if we document this as the recommended way to disable implicit 64-bit off_t on 32-bit, people will use it.

We should also add a sample android32 cross-info file to git that people can either use as-is or edit as they need.

@jpakkane
Copy link
Member

According to #3049 this flag causes build corruption on Raspi + some other platforms. If this is the case maybe we should remove it altogether or at least not have it on by default.

@nirbheek
Copy link
Member

nirbheek commented Feb 21, 2018

We should probably start using a whitelist of distros/libcs, and run a compiler check on the rest. By not having it on by default, we get broken binaries on 32-bit x86.

@nirbheek
Copy link
Member

This change is not sufficient because this is an issue while compiling natively too. The feature provided by this PR can also be availed by passing -U_FILE_OFFSET_BITS in c_args in the cross file since #3411 (0.46 onwards).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants