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: basename in string.h defined for all versions if defined(__cplusplus) #440

Closed
fornwall opened this issue Jun 30, 2017 · 8 comments
Assignees
Milestone

Comments

@fornwall
Copy link

From <string.h> in NDK r15b:

#if defined(__USE_GNU) && !defined(basename)
/*
 * glibc has a basename in <string.h> that's different to the POSIX one in <libgen.h>.
 * It doesn't modify its argument, and in C++ it's const-correct.
 */
#if defined(__cplusplus)
extern "C++" char* basename(char* _Nonnull) __RENAME(__gnu_basename);
extern "C++" const char* basename(const char* _Nonnull) __RENAME(__gnu_basename);
#else

#if __ANDROID_API__ >= 23
char* basename(const char* _Nonnull) __RENAME(__gnu_basename) __INTRODUCED_IN(23);
#endif /* __ANDROID_API__ >= 23 */

#endif
#endif

Is it really correct that basename is declared if C++ is used regardless of __ANDROID_API__ while android-23 is required for the C function?

@fornwall
Copy link
Author

Likewise with strchrnul:

#if defined(__USE_GNU)
#if defined(__cplusplus)
extern "C++" char* strchrnul(char* _Nonnull, int) __RENAME(strchrnul) __attribute_pure__;
extern "C++" const char* strchrnul(const char* _Nonnull, int) __RENAME(strchrnul) __attribute_pure__;
#else

#if __ANDROID_API__ >= 24
char* strchrnul(const char* _Nonnull, int) __attribute_pure__ __INTRODUCED_IN(24);
#endif /* __ANDROID_API__ >= 24 */

#endif
#endif

@jmgao jmgao self-assigned this Jun 30, 2017
fornwall added a commit to termux/termux-packages that referenced this issue Jun 30, 2017
See android/ndk#440

Fixes gdb build with unified headers.
@jmgao
Copy link
Contributor

jmgao commented Jun 30, 2017

Oops, this slipped through because the tool we have to verify correctness only builds the headers in C.

Header fix at https://android-review.googlesource.com/428060, but the preprocessor needs to be updated to build with C++ as well to insert guards.

hubot pushed a commit to aosp-mirror/platform_bionic that referenced this issue Jun 30, 2017
These still won't get guards added by the preprocessor, because it
compiles with C-only.

Bug: android/ndk#440
Test: treehugger
Change-Id: I893b345e528ed1b761e0db00700037411bbb8b78
@DanAlbert DanAlbert added this to the r16 milestone Jul 10, 2017
hubot pushed a commit to aosp-mirror/platform_bionic that referenced this issue Aug 16, 2017
The versioner doesn't handle C++ blocks yet, so these guards won't be
added and it will appear as though the functions are always available
in C++, but based on API level in C.

Test: make checkbuild
Bug: android/ndk#440
Change-Id: I31a20fa1596d836b280ffc6d7feb863afccca6c7
@DanAlbert DanAlbert removed this from the r16 milestone Aug 22, 2017
@DanAlbert
Copy link
Member

The user facing part of this is fixed, so taking off r16. @jmgao still needs to fix the versioner to make sure we don't have more mistakes like this one, but not necessary for r16 (aiui we're not going to be adding more like these anyway since libc++ already provides const-correct overloads for most of the C library).

@enh
Copy link
Contributor

enh commented Sep 8, 2017

did we introduce new problems for strcasestr and memrchr with https://android-review.googlesource.com/#/c/platform/bionic/+/455204/?

@DanAlbert DanAlbert added this to the unplanned milestone Oct 3, 2017
@DanAlbert
Copy link
Member

Guessing @jmgao won't be able to do any work on the versioner any time soon.

@enh
Copy link
Contributor

enh commented Oct 3, 2017

answering my own question about strcasestr and memrchr: no, they're fine because they're already surrounded by ANDROID_API checks for other reasons, or were there from the beginning anyway.

hubot pushed a commit to aosp-mirror/platform_bionic that referenced this issue Oct 26, 2017
Bug: android/ndk#440
Test: python run_tests.py
Change-Id: Id893979146bc609a17bd1fa2a6bec6f10dfe4804
hubot pushed a commit to aosp-mirror/platform_bionic that referenced this issue Oct 26, 2017
Some declarations, like bitfield members, don't need identifiers.

Bug: android/ndk#440
Test: ran versioner with -x c++ on a manually reduced <linux/timex.h>
Change-Id: Ic7eea780762cff653c54fdde4d10df203d630c25
hubot pushed a commit to aosp-mirror/platform_bionic that referenced this issue Nov 2, 2017
Attribute the versioning information on `void foo() __asm("bar")` to
bar, not foo.

The various long double functions in <math.h> run into this.

Bug: android/ndk#440
Test: python run_tests.py
Test: m
Change-Id: Idd3681ddbd006b4705608449935c9cfacfa3556e
hubot pushed a commit to aosp-mirror/platform_bionic that referenced this issue Nov 2, 2017
Bug: android/ndk#440
Test: python run_tests.py
Change-Id: Ib572a8fdcc00f6b88a25003a085b16ce9698d692
hubot pushed a commit to aosp-mirror/platform_bionic that referenced this issue Nov 7, 2017
extern "C" and "C++" are parsed as a LinkageSpecDecl with the real Decl
as a child node. This leads to the preprocessor sticking its guard
between the extern specifier and the declaration.

Update the AST visitor to add a special-case for calculating the
SourceRange on a LinkageSpecDecl, and add a test.

Bug: android/ndk#440
Test: python run_tests.py
Change-Id: I76445fe366cef46cfd2f16fb93d534d410c5edca
@jmgao jmgao assigned DanAlbert and unassigned jmgao Mar 6, 2018
@jmgao
Copy link
Contributor

jmgao commented Mar 6, 2018

AFAIK, the versioner support needed for this has been done for a while, should this be closed?

@jmgao
Copy link
Contributor

jmgao commented Mar 6, 2018

(After scrolling up and rereading the comments, yeah, looks like it.)

@jmgao jmgao closed this as completed Mar 6, 2018
sepehrst pushed a commit to spsforks/android-bionic-libc that referenced this issue Apr 22, 2024
Some declarations, like bitfield members, don't need identifiers.

Bug: android/ndk#440
Test: manually reduced <sys/timex.h>
Change-Id: Ic7eea780762cff653c54fdde4d10df203d630c25
sepehrst pushed a commit to spsforks/android-bionic-libc that referenced this issue Apr 22, 2024
Bug: android/ndk#440
Test: python run_tests.py
Change-Id: Ib572a8fdcc00f6b88a25003a085b16ce9698d692
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