add android support #171

SchrodingerZhu · 2020-04-13T06:11:02Z

No description provided.

SchrodingerZhu · 2020-04-13T06:13:09Z

You can test the compiled binaries with the methods mentioned in https://stackoverflow.com/questions/9868309/how-to-compile-c-into-an-executable-binary-file-and-run-it-in-android-from-andro.

works fine on my side.

SchrodingerZhu · 2020-04-13T08:46:16Z

@mjp41 is it better to implement a specified stacktrace using libunwind here rather than simply removing it?

davidchisnall · 2020-04-13T10:19:20Z

Does Android have libexecinfo? It would be nice to make the backtrace support conditional on the backtrace symbols existing, with a CMake check.

mjp41 · 2020-04-13T10:27:20Z

@davidchisnall are you proposing a check like we do in Verona for filesystem support:

https://github.com/microsoft/verona/blob/f193b1ce246333d6826e4538bebb29e117996ce6/src/compiler/CMakeLists.txt#L3-L11

SchrodingerZhu · 2020-04-13T12:33:07Z

I do not know whether this would work or not, will give it a try: https://cmake.org/cmake/help/v3.0/module/CheckLibraryExists.html

SchrodingerZhu · 2020-04-13T13:16:04Z

CMakeLists.txt

 endif()

+check_include_file(execinfo.h HAS_EXECINFO_H)


This macro can help us check the header file directly.

mjp41

LGTM

mjp41 · 2020-04-14T10:47:13Z

Could you add some documentation about how to use this feature?

SchrodingerZhu · 2020-04-14T12:13:33Z

CMakeLists.txt

@@ -74,6 +74,11 @@ target_include_directories(snmalloc_lib INTERFACE src/)

 if(NOT MSVC)
  find_package(Threads REQUIRED COMPONENTS snmalloc_lib)
+  if (SNMALLOC_ARM_ANDROID)
+    if (NOT "${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
+      target_link_libraries(snmalloc_lib INTERFACE log c++_static c++abi m)


with this I can build the library and run it correctly for armv7, but the size is larger.

dynamic linking failed to resolve __emutls_get_address

SchrodingerZhu · 2020-04-14T12:16:56Z

CMakeLists.txt

@@ -10,10 +10,10 @@ option(EXPOSE_EXTERNAL_PAGEMAP "Expose the global pagemap" OFF)
 option(EXPOSE_EXTERNAL_RESERVE "Expose an interface to reserve memory using the default memory provider" OFF)
 option(SNMALLOC_RUST_SUPPORT "Build static library for rust" OFF)
 option(SNMALLOC_QEMU_WORKAROUND "Disable using madvise(DONT_NEED) to zero memory on Linux" Off)
-option(SNMALLOC_ANDROID "Set cross build target to Android" OFF)
+option(SNMALLOC_ARM_ANDROID "Set cross build target to ARM Android" OFF)


The reason why I change this is that android does not always mean arm

You are right. CMAKE_TOOLCHAIN_FILE is definitely a good choice, will look into it later.

I set this just because passing CMAKE_SYSTEM_PROCESSOR via command line is not working.

It should be set by android.toolchain.cmake.

using toolchain file for androideabi will solve the linking issue too. the linking result of emutls after changing to the new way:

0002d7e0 t __emutls_get_address 0002d8b8 t __emutls_register_common 0003eff4 r __emutls_t._ZL16g_current_locale 0003e15c r __emutls_t._ZN10__cxxabiv112_GLOBAL__N_15dtorsE 00040dc0 V __emutls_t._ZZN8snmalloc17ThreadAllocCommon13get_referenceEvE5alloc 0002d7c8 t __emutls_unregister_key 00043020 V __emutls_v._ZGVZN8snmalloc27ThreadAllocThreadDestructor16register_cleanupEvE6tidier 00043080 d __emutls_v._ZL16g_current_locale 00043050 d __emutls_v._ZN10__cxxabiv112_GLOBAL__N_111dtors_aliveE 00043060 d __emutls_v._ZN10__cxxabiv112_GLOBAL__N_15dtorsE 00043030 V __emutls_v._ZN8snmalloc17ThreadAllocCommon18destructor_has_runE 00043040 V __emutls_v._ZN8snmalloc19operation_in_flightE 00043000 V __emutls_v._ZZN8snmalloc17ThreadAllocCommon13get_referenceEvE5alloc 00043010 V __emutls_v._ZZN8snmalloc27ThreadAllocThreadDestructor16register_cleanupEvE6tidier 0002d6ec t emutls_alloc 0002d6a8 t emutls_destroy 0002d784 t emutls_init 00046dd8 b emutls_key 00046de0 b emutls_key_created 00046ddc b emutls_mutex 00046de8 b emutls_size

CMakeLists.txt

@@ -10,10 +10,10 @@ option(EXPOSE_EXTERNAL_PAGEMAP "Expose the global pagemap" OFF)
 option(EXPOSE_EXTERNAL_RESERVE "Expose an interface to reserve memory using the default memory provider" OFF)
 option(SNMALLOC_RUST_SUPPORT "Build static library for rust" OFF)
 option(SNMALLOC_QEMU_WORKAROUND "Disable using madvise(DONT_NEED) to zero memory on Linux" Off)
-option(SNMALLOC_ANDROID "Set cross build target to Android" OFF)
+option(SNMALLOC_ARM_ANDROID "Set cross build target to ARM Android" OFF)


CMakeLists.txt

 endif()

+check_include_file(execinfo.h HAS_EXECINFO_H)


davidchisnall · 2020-04-14T13:30:05Z

src/override/malloc.cc

+  SNMALLOC_EXPORT
+  size_t SNMALLOC_NAME_MANGLE(malloc_usable_size)(const void* ptr)
+  {
+    return Alloc::alloc_size(const_cast<void*>(ptr));


alloc_size shouldn't need to modify anything through the pointer. I'd prefer to pass a const void* here and unify both code paths.

Do you mean changing the inner paths within snmalloc?

Yes, exactly. I don't think Alloc::alloc_size should need to take a non-const pointer, so let's make that one const.

src/pal/pal_posix.h

davidchisnall · 2020-04-14T13:34:18Z

Thanks, I've added a few comments inline, but they're all minor cleanups.

SchrodingerZhu · 2020-04-15T03:11:47Z

using toolchain file however, may make it hard for other language like rust to automatically compile this library. see rust-lang/cmake-rs#80.

I think I will adopt a dirty hack to let the user pass a ANDROID_NDK environment variable in my rust crate. Do you have any thought on how to improve this?

davidchisnall · 2020-04-15T08:15:16Z

Does Rust depend on the NDK at all for Android cross-compilation? I don't have much experience there, but it looks as if the NDK has a fairly standard CMake cross-compile process and is gradually moving in the direction of better alignment with the normal CMake mechanisms where they differ.

I'd assume that if you're cross-compiling any CMake project you'd use a toolchain file (every other approach I've tried in the past has resulted in far more pain in the long term), but the cmake-rs issue that you link to seems to imply that they don't want to do that.

davidchisnall · 2020-04-15T08:18:44Z

CMakeLists.txt

@@ -210,7 +209,7 @@ if(NOT DEFINED SNMALLOC_ONLY_HEADER_LIBRARY)
    add_shim(snmallocshim-1mib SHARED ${SHARED_FILES})
    target_compile_definitions(snmallocshim-1mib PRIVATE IS_ADDRESS_SPACE_CONSTRAINED)
  endif()
-
+  


The only changes in this file are now whitespace. Can you remove them to reduce churn?

I have just rebased away the changes on CMakeLists.txt

plietar · 2020-04-15T10:51:52Z

i686 target is not supported because of the design of snmalloc.

Is there a reason for this? We have 32-bit windows on the CI, and I can get 32-bit linux building with a very small change (just a spurious #error in aba.h, I'll make a PR later).

SchrodingerZhu · 2020-04-15T11:11:37Z

i686 target is not supported because of the design of snmalloc.

Is there a reason for this? We have 32-bit windows on the CI, and I can get 32-bit linux building with a very small change (just a spurious #error in aba.h, I'll make a PR later).

@plietar It was the previous discussion that make me write something like this. Perhaps my words were not proper here. Sorry about that.
#137 (comment)

SchrodingerZhu · 2020-04-15T12:26:03Z

src/ds/address.h

@@ -70,7 +70,7 @@ namespace snmalloc
   * power of two.
   */
  template<size_t alignment, typename T = void>
-  SNMALLOC_FAST_PATH T* pointer_align_down(void* p)
+  SNMALLOC_FAST_PATH T* pointer_align_down(const void* p)


Notice that this result in an inconsistent qualifier with other alignment cast operations.

davidchisnall · 2020-04-15T13:36:40Z

src/override/malloc.cc

@@ -43,7 +43,8 @@ extern "C"
    return ThreadAlloc::get_noncachable()->alloc<ZeroMem::YesZero>(sz);
  }

-  SNMALLOC_EXPORT size_t SNMALLOC_NAME_MANGLE(malloc_usable_size)(void* ptr)
+  SNMALLOC_EXPORT
+    size_t SNMALLOC_NAME_MANGLE(malloc_usable_size)(const void* ptr)


I think you still need the non-const version for other platforms that declare this in a header that we include here.

Currently, it seems to work for other platforms.

I guess the case seems to be this:

Android somehow includes the malloc.h via the headers.

Other platforms does not do this.

Compiler discards CV-qualifiers so linking stage is not affected for most platforms.

However, the definitions conflicts on android because the compiler can find the real definitions.

So, do you still suggest me to provide two different versions? if so, I will work on it ASAP.

@davidchisnall How about something like:

include(CheckCSourceCompiles) CHECK_C_SOURCE_COMPILES(" #include <malloc.h> size_t malloc_usable_size(const void* ptr) { return 0; } int main() { return 0; } " CONST_QUALIFIED_MALLOC_USABLE_SIZE) if(CONST_QUALIFIED_MALLOC_USABLE_SIZE) target_compile_definitions(snmalloc_lib INTERFACE -DMALLOC_USABLE_SIZE_QUALIFIER=const) endif()

And then in the source,

#ifndef MALLOC_USABLE_SIZE_QUALIFIER # define MALLOC_USABLE_SIZE_QUALIFIER #endif // ... size_t SNMALLOC_NAME_MANGLE(malloc_usable_size)(MALLOC_USABLE_SIZE_QUALIFIER void* ptr) {...}

SchrodingerZhu · 2020-04-16T07:06:19Z

Rebased again to master:

remove i686 remarks in documentation
use cmake to detect the const qualifier

mjp41 · 2020-04-18T06:58:51Z

@SchrodingerZhu thanks for this.

SchrodingerZhu marked this pull request as ready for review April 13, 2020 06:11

SchrodingerZhu commented Apr 13, 2020

View reviewed changes

mjp41 approved these changes Apr 14, 2020

View reviewed changes

SchrodingerZhu commented Apr 14, 2020

View reviewed changes

davidchisnall reviewed Apr 14, 2020

View reviewed changes

davidchisnall reviewed Apr 15, 2020

View reviewed changes

SchrodingerZhu commented Apr 15, 2020

View reviewed changes

davidchisnall reviewed Apr 15, 2020

View reviewed changes

SchrodingerZhu added 5 commits April 16, 2020 14:59

adjust for android

7a1c7b7

update docs

244ab18

add const qualifier to unify the path

8591aa4

move const qualifer out of the alignment cast

7cfb413

check const qualifier in cmake

1483cff

SchrodingerZhu requested review from mjp41 and davidchisnall April 16, 2020 07:41

mjp41 approved these changes Apr 16, 2020

View reviewed changes

mjp41 merged commit a43773c into microsoft:master Apr 18, 2020

mjp41 mentioned this pull request Apr 18, 2020

Android Support #163

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add android support #171

add android support #171

SchrodingerZhu commented Apr 13, 2020

SchrodingerZhu commented Apr 13, 2020

SchrodingerZhu commented Apr 13, 2020

davidchisnall commented Apr 13, 2020

mjp41 commented Apr 13, 2020

SchrodingerZhu commented Apr 13, 2020

SchrodingerZhu Apr 13, 2020

This comment was marked as resolved.

mjp41 left a comment

mjp41 commented Apr 14, 2020

SchrodingerZhu Apr 14, 2020

SchrodingerZhu Apr 14, 2020

SchrodingerZhu Apr 14, 2020

This comment was marked as resolved.

SchrodingerZhu Apr 14, 2020

SchrodingerZhu Apr 15, 2020 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

davidchisnall Apr 14, 2020

SchrodingerZhu Apr 15, 2020

davidchisnall Apr 15, 2020

davidchisnall commented Apr 14, 2020

SchrodingerZhu commented Apr 15, 2020

davidchisnall commented Apr 15, 2020

davidchisnall Apr 15, 2020

SchrodingerZhu Apr 15, 2020

plietar commented Apr 15, 2020

SchrodingerZhu commented Apr 15, 2020

SchrodingerZhu Apr 15, 2020

davidchisnall Apr 15, 2020

SchrodingerZhu Apr 15, 2020

SchrodingerZhu Apr 15, 2020 •

edited

Loading

SchrodingerZhu commented Apr 16, 2020 •

edited

Loading

mjp41 commented Apr 18, 2020

add android support #171

add android support #171

Conversation

SchrodingerZhu commented Apr 13, 2020

SchrodingerZhu commented Apr 13, 2020

SchrodingerZhu commented Apr 13, 2020

davidchisnall commented Apr 13, 2020

mjp41 commented Apr 13, 2020

SchrodingerZhu commented Apr 13, 2020

Choose a reason for hiding this comment

This comment was marked as resolved.

mjp41 left a comment

Choose a reason for hiding this comment

mjp41 commented Apr 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

SchrodingerZhu Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidchisnall commented Apr 14, 2020

SchrodingerZhu commented Apr 15, 2020

davidchisnall commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plietar commented Apr 15, 2020

SchrodingerZhu commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SchrodingerZhu Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

SchrodingerZhu commented Apr 16, 2020 • edited Loading

mjp41 commented Apr 18, 2020

SchrodingerZhu Apr 15, 2020 •

edited

Loading

SchrodingerZhu Apr 15, 2020 •

edited

Loading

SchrodingerZhu commented Apr 16, 2020 •

edited

Loading