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

Migrate from GCC to CLANG #240

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

clebeaupin
Copy link
Contributor

No description provided.

extern "C" {
#endif

int __cxa_thread_atexit(void (*func)(), void *obj,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clang shipped with Android does not implement __cxa_thread_atexit. The following implementation is incomplete.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quote from http://clang.llvm.org/cxx_status.html#n2659

(5): thread_local support requires a C++ runtime library providing __cxa_thread_atexit, such as libc++abi 3.6 or later, or libsupc++ 4.8 or later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: I disabled unnecessary Future support in the LSD branch, so I will give Clang a try with NDK 13

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libc++abi has been updated and now has a fallback implementation of __cxa_thread_atexit_impl. Non-global thread_local variables with non-trivial destructors are now supported.

https://github.com/android-ndk/ndk/wiki/Changelog-r14

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Daniel, we had talked about removing altogether thread_local support from ReadiumSDK since Apple considers it a private API. Does removing that affect this issue?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the lcp branch we totally disabled the std::future etc. features, so this should indeed be a moot point at this stage. I just wanted to bring people's attention to the NDK changes.

@danielweck
Copy link
Member

danielweck commented Oct 19, 2016

Another Clang fix:
https://github.com/readium/readium-sdk/blob/develop/ePub3/ThirdParty/sha1/sha1.h#L30

        static void SHA1::storeBigEndianUint32( unsigned char* byte, Uint32 num );
        static void SHA1::hexPrinter( unsigned char* c, int l );

(the SHA1:: qualifier must be removed)

@danielweck
Copy link
Member

danielweck commented Oct 19, 2016

Info: I am using this code diff for the LSD feature branch, so I can test Clang instead of GCC in my Android build (with which I currently experience random segmentation faults). armv7 and x86 seem to be compiling fine...I will report later about linking and runtime. Watch this space :)
See: f65bfd8

danielweck added a commit that referenced this pull request Oct 19, 2016
#240 migration from GCC to
CLANG on Android
@danielweck
Copy link
Member

Ah, I finished fixing compilation and linking bugs in my LCP/LSD branch (Android launcher app), and I now get runtime segmentation faults on std::regex (when I attempt to load a LCP file), and libzip (when I attempt to load an EPUB archive). Damn!! :(
I believe we are using libc++ here, not libstdc++, right?

@danielweck
Copy link
Member

danielweck commented Oct 22, 2016

Update: I implemented a boolean flag in local.properties similar to readium.ndk_debug (which is used to activate the Gradle "experimental" plugin that allows hybrid Java / C++ LLDB debugging) to easily control switching from GCC to CLANG: readium.ndk_clang (true or false). Whilst I was at it, I also implemented X86 and ARM switches (otherwise both build and it is quite time-consuming).
Code in the feature/lsd branches:
LCP client lib:
readium/readium-lcp-client@69a7c49
Readium SDK:
997fe7a
Android app:
readium/SDKLauncher-Android@a92e41a

@danielweck
Copy link
Member

Interesting remarks about the LLVM libc++ runtime (c++_static and c++_shared / libc++_shared.so):
https://developer.android.com/ndk/guides/cpp-support.html
"
The NDK's libc++ is not stable. Not all the tests pass, and the test suite is not comprehensive. Some known issues are:
Using c++_shared on ARM can crash when an exception is thrown.
"

@danielweck
Copy link
Member

Note that the static ICU4c libs are pre-compiled with GCC, so this might need to be ported too:
https://github.com/readium/readium-sdk/tree/develop/ePub3/ThirdParty/icu4c/lib
...oh, and Android's libxml2 is patched to support ICU unicode conversions:
https://github.com/readium/readium-sdk/blob/develop/ePub3/ThirdParty/libxml2-android/patches/0001-Add-ICU-support-for-libxml.patch

@danielweck
Copy link
Member

Regarding the ICU4c lib, there are build instructions here:
#245
(ARM 64)

@danielweck
Copy link
Member

Actually, ICU is not even required. See last comment in this PR discussion thread:
#245 (comment)

@danielweck
Copy link
Member

Note that I have successfully been using NDK 13 with GCC / gnustl (shared lib), but Clang fails to link with libc++ (also shared lib), and the Clang build with gnustl fails at runtime. Google will not fully deprecate GCC until the remaining libc++ instability / missing API issues are solved, so we can continue to use Gcc for now.

Also note that sometimes Android Studio's own update channel announces a new NDK download before it is available in the Android SDK Manager (or on the official NDK web page).
This is actually why/how I have been using NDK 13b for the past few days.
Regarding the change log, I was advised to check this wiki page:
https://github.com/android-ndk/ndk/wiki
...in addition to:
https://developer.android.com/ndk/downloads/index.html

@rkwright
Copy link
Member

rkwright commented Mar 3, 2017

@danielweck @bluefirepatrick @clebeaupin @llemeurfr
I STRONGLY feel that we should simply REMOVE all the code associated with these future/synch code that is in the SDK. 0.26 and the merger of the LCP code seems like a really confluent point in time to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants