Skip to content

[android] Put in fixes for librt and armv7-a #554

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 24, 2021
Merged

[android] Put in fixes for librt and armv7-a #554

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: 3 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,9 @@ if(__BUILTIN_TRAP)
set(HAVE_NORETURN_BUILTIN_TRAP 1)
endif()

find_package(LibRT)
if(NOT CMAKE_SYSTEM_NAME STREQUAL Android)
find_package(LibRT)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

This is fine to do, but really unfortunate. CMake already already can deal with librt properly on various targets, but we don't use CMake properly :-(.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you could use CMAKE_FIND_ROOT_PATH, like I do when cross-compiling the corelibs, if there is an librt.so to find, but there is none on Android- it's just part of libc.so- so this is the right change for Android.


check_function_exists(_pthread_workqueue_init HAVE__PTHREAD_WORKQUEUE_INIT)
check_function_exists(getprogname HAVE_GETPROGNAME)
Expand Down
8 changes: 8 additions & 0 deletions tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,14 @@ function(add_unit_test name)
target_compile_options(${name} PRIVATE -fblocks)
target_compile_options(${name} PRIVATE -Wall -Wno-deprecated-declarations)
endif()
# Without this flag, cross-compiling static test executables for Android armv7
# fails with the multiple definition errors seen in android/ndk#176, so I
# pulled in this workaround noted there. The tests build and run with this
# flag applied.
if(NOT BUILD_SHARED_LIBS AND CMAKE_SYSTEM_NAME STREQUAL Android AND
CMAKE_SYSTEM_PROCESSOR STREQUAL armv7-a)
target_link_options(${name} PRIVATE "LINKER:--allow-multiple-definition")
endif()
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs a huge comment. This is indicative of an issue - the multiple definitions shouldnt be there. The warnings indicate that there is a mix up between libgcc and libunwind, which is a really bad thing as it will crash at runtime in a difficult to diagnose manner (the internal layouts of the unwind context is not compatible/guaranteed).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I don't know exactly what's going on here, their doc doesn't clear it up for me. All I know is that applying this workaround flag gets the test executables to build, and I tried moving a handful to Android and they ran fine. I will add a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the comment.

target_link_libraries(${name}
PRIVATE
dispatch
Expand Down