Do not use LFS64 functions on linux/musl #2589
Conversation
On musl, off_t is 64bit always ( even on 32bit platforms ), therefore using LFS64 funcitons is not needed on such platforms. Moreover, musl has stopped providing aliases for these functions [1] which means it wont compile on newer musl systems. Therefore only use it on 32bit glibc/linux platforms and exclude musl like cygwin or OSX [1] https://git.musl-libc.org/cgit/musl/commit/?id=246f1c811448f37a44b41cd8df8d0ef9736d95f4 Signed-off-by: Khem Raj <raj.khem@gmail.com>
| // 64 bits(but not in osx or cygwin, where fstat64 is deprecated) | ||
| # if (defined(__linux__) || defined(__sun) || defined(_AIX)) && (defined(__LP64__) || defined(_LP64)) | ||
| // 64 bits(but not in osx, linux/musl or cygwin, where fstat64 is deprecated) | ||
| # if ((defined(__linux__) && defined(__GLIBC__)) || defined(__sun) || defined(_AIX)) && (defined(__LP64__) || defined(_LP64)) |
There was a problem hiding this comment.
Better check for some kind of musl specifc macro to detect it. Who knows what it may break checking GLIBC for this.
There was a problem hiding this comment.
Glibc defined the macro long time ago and it won’t go away ever however musl does not define any libc identifying macros intentionally
There was a problem hiding this comment.
This would affect any other libc vendors that dont define GLIBC .
There was a problem hiding this comment.
its already checking for linux, which limits the C library choices. Glibc and uclibc already define GLIBC, third option is musl and another option is bionic/android which would be similar to musl, if needed it can be controlled via android define. Do you have any other combination that spdlog supports on linux ?
There was a problem hiding this comment.
There is no specific combination, but this is bit dangerous. Who knows what libc impl this would break. According to this, there are at least 11. Makes much more sense to check for musl if this is a musl fix
There was a problem hiding this comment.
Let me put it differently. What this code does currently is glibc specific, LFS64 functions are not posix defined. It’s not Linux specific. So perhaps making the check to also look for glibc is the right thing here
There was a problem hiding this comment.
That’s probably right. I am even thinking to remove the use of fstat64() entirely, since it seems that in 64 bit platforms the regular fstat() already returns the 64 version of off_t (including android bionic). Could you please check this assumption @kraj ?
There was a problem hiding this comment.
yes. On 64bit linux and AFAICT BSDs and OSX off_t is 64bit
|
Merged. Thanks @kraj |
On musl, off_t is 64bit always ( even on 32bit platforms ), therefore using LFS64 funcitons is not needed on such platforms. Moreover, musl has stopped providing aliases for these functions [1] which means it wont compile on newer musl systems. Therefore only use it on 32bit glibc/linux platforms and exclude musl like cygwin or OSX [1] https://git.musl-libc.org/cgit/musl/commit/?id=246f1c811448f37a44b41cd8df8d0ef9736d95f4 Signed-off-by: Khem Raj <raj.khem@gmail.com>
On musl, off_t is 64bit always ( even on 32bit platforms ), therefore using LFS64 funcitons is not needed on such platforms. Moreover, musl has stopped providing aliases for these functions [1] which means it wont compile on newer musl systems. Therefore only use it on 32bit glibc/linux platforms and exclude musl like cygwin or OSX
[1] https://git.musl-libc.org/cgit/musl/commit/?id=246f1c811448f37a44b41cd8df8d0ef9736d95f4
Signed-off-by: Khem Raj raj.khem@gmail.com