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

Wrong DATAEND detection on Android #259

Closed
spaghettisalat opened this issue Jan 9, 2019 · 16 comments

Comments

@spaghettisalat
Copy link

commented Jan 9, 2019

bdwgc compiled with Android NDK r17c aborts with an "Wrong __data_start/_end pair" error when compiled with the (default) ld.gold linker (i.e. it works fine when using the ld.bfd linker with the "-fuse-ld=bfd" LDFLAGS option). I have observed this on an Android 4.1.x (API level 16) ARM device, but it will likely also occur in other versions. The issue is caused by the following workaround in include/private/gcconfig.h:

/* Workaround for Android NDK clang 3.5+ (as of NDK r10e) which does    */
/* not provide correct _end symbol.  Unfortunately, alternate __end__   */
/* symbol is provided only by NDK "bfd" linker.                         */
#if defined(PLATFORM_ANDROID) && defined(__clang__)
# undef DATAEND
# pragma weak __end__
  extern int __end__[];
# define DATAEND (__end__ != 0 ? (ptr_t)__end__ : (ptr_t)_end)
#endif

__end__ is nonzero, but smaller than GC_data_start.

Do you have any idea how the detection logic for the workaround can be improved? Another idea would be to just throw the workaround out and require a new enough NDK to compile bdwgc (unfortunately I don't know what version is new enough, since I don't have time to test out all NDK versions).

@ivmai

This comment has been minimized.

Copy link
Owner

commented Jan 9, 2019

Which clang version? In the absence of time to test all NDK versions, I think we could just turn off the workaround if clang version is at least the one you have tested with.

@spaghettisalat

This comment has been minimized.

Copy link
Author

commented Jan 9, 2019

./arm-linux-androideabi-clang --version
Android (4691093 based on r316199) clang version 6.0.2 (https://android.googlesource.com/toolchain/clang 183abd29fc496f55536e7d904e0abae47888fc7f) (https://android.googlesource.com/toolchain/llvm 34361f192e41ed6e4e8f9aca80a4ea7e9856f327) (based on LLVM 6.0.2svn)
@ivmai

This comment has been minimized.

Copy link
Owner

commented Jan 9, 2019

Is it correct that the workaround no longer needed for bfd linker?

@spaghettisalat

This comment has been minimized.

Copy link
Author

commented Jan 12, 2019

Yes, the situation summarized is:

Linker Workaround No Workaround
bfd works works
gold aborts works
@spaghettisalat

This comment has been minimized.

Copy link
Author

commented Jan 12, 2019

Unfortunately, it seems that the workaround is still needed for ARM64. With NDK r17c, same clang version as above, I'm observing the following behaviour with the Android emulator for Android API version 24 on arm64-v8a:

Linker Workaround No Workaround
bfd works aborts
gold aborts aborts
snmsts pushed a commit to roswell/ecl that referenced this issue Jan 13, 2019
…ARM64

    Works for Android NDKs with unified headers (see https://android.googlesource.com/platform/ndk/+/ndk-release-r16/docs/UnifiedHeaders.md).
    Separate step of configuring a standalone toolchain is recommended
    by the Android docs. Explicitely setting CC to clang is useful,
    since gcc in Android NDK is deprecated and outright broken in NDK
    version 18. Using the bfd linker is needed for bdwgc to work
    correctly (see also ivmai/bdwgc#259).
@ivmai

This comment has been minimized.

Copy link
Owner

commented Jan 14, 2019

I see, thank you. I will think of the fix.

@ivmai

This comment has been minimized.

Copy link
Owner

commented Jan 23, 2019

I use the following script to check symbols:

#!/bin/sh
set -e

if [ x$NDK_REL = x ]; then
NDK_REL=r18b
fi

NDK_PATH=/cygdrive/c/_/ProgramFiles/android-ndk-$NDK_REL
HOST_FLAVOR=windows-x86_64
GCC_VER=4.9

if [ x$NDK_API = x ]; then
NDK_API=28
fi

if [ $ARCH = arm ]; then
LLVM_TRIPLE=armv7-none-linux-androideabi
TOOLCHAIN_NAME=arm-linux-androideabi
TOOLCHAIN_FOLDER=$TOOLCHAIN_NAME
LIB_SUFFIX=
elif [ $ARCH = arm64 ]; then
LLVM_TRIPLE=aarch64-none-linux-android
TOOLCHAIN_NAME=aarch64-linux-android
TOOLCHAIN_FOLDER=$TOOLCHAIN_NAME
LIB_SUFFIX=
elif [ $ARCH = x86 ]; then
LLVM_TRIPLE=i686-none-linux-android
TOOLCHAIN_NAME=i686-linux-android
TOOLCHAIN_FOLDER=$ARCH
LIB_SUFFIX=
elif [ $ARCH = x86_64 ]; then
LLVM_TRIPLE=x86_64-none-linux-android
TOOLCHAIN_NAME=x86_64-linux-android
TOOLCHAIN_FOLDER=$ARCH
LIB_SUFFIX=64
fi

USE_LD_OPT=
#USE_LD=bfd
if [ x$USE_LD != x ]; then
USE_LD_OPT="-fuse-ld=$USE_LD"
else
USE_LD=gold
fi

TOOLCHAIN_PATH=$NDK_PATH/toolchains/$TOOLCHAIN_FOLDER-$GCC_VER/prebuilt/$HOST_FLAVOR
PLATFORM_USR_LIB=$NDK_PATH/platforms/android-$NDK_API/arch-$ARCH/usr/lib$LIB_SUFFIX
cp $PLATFORM_USR_LIB/crtbegin_dynamic.o $PLATFORM_USR_LIB/crtend_android.o .
$NDK_PATH/toolchains/llvm/prebuilt/$HOST_FLAVOR/bin/clang -target $LLVM_TRIPLE -I $NDK_PATH/sysroot/usr/include -I $NDK_PATH/sysroot/usr/include/$TOOLCHAIN_NAME -gcc-toolchain $TOOLCHAIN_PATH $USE_LD_OPT -I include -L $PLATFORM_USR_LIB -Wall -Wextra -D DONT_USE_ATEXIT -o gctest tests/test.c extra/gc.c $PLATFORM_USR_LIB/libc.a
rm crtbegin_dynamic.o crtend_android.o
echo $NDK_REL:$ARCH:android-$NDK_API:$USE_LD:
$TOOLCHAIN_PATH/$TOOLCHAIN_NAME/bin/readelf -s gctest > gctest.sym
grep --max-count=1 __end__ gctest.sym || echo "No __end__"
grep --max-count=1 " _end" gctest.sym || echo "No _end"
grep --max-count=1 _etext gctest.sym || echo "No _etext"
grep --max-count=1 __dso_handle gctest.sym || echo "No __dso_handle"
grep --max-count=1 __data_start gctest.sym || echo "No __data_start"
grep --max-count=1 " data_start" gctest.sym || echo "No data_start"

The invocation like this:
NDK_REL=r18b NDK_API=24 ARCH=arm USE_LD=bfd ndk-build-gctest.sh

@ivmai

This comment has been minimized.

Copy link
Owner

commented Jan 24, 2019

end is nonzero, but smaller than GC_data_start.

I have launched the above script (with the patch, gold linker is used by default):
NDK_REL=r17c NDK_API=16 ARCH=arm _x/ndk-build-gctest.sh

r17c:arm:android-16:gold:
    50: 00000000     0 NOTYPE  WEAK   DEFAULT  UND __end__
    52: 000d9df0     0 NOTYPE  GLOBAL DEFAULT  ABS _end
 23514: 0007c9c0     0 NOTYPE  GLOBAL DEFAULT  ABS _etext
 22392: 00080000     4 OBJECT  LOCAL  HIDDEN    24 __dso_handle
    49: 00000000     0 NOTYPE  WEAK   DEFAULT  UND __data_start
    56: 00000000     0 NOTYPE  WEAK   DEFAULT  UND data_start

So, I see that __end__ is zero. Do I miss something?

@ivmai

This comment has been minimized.

Copy link
Owner

commented Jan 24, 2019

@spaghettisalat Could you please check output of readelf (ndk r17c, api 16, arm, gold) regarding __end__ symbol?

@spaghettisalat

This comment has been minimized.

Copy link
Author

commented Jan 24, 2019

ivmai added a commit that referenced this issue Jan 25, 2019
Issue #259 (bdwgc).

To prevent use of __end__ symbol, "-D BROKEN_UUENDUU_SYM" should be
passed to CFLAGS.

* include/private/gcconfig.h [HOST_ANDROID && __clang__] (DATAEND):
Do not redefine to __end__ if BROKEN_UUENDUU_SYM.
@ivmai

This comment has been minimized.

Copy link
Owner

commented Jan 25, 2019

For now, I just added possibility to disable the workaround (commit d6bd4fa) by passing "-D BROKEN_UUENDUU_SYM" to CFLAGS.

1 similar comment
@ivmai

This comment has been minimized.

Copy link
Owner

commented Jan 25, 2019

For now, I just added possibility to disable the workaround (commit d6bd4fa) by passing "-D BROKEN_UUENDUU_SYM" to CFLAGS.

@spaghettisalat

This comment has been minimized.

Copy link
Author

commented Jan 26, 2019

readelf indeed shows that __end__ is zero:

r17c:arm:android-16:gold:
    50: 00000000     0 NOTYPE  WEAK   DEFAULT  UND __end__
    52: 000d9df0     0 NOTYPE  GLOBAL DEFAULT  ABS _end
 23514: 0007c840     0 NOTYPE  GLOBAL DEFAULT  ABS _etext
 22392: 00080000     4 OBJECT  LOCAL  HIDDEN    24 __dso_handle
    49: 00000000     0 NOTYPE  WEAK   DEFAULT  UND __data_start
    56: 00000000     0 NOTYPE  WEAK   DEFAULT  UND data_start

I also checked the shared library, which is used in the test app I'm actually testing with on Android, showing the same:

  2458: 00000000     0 NOTYPE  WEAK   DEFAULT  UND __end__

However bdwgc still aborts with

Wrong __data_start/_end pair: 0x53a56000 .. 0x4020301c
ivmai added a commit that referenced this issue Jan 28, 2019
Issue #259 (bdwgc).

To prevent use of __end__ symbol, "-D BROKEN_UUENDUU_SYM" should be
passed to CFLAGS.

* include/private/gcconfig.h [HOST_ANDROID && __clang__] (DATAEND):
Do not redefine to __end__ if BROKEN_UUENDUU_SYM.
@ivmai

This comment has been minimized.

Copy link
Owner

commented Jan 28, 2019

I see what's wrong. In case of ligc.a is linked statically into a console app end should point to the correct value. In a dynamic libgc is built then end points to the end of data section of app_process.
So, data start/end should determined using DYNAMIC_LOADING functionality.

ivmai added a commit that referenced this issue Feb 13, 2019
(fix of commits b746e63, 5e73ff1, a25965b)

Issue #259 (bdwgc).

* include/gc.h [(HOST_ANDROID || __ANDROID__)
&& IGNORE_DYNAMIC_LOADING] (_etext, __data_start, __end__, _end): Do
not declare weak symbols.
* os_dep.c [SEARCH_FOR_DATA_START && (LINUX || HURD) && HOST_ANDROID]
(etext, __dso_handle): Likewise.
* include/gc.h [(HOST_ANDROID || __ANDROID__)
&& IGNORE_DYNAMIC_LOADING] (GC_find_limit): Declare function as public.
* include/gc.h [(HOST_ANDROID || __ANDROID__)
&& IGNORE_DYNAMIC_LOADING] (GC_INIT_CONF_ROOTS): Update comment;
do not use _etext, __data_start, __end__, _end symbols; use
GC_find_limit(__dso_handle,1) as the end of the added GC data root.
* include/private/gc_priv.h [SEARCH_FOR_DATA_START
|| NETBSD && __ELF__] (GC_find_limit): Change ptr_t type to void*,
GC_bool to int.
* include/private/gcconfig.h [(SPARC || ALPHA) && FREEBSD]
(GC_find_limit): Likewise.
* os_dep.c [NEED_FIND_LIMIT || USE_PROC_FOR_LIBRARIES] (GC_find_limit):
Likewise.
* include/private/gcconfig.h [(SPARC || ALPHA) && FREEBSD]
(DATAEND): Cast the result to ptr_t.
* include/private/gcconfig.h [AARCH64 && LINUX && HOST_ANDROID]
(SEARCH_FOR_DATA_START): Remove outdated comment about __data_start.
* os_dep.c [SEARCH_FOR_DATA_START && (LINUX || HURD)
&& !IGNORE_PROG_DATA_START && HOST_ANDROID && !CPPCHECK]
(GC_init_linux_data_start): Do not compare __dso_handle to _etext and
do not use __dso_handle as data start.
* os_dep.c [SEARCH_FOR_DATA_START] (GC_init_linux_data_start): Cast the
result of GC_find_limit() to ptr_t.
* os_dep.c [NETBSD && __ELF__] (GC_init_netbsd_elf): Likewise.
* os_dep.c [LINUX_STACKBOTTOM && IA64] (GC_get_register_stack_base):
Likewise.
* os_dep.c [!AMIGA && !HAIKU && !OS2 && !MSWIN32 && !MSWINCE
&& !CYGWIN32 && !GC_OPENBSD_THREADS && (!GC_SOLARIS_THREADS
|| _STRICT_STDC)] (GC_get_main_stack_base): Likewise.
* os_dep.c [DATASTART_USES_BSDGETDATASTART] (GC_FreeBSDGetDataStart):
Likewise.
@ivmai

This comment has been minimized.

Copy link
Owner

commented Feb 13, 2019

Please test the latest master - should work for you (both arm and arm64, gold and bfd).

@ivmai ivmai closed this Feb 14, 2019
ivmai added a commit that referenced this issue Feb 15, 2019
(a cherry-pick of commit e1ef3b2 from 'master')

Issue #259 (bdwgc).

* include/gc.h [(HOST_ANDROID || __ANDROID__)
&& IGNORE_DYNAMIC_LOADING] (_etext, __data_start, __end__, _end): Do
not declare weak symbols.
* os_dep.c [SEARCH_FOR_DATA_START && (LINUX || HURD) && HOST_ANDROID]
(etext, __dso_handle): Likewise.
* include/gc.h [(HOST_ANDROID || __ANDROID__)
&& IGNORE_DYNAMIC_LOADING] (GC_find_limit): Declare function as public.
* include/gc.h [(HOST_ANDROID || __ANDROID__)
&& IGNORE_DYNAMIC_LOADING] (GC_INIT_CONF_ROOTS): Update comment;
do not use _etext, __data_start, __end__, _end symbols; use
GC_find_limit(__dso_handle,1) as the end of the added GC data root.
* include/private/gc_priv.h [SEARCH_FOR_DATA_START
|| NETBSD && __ELF__] (GC_find_limit): Change ptr_t type to void*,
GC_bool to int.
* include/private/gcconfig.h [(SPARC || ALPHA) && FREEBSD]
(GC_find_limit): Likewise.
* os_dep.c [NEED_FIND_LIMIT || USE_PROC_FOR_LIBRARIES] (GC_find_limit):
Likewise.
* include/private/gcconfig.h [(SPARC || ALPHA) && FREEBSD]
(DATAEND): Cast the result to ptr_t.
* include/private/gcconfig.h [AARCH64 && LINUX && HOST_ANDROID]
(SEARCH_FOR_DATA_START): Remove outdated comment about __data_start.
* os_dep.c [SEARCH_FOR_DATA_START && (LINUX || HURD)
&& !IGNORE_PROG_DATA_START && HOST_ANDROID]
(GC_init_linux_data_start): Do not compare __dso_handle to _etext and
do not use __dso_handle as data start.
* os_dep.c [SEARCH_FOR_DATA_START] (GC_init_linux_data_start): Cast the
result of GC_find_limit() to ptr_t.
* os_dep.c [NETBSD && __ELF__] (GC_init_netbsd_elf): Likewise.
* os_dep.c [LINUX_STACKBOTTOM && IA64] (GC_get_register_stack_base):
Likewise.
* os_dep.c [!AMIGA && !HAIKU && !OS2 && !MSWIN32 && !MSWINCE
&& !CYGWIN32 && !GC_OPENBSD_THREADS && (!GC_SOLARIS_THREADS
|| _STRICT_STDC)] (GC_get_main_stack_base): Likewise.
* os_dep.c [DATASTART_USES_BSDGETDATASTART] (GC_FreeBSDGetDataStart):
Likewise.
ivmai added a commit that referenced this issue Feb 16, 2019
(a cherry-pick of commit 7744b5d from 'release-8_0')

Issue #259 (bdwgc).

* include/gc.h [(PLATFORM_ANDROID || __ANDROID__)
&& IGNORE_DYNAMIC_LOADING] (_etext, __data_start, __end__, _end): Do
not declare weak symbols.
* os_dep.c [SEARCH_FOR_DATA_START && (LINUX || HURD) && PLATFORM_ANDROID]
(etext, __dso_handle): Likewise.
* include/gc.h [(PLATFORM_ANDROID || __ANDROID__)
&& IGNORE_DYNAMIC_LOADING] (GC_find_limit): Declare function as public.
* include/gc.h [(PLATFORM_ANDROID || __ANDROID__)
&& IGNORE_DYNAMIC_LOADING] (GC_INIT_CONF_ROOTS): Update comment;
do not use _etext, __data_start, __end__, _end symbols; use
GC_find_limit(__dso_handle,1) as the end of the added GC data root.
* include/private/gcconfig.h [(SPARC || ALPHA) && FREEBSD]
(GC_find_limit): Change ptr_t type to void*,
GC_bool to int.
* os_dep.c [SEARCH_FOR_DATA_START || NETBSD && __ELF__
|| NEED_FIND_LIMIT || USE_PROC_FOR_LIBRARIES] (GC_find_limit):
Likewise.
* include/private/gcconfig.h [(SPARC || ALPHA) && FREEBSD]
(DATAEND): Cast the result to ptr_t.
* os_dep.c [SEARCH_FOR_DATA_START && (LINUX || HURD)
&& !IGNORE_PROG_DATA_START && PLATFORM_ANDROID]
(GC_init_linux_data_start): Do not compare __dso_handle to _etext and
do not use __dso_handle as data start.
* os_dep.c [SEARCH_FOR_DATA_START] (GC_init_linux_data_start): Cast the
result of GC_find_limit() to ptr_t.
* os_dep.c [NETBSD && __ELF__] (GC_init_netbsd_elf): Likewise.
* os_dep.c [LINUX_STACKBOTTOM && IA64] (GC_get_register_stack_base):
Likewise.
* os_dep.c [!AMIGA && !HAIKU && !OS2 && !MSWIN32 && !MSWINCE
&& !CYGWIN32 && !GC_OPENBSD_THREADS && (!GC_SOLARIS_THREADS
|| _STRICT_STDC)] (GC_get_main_stack_base): Likewise.
* os_dep.c [DATASTART_USES_BSDGETDATASTART] (GC_FreeBSDGetDataStart):
Likewise.
@spaghettisalat

This comment has been minimized.

Copy link
Author

commented Feb 16, 2019

I've gotten around to testing it now and can confirm that it works fine. Thanks for fixing the issue.

ivmai added a commit that referenced this issue Feb 16, 2019
(a cherry-pick of commit 8892354 from 'release-7_6')

Issue #259 (bdwgc).

* include/gc.h [(PLATFORM_ANDROID || __ANDROID__)
&& IGNORE_DYNAMIC_LOADING] (_etext, __data_start, __end__, _end): Do
not declare weak symbols.
* os_dep.c [SEARCH_FOR_DATA_START && (LINUX || HURD) && PLATFORM_ANDROID]
(etext, __dso_handle): Likewise.
* include/gc.h [(PLATFORM_ANDROID || __ANDROID__)
&& IGNORE_DYNAMIC_LOADING] (GC_find_limit): Declare function as public.
* include/gc.h [(PLATFORM_ANDROID || __ANDROID__)
&& IGNORE_DYNAMIC_LOADING] (GC_INIT_CONF_ROOTS): Update comment;
do not use _etext, __data_start, __end__, _end symbols; use
GC_find_limit(__dso_handle,1) as the end of the added GC data root.
* include/private/gcconfig.h [(SPARC || ALPHA) && FREEBSD]
(GC_find_limit): Change ptr_t type to void*,
GC_bool to int.
* os_dep.c [SEARCH_FOR_DATA_START || NETBSD && __ELF__
|| NEED_FIND_LIMIT || USE_PROC_FOR_LIBRARIES] (GC_find_limit):
Likewise.
* include/private/gcconfig.h [(SPARC || ALPHA) && FREEBSD]
(DATAEND): Cast the result to ptr_t.
* os_dep.c [SEARCH_FOR_DATA_START && (LINUX || HURD)
&& !IGNORE_PROG_DATA_START && PLATFORM_ANDROID]
(GC_init_linux_data_start): Do not compare __dso_handle to _etext and
do not use __dso_handle as data start.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.