Remove LFS64 calls and set _FILE_OFFSET_BITS=64

[vimproved]

Musl 1.2.4 made the LFS64 interfaces only available when _LARGEFILE64_SOURCE is defined, and they will be removed altogether in Musl 1.2.5. This commit replaces the LFS64 calls with their non-LFS64 versions and defines _FILE_OFFSET_BITS=64, which makes all interfaces 64-bit.

Bug: https://bugs.gentoo.org/905999

[thesamesam]

This looks right - glibc needs _F_O_B=64 to make these functions LFS aware and other libcs, like musl, should get it right out of the box. It's harmless to set _F_O_B on !glibc.

This fixes #939 too.

[inkychris]

The now hardcoded definition of _FILE_OFFSET_BITS=64 is causing issues when compiling using the Android NDK targetting API v21 for armeabi-v7a and x86. In this particular case the PUBLIC nature of it is poluting cmake targets that link to tinyxml2. It might be enough to simply set this to PRIVATE, but I don't think this should be handled by tinyxml's build configuration at all.

This appears to keep causing issues for various environments, and each fix for one seems to break another. I think a better approach would be to allow the values to be configured externally so that tinyxml2 doesn't need to be modified to get it building in various environments. Alternatively, the build configurations should check what's actually available and set the values accordingly.

[thesamesam]

but I don't think this should be handled by tinyxml's build configuration at all.

The software expects and requires functions which are LFS-capable.

Anyway, you may need to adjust the #elif defined(__ANDROID__) case -- it's hard to say without seeing what the exact failure is. Please file a bug with the logs?

If Android really is special (I recall reading some weirdness about this before, but not specifics), then carving out an exception for it should be fine.

[inkychris]

The value of _FILE_OFFSET_BITS doesn't actually impact what's used in the tinyxml source code. Tinyxml builds fine without the LFS-capable functions. This is evident with 32-bit android v21, even with _FILE_OFFSET_BITS=64 set, because fseek and ftell still end up being used.

My problem specifically is that the target I'm linking tinyxml to (cmake) is now inheriting the public compile definition for _FILE_OFFSET_BITS which impacts some standard library imports and for 32-bit android v21, subsequently fails to link to fseeko. I've gotten around this issue by adding -U_FILE_OFFSET_BITS to CMAKE_CXX_FLAGS in my preset for my android build.

I think this is a good example of why settings like this shouldn't be specified at the target level in cmake, but instead set at build/configure time, e.g. in a toolchain or preset file. I think the simplest solution which would be consistent between CMake and Make at least, would be a feature flag to allow at build/configuration time to select which functions to use if the ones tinyxml defaults to aren't available or are incorrect. For cmake at least, we could then also use something like check_symbol_exists for the various functions and select them accordingly, which I'd expect would provide the most "correct" behaviour for all environments.

[thesamesam]

The value of _FILE_OFFSET_BITS doesn't actually impact what's used in the tinyxml source code. Tinyxml builds fine without the LFS-capable functions. This is evident with 32-bit android v21, even with _FILE_OFFSET_BITS=64 set, because fseek and ftell still end up being used.

Sure, but then the project ends up getting bug reports because it can't handle larger files and so on.

I think this is a good example of why settings like this shouldn't be specified at the target level in cmake, but instead set at build/configure time, e.g. in a toolchain or preset file

Asking the user to specify this is more challenging because it affects ABI in some libraries and not in others.

[inkychris]

Having done some more digging, this seems to me like something that CMake itself should be handling at the compiler level if the general opinion is that it should always be set. That aside, LLVM made a special case in their CMake configuration for 32-bit android which is the issue I'm seeing. They've simply guarded it with a feature flag and disabled it for android <v24. I assume if this wasn't likely just an Android issue, they'd have encountered it by now.

I'm happy to submit a PR following their approach if it's a palatable solution.

[thesamesam]

Sounds good to me at least.