can't #include <cstdio> with __FILE_OFFSET_BITS=64

[DanAlbert]

Refiling @koying's follow up on #442:

I'm afraid this fix produced an unwanted side-effect in 15c if you #include <cstdio> with __USE_FILE_OFFSET64

/opt/android-dev/android-toolchain-arm-21-r15c/include/c++/4.9.x/cstdio:107:11: error: '::fgetpos' has not been declared
   using ::fgetpos;
           ^
/opt/android-dev/android-toolchain-arm-21-r15c/include/c++/4.9.x/cstdio:117:11: error: '::fsetpos' has not been declared
   using ::fsetpos;
           ^

[DanAlbert]

This is similar to a problem that libandroid_support solves for libc++. <cmath> (just as one example) tries to do this for a bunch of functions that aren't really available. libandroid_support patches those holes by adding declarations for anything that needs to be pulled in by using. There are no definitions, but it lets the header be included.

We can add all the stuff that would be hidden in the _FILE_OFFSET_BITS=64 case to libandroid_support for libc++, but for gnustl I'm going to say just turn off _FILE_OFFSET_BITS=64, since gnustl doesn't use libandroid_support and will be removed from the NDK soon anyway.

[koying]

Actually, worse, as fgetpos and friends are not defined at all, 32 or 64 bits, if API < N

[DanAlbert]

Ugh. These hacks that should have been in libandroid_support were directly in the deprecated headers.

[DanAlbert]

Oh, no, that isn't correct. fgetpos was available since at least Gingerbread.

[koying]

Yeah, I know ;)
The 32bits version, at least. Down't know about the 64bits one...

[koying]

Actually, pretty sure the 64bits ones were also, as we used some hacks before unified headers to do LARGE_OFFSET

[DanAlbert]

Then I don't understand what you meant by

Actually, worse, as fgetpos and friends are not defined at all, 32 or 64 bits, if API < N

[enh]

fgetpos64 wasn't available until N. _FILE_OFFSET_BITS=64 used to be a no-op until r15, so you're probably just confused.

[koying]

@DanAlbert Well, with current headers, nothing is defined.
Maybe I misread, though

#if defined(__USE_FILE_OFFSET64)

#if __ANDROID_API__ >= 24
int fgetpos(FILE*, fpos_t*) __RENAME(fgetpos64) __INTRODUCED_IN(24);
int fsetpos(FILE*, const fpos_t*) __RENAME(fsetpos64) __INTRODUCED_IN(24);
int fseeko(FILE*, off_t, int) __RENAME(fseeko64) __INTRODUCED_IN(24);
off_t ftello(FILE*) __RENAME(ftello64) __INTRODUCED_IN(24);
#endif /* __ANDROID_API__ >= 24 */

#  if defined(__USE_BSD)

#if __ANDROID_API__ >= 24
FILE* funopen(const void*,
              int (*)(void*, char*, int),
              int (*)(void*, const char*, int),
              fpos_t (*)(void*, fpos_t, int),
              int (*)(void*)) __RENAME(funopen64) __INTRODUCED_IN(24);
#endif /* __ANDROID_API__ >= 24 */

#  endif
#else

[koying]

@enh Might be that the code wasn't hit, but at least it compiled (we are a multi-platform app)

[DanAlbert]

Scroll down one more line.

[koying]

Sure, but this is in the context of __USE_FILE_OFFSET64
Or do I really misread the #if 's ?

[enh]

i think you don't understand that until r15 you were using headers that just ignored _FILE_OFFSET_BITS. full story: https://android.googlesource.com/platform/bionic/+/master/#32_bit-ABI-bugs-is-32_bit-1

[koying]

@enh We know, trust me ;)
https://github.com/xbmc/xbmc/blob/master/tools/depends/target/samba-gplv3/samba_off64_t.patch

[DanAlbert]

Or do I really misread the #if 's ?

Certainly one of us has :) A trivial test case that just calls fgetpos works fine when targeting ICS.

[koying]

With __USE_FILE_OFFSET64 ?
I'll have to drink a beer or two and re-read, then ;)

[DanAlbert]

No, without __USE_FILE_OFFSET64, but that's precisely what I would expect. That brings us back to my original plan:

We can add all the stuff that would be hidden in the _FILE_OFFSET_BITS=64 case to libandroid_support for libc++, but for gnustl I'm going to say just turn off _FILE_OFFSET_BITS=64, since gnustl doesn't use libandroid_support and will be removed from the NDK soon anyway.

I still encourage the beer either way :)

[koying]

So the plan is to revert to pre-r15 situation re LARGE_OFFSET, ie no support. Right?
I'm not aware of libandroid_support, but I assume it's some kind of extension library?

[DanAlbert]

If you don't need 64-bit file offsets in your app, turn off _FILE_OFFSET_BITS=64. If you do, keep it on. Depending on your minSdkVersion, you might have to work around things that weren't available in that API level for the things you use.

That last bit (including <cstdio> and friends breaks your build, even if you're not using the functions that are the problem) is the part that I think we should fix here, but only for libc++. Since _FILE_OFFSET_BITS=64 is new to the NDK, this isn't a regression for either STL. You can get the old behavior back by just not using that define.

We'll make it work well for libc++, but gnustl hasn't been getting updates/fixes for a while now and is going to be removed in a release or two anyway.

I'm not aware of libandroid_support, but I assume it's some kind of extension library?

Yeah, it's a library that backports the things that libc++ needs to function. We've never tested it with any of the other STLs, and don't plan to (since the others are being removed soon).

[enh]

If you don't need 64-bit file offsets in your app, turn off _FILE_OFFSET_BITS=64

strictly: "if you don't need off_t to be 64-bit in your app, turn off _FILE_OFFSET_BITS=64"

the link he showed above was using off64_t and blah64(), corresponding to _LARGEFILE_SOURCE, which you don't need on Android: those functions are always available (if they existed in your target API level).

[DanAlbert]

Fixed in r16.

[dmxraj]

@DanAlbert
Does r16 Beta 1 has this fix?

Link: https://dl.google.com/android/repository/android-ndk-r16-beta1-linux-x86_64.zip

Thanks

[feliwir]

@DanAlbert try using CMake 3.10 with Android NDK r16, libc++_static and API version 14. As soon as i define __ANDROID_API__=14, the entire build is breaking, because of missing fseeko (and friends) in the universal headers. This got fixed by the macro i pasted above.

And no, i can't use the toolchain file provided with the NDK for good reasons.

[jaimecbernardo]

This issue seems to be back on NDK r17.
I'm testing with this test.cpp file:

#define _FILE_OFFSET_BITS 64
#include <cstdio>

I'm then building six standalone toolchains (r15c,r16b,r17b)x(android-22,android-24):

~/android-ndk-r15c/build/tools/make-standalone-toolchain.sh --toolchain=arm-linux-androidabi-4.9 --arch=arm --stl=libc++ --platform=android-22 --install-dir=./ndk-r15-API22
~/android-ndk-r15c/build/tools/make-standalone-toolchain.sh --toolchain=arm-linux-androidabi-4.9 --arch=arm --stl=libc++ --platform=android-24 --install-dir=./ndk-r15-API24
~/android-ndk-r16b/build/tools/make-standalone-toolchain.sh --toolchain=arm-linux-androidabi-4.9 --arch=arm --stl=libc++ --platform=android-22 --install-dir=./ndk-r16-API22
~/android-ndk-r16b/build/tools/make-standalone-toolchain.sh --toolchain=arm-linux-androidabi-4.9 --arch=arm --stl=libc++ --platform=android-24 --install-dir=./ndk-r16-API24
~/android-ndk-r17/build/tools/make-standalone-toolchain.sh --toolchain=arm-linux-androidabi-4.9 --arch=arm --stl=libc++ --platform=android-22 --install-dir=./ndk-r17-API22
~/android-ndk-r17/build/tools/make-standalone-toolchain.sh --toolchain=arm-linux-androidabi-4.9 --arch=arm --stl=libc++ --platform=android-24 --install-dir=./ndk-r17-API24

These toolchains are able to build test.cpp:

./ndk-r15-API24/bin/clang -c test.cpp
./ndk-r16-API22/bin/clang -c test.cpp
./ndk-r16-API24/bin/clang -c test.cpp
./ndk-r17-API24/bin/clang -c test.cpp

But these toolchains are unable to build test.cpp:

$ ./ndk-r15-API22/bin/clang -c test.cpp
In file included from test.cpp:2:
/Users/myuser/android-single-c/./ndk-r15-API22/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;
      ~~^
/Users/myuser/android-single-c/./ndk-r15-API22/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;
      ~~^
2 errors generated.

$ ./ndk-r17-API22/bin/clang -c test.cpp
In file included from test.cpp:2:
/Users/myuser/android-single-c/./ndk-r17-API22/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/cstdio:135:9: error: 
      no member named 'fgetpos' in the global namespace; did you mean 'fgets'?
using ::fgetpos;
      ~~^
./ndk-r17-API22/bin/../sysroot/usr/include/stdio.h:112:7: note: 'fgets' declared
      here
char* fgets(char* __buf, int __size, FILE* __fp);
      ^
In file included from test.cpp:2:
/Users/myuser/android-single-c/./ndk-r17-API22/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/cstdio:137:9: error: 
      no member named 'fsetpos' in the global namespace
using ::fsetpos;
      ~~^
2 errors generated.

While this issue was fixed for r16, it seems to have come back in r17.
I think this issue should be re-opened, or should a new one be created?

[enh]

API 24 works, because you're asking for a 64-bit fgetpos/fsetpos, and they're available in API 24:

#if defined(__USE_FILE_OFFSET64)
int fgetpos(FILE* __fp, fpos_t* __pos) __RENAME(fgetpos64) __INTRODUCED_IN(24);
int fsetpos(FILE* __fp, const fpos_t* __pos) __RENAME(fsetpos64) __INTRODUCED_IN(24);

API 22 doesn't work because they're not.

libandroid_support has a hack in its stdio.h, specifically to allow <cstdio> to be included:

#if defined(__USE_FILE_OFFSET64) && __ANDROID_API__ < __ANDROID_API_N__
// Not really available, but we need a decl to allow `#include <cstdio>`.
int fgetpos(FILE*, const fpos_t*) __RENAME(fgetpos64);
int fsetpos(FILE*, const fpos_t*) __RENAME(fsetpos64);
#endif

what changed in r17 is that libandroid_support is no longer used for API >= 21.

(even before then, i'm not sure why there was a special case for these, but not for the other late additions. huh, looking at <cstdio> it doesn't seem to mention them at all. fseeko, for example, is completely missing. at least i can feel vindicated that i never use the <c headers.)

i guess we'll have to move this hack into bionic then. preferably with a slightly improved comment explicitly saying "this doesn't matter because you'll still get a link-time error if you mistakenly try to actually use these".

[DanAlbert]

This is probably deserving of an r17b :(

No timeline on that yet. We generally want to give the release a week or two of soak time (not counting last week since I/O means lots of Android developers were ooo) to make sure we catch as many regressions as possible in the hotfix rather than having to release a dozen hotfixes.

[marsbible]

waiting for r17b...

[epicstar]

I'm getting this exact problem too with boost :(

I'll patiently wait for r17b.

what changed in r17 is that libandroid_support is no longer used for API >= 21.

(even before then, i'm not sure why there was a special case for these, but not for the other late additions. huh, looking at it doesn't seem to mention them at all. fseeko, for example, is completely missing. at least i can feel vindicated that i never use the <c headers.)

i guess we'll have to move this hack into bionic then. preferably with a slightly improved comment explicitly saying "this doesn't matter because you'll still get a link-time error if you mistakenly try to actually use these".

Based on these comments above, wouldn't it make more sense to keep using libandroid_support above API 21? I can see similar problems to this if the team keeps adding standard POSIX C and C++ methods to the newer Android APIs that will be missing below API 27 and more, and the point of libandroid_support was to implement missing standard methods not present in the lower APIs.

[DanAlbert]

the point of libandroid_support was to implement missing standard methods not present in the lower APIs.

The point of libandroid_support is to backport just enough for libc++ to work. We'd once discussed backporting everything we could, but when we asked if that was even a thing people wanted the answer was "not if it increases APK size", which is unavoidable: #456.

[amartinz]

what about a support library for libc++, like it is right now and a opt-in support library for backports?

this does not increase code size for people who care about it but is really helpful for people who want to utilize features across the platforms without caring about code size.

[enh]

what about a support library for libc++, like it is right now and a opt-in support library for backports?

i think you're misunderstanding what's going on here: no implementations have been taken away. you never had 64-bit off_t functions: you were just in a world where your request for them was being ignored and you were getting 32-bit functions. if that's what you actually want, stop requesting the 64-bit off_t. folks who genuinely want 64-bit functions now get them where available.

the bug here is that we took away a declaration for a function you don't have, but whose declaration is required by a libc++ header. that's just a bug, and we'll fix that.

@DanAlbert --- am i supposed to be making that change or are you?

[DanAlbert]

no implementations have been taken away. you never had 64-bit off_t functions: you were just in a world where your request for them was being ignored

To clarify: _FILE_OFFSET_BITS=64 was a no-op because the C library didn't have support for that in the past. We added that and now the feature flag that was previously silently ignored is now in effect.

@DanAlbert --- am i supposed to be making that change or are you?

I already have it more or less ready.

[DanAlbert]

For the time being I've modified libc++ rather than bionic: https://android-review.googlesource.com/c/platform/external/libcxx/+/691583

We need to decide if we want to try ton get this change upstream (given that it's forcing libc++ to workaround a bionic bug, I sort of doubt it), push most of the libandroid_support crap into bionic, or something else.

Also added an additional test case since we were missing the configuration of LP32 21+.

[amartinz]

i think you're misunderstanding what's going on here

Sorry, i did not mean the 64-bit off_t, i was just speaking in generally about the whole idea of an android backport library.

Wrong place and wrong time to discuss this though :)

[smilesworld116]

Any updates on r17b?