Skip to content
This repository was archived by the owner on May 6, 2025. It is now read-only.

Migrate from GCC to CLANG #240

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

Migrate from GCC to CLANG #240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Platform/Android/epub3/Application.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
APP_ABI := armeabi-v7a x86
APP_PLATFORM := android-22
NDK_TOOLCHAIN_VERSION := 4.9
APP_STL := gnustl_static
NDK_TOOLCHAIN_VERSION := clang
APP_STL := c++_static
8 changes: 5 additions & 3 deletions Platform/Android/epub3/Experimental.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,14 +137,14 @@ include $(BUILD_STATIC_LIBRARY)

include $(CLEAR_VARS)
LOCAL_MODULE := epub3
LOCAL_CPPFLAGS := -std=gnu++11 -fpermissive -DBUILDING_EPUB3
LOCAL_CFLAGS := -DBUILDING_EPUB3
LOCAL_CPPFLAGS := -std=c++11 -fpermissive -DBUILDING_EPUB3 -D_LIBCPP_INLINE_VISIBILITY_EXCEPT_GCC49=_LIBCPP_INLINE_VISIBILITY
LOCAL_CFLAGS := -DBUILDING_EPUB3 -D_LIBCPP_INLINE_VISIBILITY_EXCEPT_GCC49=_LIBCPP_INLINE_VISIBILITY
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to define _LIBCPP_INLINE_VISIBILITY_EXCEPT_GCC49 to avoid issue in NDK file sources/cxx-stl/llvm-libc++/libcxx/include/string


ifeq ($(TARGET_ARCH_ABI),x86)
LOCAL_CFLAGS += -mtune=atom -mssse3 -mfpmath=sse
endif

LOCAL_CXXFLAGS := -std=gnu++11 -fpermissive -DBUILDING_EPUB3
LOCAL_CXXFLAGS := -std=c++11 -fpermissive -DBUILDING_EPUB3 -D_LIBCPP_INLINE_VISIBILITY_EXCEPT_GCC49=_LIBCPP_INLINE_VISIBILITY
LOCAL_CPP_FEATURES += exceptions rtti
LOCAL_STATIC_LIBRARIES := xml2 crypto
LOCAL_LDLIBS := -lz -landroid -llog
Expand Down Expand Up @@ -252,6 +252,8 @@ LOCAL_SRC_FILES := \
$(EPUB3_PATH)/utilities/ring_buffer.cpp \
$(EPUB3_PATH)/utilities/run_loop_android.cpp \
$(EPUB3_PATH)/utilities/utfstring.cpp \
$(wildcard $(LOCAL_PATH)/src/main/jni/*.cpp) \
$(wildcard $(LOCAL_PATH)/src/main/jni/jni/*.cpp)

include $(BUILD_STATIC_LIBRARY)

11 changes: 6 additions & 5 deletions Platform/Android/epub3/Stable.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,14 +138,14 @@ include $(BUILD_STATIC_LIBRARY)
include $(CLEAR_VARS)
LOCAL_DISABLE_FATAL_LINKER_WARNINGS := true
LOCAL_MODULE := epub3
LOCAL_CPPFLAGS := -std=gnu++11 -fpermissive -DBUILDING_EPUB3
LOCAL_CFLAGS := -DBUILDING_EPUB3
LOCAL_CPPFLAGS := -std=c++11 -fpermissive -DBUILDING_EPUB3 -D_LIBCPP_INLINE_VISIBILITY_EXCEPT_GCC49=_LIBCPP_INLINE_VISIBILITY
LOCAL_CFLAGS := -DBUILDING_EPUB3 -D_LIBCPP_INLINE_VISIBILITY_EXCEPT_GCC49=_LIBCPP_INLINE_VISIBILITY

ifeq ($(TARGET_ARCH_ABI),x86)
LOCAL_CFLAGS += -mtune=atom -mssse3 -mfpmath=sse
endif

LOCAL_CXXFLAGS := -std=gnu++11 -fpermissive -DBUILDING_EPUB3
LOCAL_CXXFLAGS := -std=c++11 -fpermissive -DBUILDING_EPUB3 -D_LIBCPP_INLINE_VISIBILITY_EXCEPT_GCC49=_LIBCPP_INLINE_VISIBILITY
LOCAL_CPP_FEATURES += exceptions rtti
LOCAL_STATIC_LIBRARIES := xml2 crypto
LOCAL_LDLIBS := -lz -landroid -llog
Expand Down Expand Up @@ -255,8 +255,9 @@ LOCAL_SRC_FILES := \
$(EPUB3_PATH)/utilities/utfstring.cpp \
$(wildcard $(EPUB3_PATH)/ePub/*.cpp) \
$(wildcard $(LOCAL_PATH)/src/main/jni/*.cpp) \
$(wildcard $(LOCAL_PATH)/src/main/jni/jni/*.cpp) \
$(wildcard $(LOCAL_PATH)/src/main/jni/android/*.cpp)
$(wildcard $(LOCAL_PATH)/src/main/jni/jni/*.cpp)

#$(wildcard $(LOCAL_PATH)/src/main/jni/android/*.cpp)
Copy link
Contributor Author

@clebeaupin clebeaupin May 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Do not continue to include jni/android. is backup_atomics useless ?

Copy link
Member

Choose a reason for hiding this comment

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

Right...I wonder too...
https://github.com/readium/readium-sdk/tree/develop/Platform/Android/epub3/src/main/jni/android
PS: OMFG_WHERE_ARE_ALL_THE_ATOMIC_INTRINSICS LOL


include $(BUILD_SHARED_LIBRARY)

7 changes: 4 additions & 3 deletions Platform/Android/epub3/build_experimental.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ model {
jni {
source {
srcDirs = [
'./src/main/jni',
'../../../ePub3/ePub'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compile using Mk

Copy link
Member

Choose a reason for hiding this comment

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

Does this remove the JNI source tree from the Android Studio navigation view on the left hand side? (or is it just a low-level Gradle compile rule?)

]
}
Expand All @@ -24,14 +23,16 @@ model {
android.ndk {
moduleName = "epub3"

stl = "gnustl_shared"
toolchain "clang"
stl = "c++_shared"

cppFlags.addAll([
"-std=gnu++11",
"-std=c++11",
"-fpermissive",
"-fexceptions",
"-frtti",
"-DBUILDING_EPUB3",
"-D_LIBCPP_INLINE_VISIBILITY_EXCEPT_GCC49=_LIBCPP_INLINE_VISIBILITY",
"-I../prefix.h",
"-I${file("./include")}".toString(),
"-I${file("./include/ePub3")}".toString(),
Expand Down
2 changes: 1 addition & 1 deletion Platform/Android/epub3/src/main/jni/epub3.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ std::shared_ptr<void> getNativePtr(JNIEnv *env, jobject thiz) {
* Optionally it may free the native string if the freeNative argument
* is true.
*/
jstring toJstring(JNIEnv *env, const char* str, bool freeNative = false) {
jstring toJstring(JNIEnv *env, const char* str, bool freeNative) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Warning on GCC but error on CLANG. Default values must be defined in .h not in .cpp

if (str == NULL) {
return NULL;
}
Expand Down
2 changes: 1 addition & 1 deletion Platform/Android/epub3/src/main/jni/jni/jni_ptr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ namespace jni {
* POINTER_GPS(name) to have a complete dump of the leaks at
* the end of your program execution.
*/
Pointer::Pointer(sptr ptr, std::string name = "") : _id((jlong) ptr.get()), _ptr(ptr), _name(name) {
Pointer::Pointer(sptr ptr, std::string name) : _id((jlong) ptr.get()), _ptr(ptr), _name(name) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Warning on GCC but error on CLANG. Default values must be defined in .h not in .cpp

// Prevent from adding nullptr to pointer pool
if(ptr != nullptr) {
PointerPool::add((Pointer)*this);
Expand Down
4 changes: 2 additions & 2 deletions ePub3/_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,14 +106,14 @@ typedef signed long ssize_t;
#else
# define EPUB3_EXPORT
#endif

/*
#if EPUB_OS(ANDROID)
//# define UTF_USE_ICU 1
# define CXX11_STRING_UNAVAILABLE 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why CXX11 is unavailable. I don't think so. So I comment this block

# if EPUB_COMPILER(CLANG)
# define nan(x) __builtin_nan(x)
# endif
#endif
#endif*/

#if EPUB_COMPILER(GCC) && !EPUB_COMPILER(CLANG)
# if GCC_VERSION_AT_LEAST(4, 7, 0)
Expand Down
8 changes: 4 additions & 4 deletions ePub3/_platform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@
#define EPUB_CPU_ARM 1

#if EPUB_COMPILER(CLANG) && defined(ANDROID)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have commented these lines. Is backup_atomics necessary ?

Copy link
Member

@danielweck danielweck May 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@AlanQuatermain (Jim Dovey)
Do you remember why / if OMFG_WHERE_ARE_ALL_THE_ATOMIC_INTRINSICS was needed?
https://github.com/readium/readium-sdk/tree/develop/Platform/Android/epub3/src/main/jni/android
(backup_atomics)

# define __atomic_fetch_add(mem, val, typ) __sync_fetch_and_add(mem, val)
/*# define __atomic_fetch_add(mem, val, typ) __sync_fetch_and_add(mem, val)
# define __atomic_fetch_sub(mem, val, typ) __sync_fetch_and_sub(mem, val)
# define __atomic_fetch_and(mem, val, typ) __sync_fetch_and_and(mem, val)
# define __atomic_fetch_or(mem, val, typ) __sync_fetch_and_or(mem, val)
Expand All @@ -176,17 +176,17 @@
# define __atomic_and_fetch(mem, val, typ) __sync_and_and_fetch(mem, val)
# define __atomic_or_fetch(mem, val, typ) __sync_or_and_fetch(mem, val)
# define __atomic_xor_fetch(mem, val, typ) __sync_xor_and_fetch(mem, val)
# define __atomic_nand_fetch(mem, val, typ) __sync_nand_and_fetch(mem, val)
# define __atomic_nand_fetch(mem, val, typ) __sync_nand_and_fetch(mem, val)*/
/*# define __atomic_load atomic_load
# define __atomic_store atomic_store
# define __atomic_exchange atomic_exchange*/
# define __atomic_load __sw_atomic_load
/*# define __atomic_load __sw_atomic_load
# define __atomic_load_n(a,m) (*(a))
# define __atomic_store __sw_atomic_store
# define __atomic_store_n(a,m) (*(a))
# define __atomic_exchange __sw_atomic_exchange
# define __atomic_exchange_n __sw_atomic_exchange
# include <backup_atomics.h>
# include <backup_atomics.h>*/
#endif

#if defined(__ARM_PCS_VFP)
Expand Down
18 changes: 17 additions & 1 deletion ePub3/utilities/future.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
#include <ePub3/epub3.h>
#include <system_error>
#include "future.h"

/*
#if EPUB_PLATFORM(ANDROID)
namespace std
{
Expand Down Expand Up @@ -79,6 +79,22 @@ namespace std

}
#endif
*/


#if EPUB_COMPILER(CLANG) && defined(ANDROID)
#ifdef __cplusplus
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.

For reference: http://clang.llvm.org/cxx_status.html#n2659

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.

void *dso_symbol) {
return 0;
}
#ifdef __cplusplus
}
#endif
#endif

EPUB3_BEGIN_NAMESPACE

Expand Down