[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 --- Comment #17 from Elliott Hughes --- (/me checks check-in comment from https://android-review.googlesource.com/393156...) yeah, looks like i misremembered: it was SSE4 that was missing. from the bug that links to, it was specifically these instructions: for (i = 0; i < len; ++i) any_set |= buf[i]; d2c70: 66 0f 38 21 51 fc pmovsxbd -0x4(%ecx),%xmm2 d2c76: 66 0f 38 21 19 pmovsxbd (%ecx),%xmm3 clang has a habit of choosing more exotic instructions that gcc for arm32 too. (that's what's changed recently: the compiler, not the compiler flags.) -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 --- Comment #16 from Julian Seward --- (In reply to Elliott Hughes from comment #15) > x86 is useless because valgrind doesn't > support SSE2 for x86 so you can't get as far as calling main, Valgrind supports up to and including SSSE3 on x86 (32 bit). -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529
--- Comment #15 from Elliott Hughes ---
fwiw, here's the current diff to be able to build valgrind 3.13.0 for Android
(arm,aarch64,x86-64). (x86 is useless because valgrind doesn't support SSE2 for
x86 so you can't get as far as calling main, but these are the only fixes you
need to support x86-64, so you might want to update your list of supported
platforms in the release notes.)
https://android-review.googlesource.com/420323/
Only in /huge-ssd/aosp-arm64/external/valgrind/: android
Only in /huge-ssd/aosp-arm64/external/valgrind/: Android.build_all.mk
Only in /huge-ssd/aosp-arm64/external/valgrind/: Android.build_host.mk
Only in /huge-ssd/aosp-arm64/external/valgrind/: Android.build_one.mk
Only in /huge-ssd/aosp-arm64/external/valgrind/: Android.clean.mk
Only in /huge-ssd/aosp-arm64/external/valgrind/: Android.mk
Only in /huge-ssd/aosp-arm64/external/valgrind/:
ANDROID_PATCH_AGAINST_UPSTREAM.txt
Only in /huge-ssd/aosp-arm64/external/valgrind/: Android.test.mk
diff '--exclude=.git' -ru valgrind-3.13.0/config.h
/huge-ssd/aosp-arm64/external/valgrind/config.h
--- valgrind-3.13.0/config.h2017-06-21 14:11:07.177545261 -0700
+++ /huge-ssd/aosp-arm64/external/valgrind/config.h 2017-06-21
14:07:44.786099941 -0700
@@ -45,10 +45,14 @@
/* Define to 1 if index() and strlen() have been optimized heavily (x86 glibc
>= 2.12) */
+#ifndef __ANDROID__
#define GLIBC_MANDATORY_INDEX_AND_STRLEN_REDIRECT 1
+#endif
/* Define to 1 if strlen() has been optimized heavily (amd64 glibc >= 2.10) */
+#ifndef __ANDROID__
#define GLIBC_MANDATORY_STRLEN_REDIRECT 1
+#endif
/* Define to 1 if you have the header file. */
#define HAVE_ASM_UNISTD_H 1
@@ -86,13 +90,15 @@
#define HAVE_CLOCK_MONOTONIC 1
/* Define to 1 if you have a dlinfo that can do RTLD_DI_TLS_MODID. */
+#ifndef __ANDROID__
#define HAVE_DLINFO_RTLD_DI_TLS_MODID 1
+#endif
/* Define to 1 if the system has the type `Elf32_Chdr'. */
-/* #undef HAVE_ELF32_CHDR */
+//#define HAVE_ELF32_CHDR 1
/* Define to 1 if the system has the type `Elf64_Chdr'. */
-/* #undef HAVE_ELF64_CHDR */
+//#define HAVE_ELF64_CHDR 1
/* Define to 1 if you have the header file. */
#define HAVE_ENDIAN_H 1
@@ -170,7 +176,9 @@
/* #undef HAVE_PTHREAD_CREATE_GLIBC_2_0 */
/* Define to 1 if you have the `PTHREAD_MUTEX_ADAPTIVE_NP' constant. */
+#ifndef __ANDROID__
#define HAVE_PTHREAD_MUTEX_ADAPTIVE_NP 1
+#endif
/* Define to 1 if you have the `PTHREAD_MUTEX_ERRORCHECK_NP' constant. */
#define HAVE_PTHREAD_MUTEX_ERRORCHECK_NP 1
@@ -182,7 +190,9 @@
#define HAVE_PTHREAD_MUTEX_TIMEDLOCK 1
/* Define to 1 if pthread_mutex_t has a member __data.__kind. */
+#ifndef __ANDROID__
#define HAVE_PTHREAD_MUTEX_T__DATA__KIND 1
+#endif
/* Define to 1 if pthread_mutex_t has a member called __m_kind. */
/* #undef HAVE_PTHREAD_MUTEX_T__M_KIND */
@@ -219,7 +229,9 @@
#define HAVE_SEMTIMEDOP 1
/* Define to 1 if libstd++ supports annotating shared pointers */
+#ifndef __ANDROID__
#define HAVE_SHARED_POINTER_ANNOTATION 1
+#endif
/* Define to 1 if you have the `signalfd' function. */
#define HAVE_SIGNALFD 1
@@ -456,7 +468,11 @@
#define VERSION "3.13.0"
/* Temporary files directory */
+#ifdef __ANDROID__
+#define VG_TMPDIR "/data/local/tmp"
+#else
#define VG_TMPDIR "/tmp"
+#endif
/* Define to `int' if doesn't define. */
/* #undef gid_t */
diff '--exclude=.git' -ru valgrind-3.13.0/coregrind/m_coredump/coredump-elf.c
/huge-ssd/aosp-arm64/external/valgrind/coregrind/m_coredump/coredump-elf.c
--- valgrind-3.13.0/coregrind/m_coredump/coredump-elf.c 2017-05-31
08:14:48.0 -0700
+++ /huge-ssd/aosp-arm64/external/valgrind/coregrind/m_coredump/coredump-elf.c
2017-06-21 14:08:45.497933443 -0700
@@ -135,6 +135,7 @@
phdr->p_align = VKI_PAGE_SIZE;
}
+#if 0 /* We've had Elf32_Nhdr since at least froyo! */
#if defined(VGPV_arm_linux_android) || defined(VGPV_x86_linux_android) \
|| defined(VGPV_mips32_linux_android)
/* Android's libc doesn't provide a definition for this. Hence: */
@@ -146,6 +147,7 @@
}
Elf32_Nhdr;
#endif
+#endif
struct note {
struct note *next;
diff '--exclude=.git' -ru valgrind-3.13.0/coregrind/vgdb.c
/huge-ssd/aosp-arm64/external/valgrind/coregrind/vgdb.c
--- valgrind-3.13.0/coregrind/vgdb.c2017-05-31 08:14:29.0 -0700
+++ /huge-ssd/aosp-arm64/external/valgrind/coregrind/vgdb.c 2017-06-21
14:17:48.668450889 -0700
@@ -682,10 +682,7 @@
sigpipe++;
} else if (signum == SIGALRM) {
sigalrm++;
-#if defined(VGPV_arm_linux_android) \
-|| defined(VGPV_x86_linux_android) \
-|| defined(VGPV_mips32_linux_android) \
-|| defined(VGPV_arm64_linux_android)
+#if defined(__BIONIC__)
/* Android has no pthread_cancel. As it also does not have
an invoker implementation, there is no need for cleanup action.
So, we just do nothing. */
diff '--exclude=.git' -ru valgrind-3.13.0/coregrind/vg_preloaded.c
/huge-ssd/aosp-arm64/external/valgrind/coregri
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 --- Comment #14 from Elliott Hughes --- > Redone, using the house conditionalisation scheme, in r16386. Please test. (sorry, didn't have time to look until now.) as i explained already, that's wrong. anywhere you have this idiom in the code and it doesn't include *all* the Android variants -- which right now is at least 30 places, almost everywhere you have this idiom -- it's wrong. why not just say __ANDROID__ for OS-details (like /system/bin/sh versus /bin/sh) or __BIONIC__ if it's a libc-specific thing? you already make use of __GLIBC__ and __UCLIBC__ and [a home-grown] MUSL_LIBC, but won't use __BIONIC__? that doesn't make any sense. (see comment 8 on this bug or https://bugs.kde.org/show_bug.cgi?id=379878 if you want more examples of bugs caused by this idiom.) -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 --- Comment #13 from Julian Seward --- Redone, using the house conditionalisation scheme, in r16386. Please test. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 Julian Seward changed: What|Removed |Added CC||[email protected] --- Comment #12 from Julian Seward --- (In reply to Elliott Hughes from comment #8) > (In reply to Ivo Raisr from comment #7) > > I am a little worried about this condition in the patch: > > > > #if defined(ANDROID) && defined(__clang__) > > > > Nowhere in Valgrind sources we currently base a decision on naked "ANDROID". > > It is always this combo (from vki-linux.h): > > > > #if defined(VGPV_arm_linux_android) || defined(VGPV_x86_linux_android) \ > > || defined(VGPV_mips32_linux_android) \ > > || defined(VGPV_arm64_linux_android) > > ... > > #endif /* defined(VGPV_*_linux_android) */ > > that idiom is kind of insane in most cases. it also leads to bugs like > https://bugs.kde.org/show_bug.cgi?id=339945 or > https://bugs.kde.org/show_bug.cgi?id=379764 where you have to manually > maintain these lists. No, I object. The platform conditionalisation is like it is for a good reason, which is that ad-hoc ifdeffery, which this __ANDROID__ define is, eventually leads to a big mess which is hard to modify and reason about. We have to keep the system maintainable on a bunch of targets, not just Android. Please use the existing system where you can. We want to continue to support Android well, but that can't be at the expense of getting into a confusing maze of ifdefs. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 Ivo Raisr changed: What|Removed |Added Resolution|--- |FIXED Status|ASSIGNED|RESOLVED --- Comment #11 from Ivo Raisr --- Committed in Valgrind SVN as r16384. Thank you for the patch. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 --- Comment #10 from Elliott Hughes --- yeah, looking through the current clusters of #if..._android_linux, they're all either wrong or the subject of an outstanding patch :-/ i've raised https://bugs.kde.org/show_bug.cgi?id=379878 for that more general issue, to avoid derailing this bug. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 --- Comment #9 from Elliott Hughes --- Created attachment 105569 --> https://bugs.kde.org/attachment.cgi?id=105569&action=edit patch for https://bugs.kde.org/show_bug.cgi?id=368529#c8 -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 --- Comment #8 from Elliott Hughes --- (In reply to Ivo Raisr from comment #7) > I am a little worried about this condition in the patch: > > #if defined(ANDROID) && defined(__clang__) > > Nowhere in Valgrind sources we currently base a decision on naked "ANDROID". > It is always this combo (from vki-linux.h): > > #if defined(VGPV_arm_linux_android) || defined(VGPV_x86_linux_android) \ > || defined(VGPV_mips32_linux_android) \ > || defined(VGPV_arm64_linux_android) > ... > #endif /* defined(VGPV_*_linux_android) */ that idiom is kind of insane in most cases. it also leads to bugs like https://bugs.kde.org/show_bug.cgi?id=339945 or https://bugs.kde.org/show_bug.cgi?id=379764 where you have to manually maintain these lists. or other bugs still to be discovered: # if defined(VGPV_arm_linux_android) \ || defined(VGPV_x86_linux_android) \ || defined(VGPV_mips32_linux_android) \ || defined(VGPV_arm64_linux_android) const HChar* default_interp_name = "/system/bin/sh"; # else const HChar* default_interp_name = "/bin/sh"; # endif is missing amd64, for example. just using __ANDROID__ would have avoided this. ah, and looking at more of these i think i'm finding the causes of other outstanding valgrind bugs on x86-64 Android... i think you generally want to use __BIONIC__ or __ANDROID__ instead, in the same way you use __GLIBC__ rather than individually listing each possible glibc/arch combination. that said, getting back to this bug... > If this patch is really arm-android specific, ...yes, this patch is arm-specific, but it's already inside #if defined(VGP_arm_linux). > then it should fold inside > existing > #if defined(VGP_arm_linux) [at line 2424] > ... > #endif [at line 2442] > > with a guard such as: > > #if defined(VGPV_arm_linux_android) > ... > #endif > > No need to "defined(__clang__)". > > > Let me know what is the case here. > Please eventually modify the patch and test it on arm. i'll attach the smallest patch that works. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 Ivo Raisr changed: What|Removed |Added Status|CONFIRMED |ASSIGNED --- Comment #7 from Ivo Raisr --- I am a little worried about this condition in the patch: #if defined(ANDROID) && defined(__clang__) Nowhere in Valgrind sources we currently base a decision on naked "ANDROID". It is always this combo (from vki-linux.h): #if defined(VGPV_arm_linux_android) || defined(VGPV_x86_linux_android) \ || defined(VGPV_mips32_linux_android) \ || defined(VGPV_arm64_linux_android) ... #endif /* defined(VGPV_*_linux_android) */ If this patch is really arm-android specific, then it should fold inside existing #if defined(VGP_arm_linux) [at line 2424] ... #endif [at line 2442] with a guard such as: #if defined(VGPV_arm_linux_android) ... #endif No need to "defined(__clang__)". Let me know what is the case here. Please eventually modify the patch and test it on arm. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 Ivo Raisr changed: What|Removed |Added Assignee|[email protected] |[email protected] CC||[email protected] Status|UNCONFIRMED |CONFIRMED Ever confirmed|0 |1 --- Comment #6 from Ivo Raisr --- I will have a look. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 --- Comment #5 from Elliott Hughes --- this patch is still necessary with 3.12 (we updated AOSP recently). it's one of only four issues building valgrind 3.12 out of the box for Android. worth taking for 3.13? -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 Elliott Hughes changed: What|Removed |Added CC||[email protected] -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 --- Comment #4 from [email protected] --- Created attachment 105101 --> https://bugs.kde.org/attachment.cgi?id=105101&action=edit diff to m_main.c This is from https://android-review.googlesource.com/#/c/272640/ -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 Mark Wielaard changed: What|Removed |Added CC||[email protected] --- Comment #3 from Mark Wielaard --- (In reply to chh from comment #2) > Julian, this was fixed for Android in > https://android-review.git.corp.google.com/#/c/272640 > Could you review and take that patch? Please always attach proposed patches to bugzilla. That makes it much easier to review and apply them. Pointing to some external project fork repository/review system makes it hard to know what is intended for the upstream project and what is specific to the downstream project. Also the above URL seems to be to some internal google project. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 --- Comment #2 from [email protected] --- Julian, this was fixed for Android in https://android-review.git.corp.google.com/#/c/272640 Could you review and take that patch? Thanks. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 --- Comment #1 from [email protected] --- The undefined symbols were due to the use of builtin functions like __aeabi_memcpy* and __aeabi_memclr* from clang/llvm compiler. Those symbols pulled in other modules in Android libc.a and then require atexit and pthread_atfork, which are not defined in libc.a. A better solution is to define those __aeabi_* functions in m_main.c, as it already defines memcpy to VG_(memcpy) and memset to VG_(memset). The following is the suggested diff. diff --git a/coregrind/m_main.c b/coregrind/m_main.c index 140efbf..66f04df 100644 --- a/coregrind/m_main.c +++ b/coregrind/m_main.c @@ -2883,6 +2883,46 @@ void __aeabi_unwind_cpp_pr1(void){ VG_(printf)("Something called __aeabi_unwind_cpp_pr1()\n"); vg_assert(0); } + +#if defined(ANDROID) && defined(__clang__) +/* Replace __aeabi_memcpy* functions with vgPlain_memcpy. */ +void* __aeabi_memcpy(void *dest, const void *src, SizeT n); +void* __aeabi_memcpy(void *dest, const void *src, SizeT n) +{ +return VG_(memcpy)(dest, src, n); +} + +void* __aeabi_memcpy4(void *dest, const void *src, SizeT n); +void* __aeabi_memcpy4(void *dest, const void *src, SizeT n) +{ +return VG_(memcpy)(dest, src, n); +} + +void* __aeabi_memcpy8(void *dest, const void *src, SizeT n); +void* __aeabi_memcpy8(void *dest, const void *src, SizeT n) +{ +return VG_(memcpy)(dest, src, n); +} + +/* Replace __aeabi_memclr* functions with vgPlain_memset. */ +void* __aeabi_memclr(void *dest, SizeT n); +void* __aeabi_memclr(void *dest, SizeT n) +{ +return VG_(memset)(dest, 0, n); +} + +void* __aeabi_memclr4(void *dest, SizeT n); +void* __aeabi_memclr4(void *dest, SizeT n) +{ +return VG_(memset)(dest, 0, n); +} + +void* __aeabi_memclr8(void *dest, SizeT n); +void* __aeabi_memclr8(void *dest, SizeT n) +{ +return VG_(memset)(dest, 0, n); +} +#endif /* ANDROID __clang__ */ #endif /* Requirement 2 */ -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 368529] Android arm target link error, missing atexit and pthread_atfork
https://bugs.kde.org/show_bug.cgi?id=368529 [email protected] changed: What|Removed |Added CC||[email protected] -- You are receiving this mail because: You are watching all bug changes.
