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

Unified headers: strerror_r not available #324

Closed
hrydgard opened this issue Mar 13, 2017 · 9 comments
Closed

Unified headers: strerror_r not available #324

hrydgard opened this issue Mar 13, 2017 · 9 comments
Assignees
Milestone

Comments

@hrydgard
Copy link

hrydgard commented Mar 13, 2017

Description

Even though I include <string.h>, with unified headers, strerror_r is not available.

FAILED: C:\Android\sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\clang++.exe  --target=aarch64-none-linux-android --gcc-toolchain=C:/Android/sdk/ndk-bundle/toolchains/aarch64-linux-android-4.9/prebuilt/windows-x86_64 --sysroot=C:/Android/sdk/ndk-bundle/sysroot  -DARM64 -DMOBILE_DEVICE -DPPSSPP -DSHARED_ZLIB -DUSING_GLES2 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE64_SOURCE=1 -D_XOPEN_SOURCE=700 -D_XOPEN_SOURCE_EXTENDED -D__BSD_VISIBLE=1 -D__STDC_CONSTANT_MACROS -I../../../../../ext/native -I../../../../../ -I../../../../../Common -I../../../../../ext/native/ext/rg_etc1 -I../../../../../ext/cityhash -I../../../../../ext/native/ext/libzip -I../../../../../ext/native/ext -I../../../../../ext/libkirk -I../../../../../ext/sfmt19937 -I../../../../../ext/xbrz -I../../../../../ext/xxhash -I../../../../../ext/snappy/. -isystem C:/Android/sdk/ndk-bundle/sources/cxx-stl/gnu-libstdc++/4.9/include -isystem C:/Android/sdk/ndk-bundle/sources/cxx-stl/gnu-libstdc++/4.9/libs/arm64-v8a/include -isystem C:/Android/sdk/ndk-bundle/sources/cxx-stl/gnu-libstdc++/4.9/include/backward -isystem C:/Android/sdk/ndk-bundle/sysroot/usr/include/aarch64-linux-android -D__ANDROID_API__=21 -g -DANDROID -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -Wa,--noexecstack -Wformat -Werror=format-security  -isystem C:/Android/sdk/ndk-bundle/sysroot/usr/include/aarch64-linux-android -D__ANDROID_API__=21 -g -DANDROID -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -Wa,--noexecstack -Wformat -Werror=format-security   -std=c++11 -O0 -fno-limit-debug-info -O0 -fno-limit-debug-info  -fPIC   -Wno-multichar -fno-strict-aliasing -include C:/dev/ppsspp/ppsspp_config.h -MD -MT CMakeFiles/Common.dir/Common/Misc.cpp.o -MF CMakeFiles\Common.dir\Common\Misc.cpp.o.d -o CMakeFiles/Common.dir/Common/Misc.cpp.o -c C:\dev\ppsspp\Common\Misc.cpp

C:\dev\ppsspp\Common\Misc.cpp:59:6: error: use of undeclared identifier 'strerror_r'
    if (strerror_r(errCode, err_str, buff_size) == 0) {
        ^

Environment Details

  • NDK Version: 14.0.3770861
  • Build system: cmake with gradle in Android Studio, cmake -DANDROID_UNIFIED_HEADERS=ON ....
  • Host OS: Windows
  • Compiler: clang
  • ABI: *
  • STL: gnustl_static
@DanAlbert DanAlbert added this to the r14b milestone Mar 13, 2017
@DanAlbert DanAlbert self-assigned this Mar 13, 2017
@DanAlbert
Copy link
Member

@hrydgard
Copy link
Author

Thanks!

However, do I understand correctly that the function is only actually there in Android 23+? There's been a call to it in my code for some time in an app built to be compatible with Android 9+, but perhaps it never actually got called - or it would have crashed?

@DanAlbert
Copy link
Member

The GNU version of it, yes. There are actually two strerror_r APIs with different signatures, a GNU one and a POSIX one. We've always had the POSIX one, but the GNU one was added in android-23, yes.

You'll get the POSIX one if you are targeting pre-23 or if you don't have _USE_GNU defined (note that this is actually defined by default in C++ mode). You'll get the GNU one if you target 23 or above and have _USE_GNU set.

This restores the old, pre-unified headers behavior, so your app should keep building as it always has once r14b ships.

@hrydgard
Copy link
Author

Oh, okay. Thanks for the detailed explanation!

@jmgao
Copy link
Contributor

jmgao commented Mar 13, 2017

(Note that if you're only targeting android K or later, you probably don't actually need to use strerror_r. We use a thread local buffer for strerror starting in K)

@hrydgard
Copy link
Author

Well, I still keep compatibility with Gingerbread, mostly just because, but also since my app is an excellent fit for the Xperia Play.

But, @jmgao, to me that feels like a move in the wrong direction. Ugly global-state APIs like strerror or strtok should simply be replaced with properly designed threadsafe APIs where the user manages the buffer, IMHO. Inserting thread local buffers behind the users back will only serve to perpetuate these terrible APIs. Though maybe there are pragmatic reasons I'm not thinking of. Anyway, rant over :)

@DanAlbert
Copy link
Member

Change is merged.

@enh
Copy link
Contributor

enh commented Mar 14, 2017

But, @jmgao, to me that feels like a move in the wrong direction.

in general, one advantage TLS has over the _r alternative is that you don't need to know how big a buffer you'll need. if you don't need a copy, no copy is made. if you do, you can size the buffer correctly before copying rather than potentially have to try multiple times or over-allocate.

this is particularly problematic with readdir_r which basically should never be used. (see http://elliotth.blogspot.com/2012/10/how-not-to-use-readdirr3.html for much more on this, including examples of how even the man page screwed up trying to use it.)

@hrydgard
Copy link
Author

hrydgard commented Mar 14, 2017

Well, readdir_r is particularly poorly designed, and readdir has the advantage that there's a DIR* object to stash the pointer to an allocated string in. strerror doesn't have anything like the DIR* and needs a global thread-local pointer, further bloating the TLS libc state.

But I'll concede that if your metric is bugs in user code overall, which is pretty sensible, "non-r" variants have an advantage.

hubot pushed a commit to aosp-mirror/platform_bionic that referenced this issue Jul 10, 2017
We had several bugs filed saying "if I set _FILE_OFFSET_BITS=64 when
targeting an API < L, various functions are missing". Instead of
saying "yes, they are", we quietly just modified the header files to
expose the non-64-bit variants. This makes no sense. We can't just say
"oh, yeah, we don't have a version of this function that agrees with
your calling code about how large off_t is, but here's a version that
doesn't: I'm sure it'll be fine".

_FILE_OFFSET_BITS=64 on Android LP32 has always been a game of chance,
but that game should be "are all the functions my code needs available
at compile time?", not "will my code actually work at run time?".

Bug: android/ndk#449
Bug: android/ndk#442
Bug: android/ndk#333
Bug: android/ndk#332
Bug: android/ndk#324
Test: builds
Change-Id: Ib095251d3e21e77ed50cc3575388107fecec4ecd
miodragdinic pushed a commit to MIPS/prebuilts-ndk that referenced this issue Apr 17, 2018
This is a partial https://android-review.googlesource.com/c/343952/.

We shipped this in r14, but r15 beta 1 is shipping the O oreview
headers, which is older than this change.

Test: ./validate.py
Bug: android/ndk#324
Change-Id: Ib301aba158f014d4fe2bf49aa7f2c1044c582f31
(cherry picked from commit 4e12436cc5af230b083e07ef3bea1d8ef1af1cf0)
sepehrst pushed a commit to spsforks/android-bionic-libc that referenced this issue Apr 22, 2024
We had several bugs filed saying "if I set _FILE_OFFSET_BITS=64 when
targeting an API < L, various functions are missing". Instead of
saying "yes, they are", we quietly just modified the header files to
expose the non-64-bit variants. This makes no sense. We can't just say
"oh, yeah, we don't have a version of this function that agrees with
your calling code about how large off_t is, but here's a version that
doesn't: I'm sure it'll be fine".

_FILE_OFFSET_BITS=64 on Android LP32 has always been a game of chance,
but that game should be "are all the functions my code needs available
at compile time?", not "will my code actually work at run time?".

Bug: android/ndk#442
Bug: android/ndk#333
Bug: android/ndk#332
Bug: android/ndk#324
Test: builds
Change-Id: Ib095251d3e21e77ed50cc3575388107fecec4ecd
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

No branches or pull requests

4 participants