Oh, and actual failure:

signal 11 (SIGSEGV), code 128, fault addr 0x0 in tid 16588 (st:test_process)
pid: 16588, tid: 16588, name: st:test_process  >>> org.chromium.native_test:test_process <<<
signal 11 (SIGSEGV), code 128 (SI_KERNEL), fault addr 0x0

+caitkp, maybe this looks familiar (but also might be a compiler bug; this seems to pass in most places)

Hm, it passes in release x64 clang builds (I tried component only so far), but not in debug. With gcc the test passes in both release and debug.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e34cd7cecd15c2bb4290fa9cfc78f1b3f7fef03c

commit e34cd7cecd15c2bb4290fa9cfc78f1b3f7fef03c
Author: thakis <thakis@chromium.org>
Date: Wed Nov 09 23:47:29 2016

Make android/x64 bot build and run tests in release.

All other testers run tests in release as well.  This will help with
cycle time, and at least webkit_unit_tests currently fails because one
shard times out.

(Also, one base_unittest fails in debug but not in release apparently)

BUG=534815, 663886 

Review-Url: https://codereview.chromium.org/2494543002
Cr-Commit-Position: refs/heads/master@{#431084}

[modify] https://crrev.com/e34cd7cecd15c2bb4290fa9cfc78f1b3f7fef03c/tools/mb/mb_config.pyl

In release builds, ArityTest passes, but these 4 fail instead:

C   19.841s Main  [  FAILED  ] CallbackListTest.AddCallbacksDuringIteration (CRASHED)
C   19.841s Main  [  FAILED  ] CallbackListTest.BasicTest (CRASHED)
C   19.841s Main  [  FAILED  ] CallbackListTest.RemoveCallbacksDuringIteration (CRASHED)
C   19.841s Main  [  FAILED  ] CallbackListTest.EmptyList (UNKNOWN)

I debugged BasicTest for a bit. If I remove all files from base_unittests except for callback_list_unittests.cc, the test passes. If I add back callback_helpers_unittests.cc, it fails again. Here are fairly minimal contents for the second file to make the test still fail:

#include "base/bind.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace {

void Increment(int* value) {
  (*value)++;
}

TEST(CallbackHelpersTest, TestScopedClosureRunnerExitScope) {
  int run_count = 0;
  base::Bind(&Increment, &run_count);
}

}  // namespace


Increment() looks fairly similar to Listener::IncrementTotal() in the other file -- maybe related to some ICF thing?

Editing out/gnand4/obj/base/_base_unittests__library.ninja to change --icf=all to --icf=none makes the crash go away. Hm. (On Linux, we also use icf=all and the test passes.)

The second file can be just:

__attribute__((visibility("default"))) void Increment(int* value) {
  (*value)++;
}

I have the first file down to:

#include "base/bind.h"
#include "base/bind_helpers.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace base {
namespace {

class Listener {
 public:
  Listener() : total_(0) {}
  void IncrementTotal() { total_++; }

  int total_;
};

TEST(CallbackListTest, BasicTest) {
  Listener a;
  Closure c = Bind(&Listener::IncrementTotal, Unretained(&a));
  c.Run();
}


}  // namespace
}  // namespace base

Here's a fairly small two-file repro:

thakis@thakis:~/src/chrome/src$ cat base/callback_list_unittest.cc 
#include "testing/gtest/include/gtest/gtest.h"

class Listener {
 public:
  Listener() : total_(0) {}
  void IncrementTotal() { total_++; }
  int total_;
};

void(Listener::*secret(void (Listener::*memptr)()))();
Listener* secret(Listener* l);

namespace {

TEST(CallbackListTest, BasicTest) {
  Listener a;

  void (Listener::* memptr)() = &Listener::IncrementTotal;
  void (Listener::* memptr2)() = secret(memptr);
  (secret(&a)->*memptr2)();

}

}  // namespace
thakis@thakis:~/src/chrome/src$ cat base/callback_helpers_unittest.cc 
__attribute__((visibility("default"))) void Increment(int* value) {
  (*value)++;
}

class Listener {
 public:
  Listener() : total_(0) {}
  void IncrementTotal() { total_++; }
  int total_;
};

void(Listener::*secret(void (Listener::*memptr)()))() { return memptr; }
Listener* secret(Listener* l) { return l; }


I wasn't able to repro with the same two files in a stand-alone ndk-build project yet though, so I guess that is missing some other compiler flag still.

I think I'm now using the same setup as the chrome build, but still I fail to repro.

thakis@thakis:~/src/chrome/src/androidndk$ cat jni/main.cc 
#include <android/log.h>
#include <stdio.h>

class Listener {
 public:
  Listener() : total_(0) {}
  void IncrementTotal() { total_++; }
  int total_;
};

void(Listener::*secret(void (Listener::*memptr)()))();
Listener* secret(Listener* l);

extern "C"
__attribute__((visibility("default"))) 
void entry() {
  Listener a;

  void (Listener::* memptr)() = &Listener::IncrementTotal;
  void (Listener::* memptr2)() = secret(memptr);
  (secret(&a)->*memptr2)();

  printf("printf\n");
  __android_log_print(ANDROID_LOG_DEBUG, "HELLO", "message\n");
}
thakis@thakis:~/src/chrome/src/androidndk$ cat jni/other.cc 
__attribute__((visibility("default"))) void Increment(int* value) {
  (*value)++;
}

class Listener {
 public:
  Listener() : total_(0) {}
  void IncrementTotal() { total_++; }
  int total_;
};

void(Listener::*secret(void (Listener::*memptr)()))() { return memptr; }
Listener* secret(Listener* l) { return l; }
thakis@thakis:~/src/chrome/src/androidndk$ cat jni/exe.cc 
#include <dlfcn.h>
#include <stdio.h>

int main() {
  void* h = dlopen("./ndk-hello.so", RTLD_GLOBAL);
  if (!h) {
    printf("no lib\n");
    return 1;
  }

  void* f = dlsym(h, "entry");

  if (!f) {
    printf("no dice\n");
    return 1;
  }

  reinterpret_cast<void(*)()>(f)();

  dlclose(h);
}
thakis@thakis:~/src/chrome/src/androidndk$ cat build.ninja 
CC = ../third_party/llvm-build/Release+Asserts/bin/clang++ -D_FILE_OFFSET_BITS=64 -DANDROID -DHAVE_SYS_UIO_H -DANDROID_NDK_VERSION=r12b -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -D__GNU_SOURCE=1 -D__compiler_offsetof=__builtin_offsetof -Dnan=__builtin_nan -Dsnprintf=snprintf -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -g -funwind-tables -fPIC -pipe -fcolor-diagnostics -ffunction-sections -fno-short-enums --target=x86_64-linux-androideabi -m64 -march=x86-64 -Wall -Werror -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-shift-negative-value -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-address-of-packed-member -Os -fno-ident -fdata-sections -ffunction-sections -fomit-frame-pointer -gdwarf-3 -g1 --sysroot=../third_party/android_tools/ndk/platforms/android-21/arch-x86_64 -fvisibility=hidden -Wheader-hygiene -Wstring-conversion -fno-threadsafe-statics -fvisibility-inlines-hidden -std=gnu++11 -fno-rtti -isystem../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++/libcxx/include -isystem../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/include -isystem../third_party/android_tools/ndk/sources/android/support/include -fno-exceptions

LD = ../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,--fatal-warnings -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -Wl,-z,defs -Wl,--as-needed --gcc-toolchain=../third_party/android_tools/ndk/toolchains/x86_64-4.9/prebuilt/linux-x86_64 -fuse-ld=gold -Wl,--icf=all -Wl,--build-id=sha1 -Wl,--no-undefined -Wl,--exclude-libs=libgcc.a -Wl,--exclude-libs=libc++_static.a -Wl,--exclude-libs=libvpx_assembly_arm.a --target=x86_64-linux-androideabi -m64 -Wl,--warn-shared-textrel -Wl,-O1 -Wl,--gc-sections -nostdlib -Wl,--warn-shared-textrel --sysroot=../third_party/android_tools/ndk/platforms/android-21/arch-x86_64  -Wl,--version-script=/usr/local/google/home/thakis/src/chrome/src/build/android/android_no_jni_exports.lst -Wl,-wrap,calloc -Wl,-wrap,free -Wl,-wrap,malloc -Wl,-wrap,memalign -Wl,-wrap,posix_memalign -Wl,-wrap,pvalloc -Wl,-wrap,realloc -Wl,-wrap,valloc -L../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++/libs/x86_64 ../third_party/android_tools/ndk/platforms/android-21/arch-x86_64/usr/lib64/crtbegin_so.o -lc++_static -lc++abi -landroid_support ../third_party/android_tools/ndk/toolchains/x86_64-4.9/prebuilt/linux-x86_64/lib/gcc/x86_64-linux-android/4.9.x/libgcc.a -lc -ldl -lm -llog ../third_party/android_tools/ndk/platforms/android-21/arch-x86_64/usr/lib64/crtend_so.o

LDEXE = ../third_party/llvm-build/Release+Asserts/bin/clang++ -Wl,--fatal-warnings -fPIE -pie -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -Wl,-z,defs -Wl,--as-needed --gcc-toolchain=../third_party/android_tools/ndk/toolchains/x86_64-4.9/prebuilt/linux-x86_64 -fuse-ld=gold -Wl,--icf=all -Wl,--build-id=sha1 -Wl,--no-undefined --target=x86_64-linux-androideabi -m64 -Wl,--warn-shared-textrel -Wl,-O1 -Wl,--gc-sections -Wl,--warn-shared-textrel --sysroot=../third_party/android_tools/ndk/platforms/android-21/arch-x86_64  -Wl,--version-script=/usr/local/google/home/thakis/src/chrome/src/build/android/android_no_jni_exports.lst -Wl,-wrap,calloc -Wl,-wrap,free -Wl,-wrap,malloc -Wl,-wrap,memalign -Wl,-wrap,posix_memalign -Wl,-wrap,pvalloc -Wl,-wrap,realloc -Wl,-wrap,valloc -L../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++/libs/x86_64 -lc++_static -lc++abi -landroid_support ../third_party/android_tools/ndk/toolchains/x86_64-4.9/prebuilt/linux-x86_64/lib/gcc/x86_64-linux-android/4.9.x/libgcc.a -lc -ldl -lm -llog 

rule cxx
  command = $CC -MMD -MF $out.d -o $out -c $in
  description = CC $out
  depfile = $out.d
  deps = gcc

rule ld
  command = $LD -o $out $in
  description = LD $out

rule ldexe
  command = $LDEXE -o $out $in
  description = LD $out

build main.o: cxx jni/main.cc
build other.o: cxx jni/other.cc
build ndk-hello.so: ld main.o other.o

build exe.o: cxx jni/exe.cc
build ndk-hello: ldexe exe.o | ndk-hello.so

In the original repro, if I flip the order of callback_helpers_unittest.cc and callback_list_unittest.cc in base/BUILD.gn, it starts passing with clang as well (I figured .o order on link link might influence which way round the CFI happens; looks like that's true)

Here's how my repro currently looks btw: https://codereview.chromium.org/2513363003 

(-caitkp btw; seems innocent ;-))

From `objdump -d out/gnand4/lib_base_unittests__library.so`:

// The test function
   1c0a4:	41 57                	push   %r15
   1c0a6:	41 56                	push   %r14
   1c0a8:	53                   	push   %rbx
   1c0a9:	48 83 ec 10          	sub    $0x10,%rsp
   1c0ad:	4c 8d 74 24 08       	lea    0x8(%rsp),%r14
   1c0b2:	41 c7 06 00 00 00 00 	movl   $0x0,(%r14)
   1c0b9:	48 8d 3d 95 ff ff ff 	lea    -0x6b(%rip),%rdi        # 1c055 <_Z9IncrementPi>
   1c0c0:	31 f6                	xor    %esi,%esi
   1c0c2:	e8 91 ff ff ff       	callq  1c058 <_Z9IncrementPi+0x3>
   1c0c7:	48 89 c3             	mov    %rax,%rbx  <- rbx has memptr
   1c0ca:	49 89 d7             	mov    %rdx,%r15  <- has 0
   1c0cd:	4c 89 f7             	mov    %r14,%rdi  <- r14 points to int/object on stack
   1c0d0:	e8 8a ff ff ff       	callq  1c05f <_Z9IncrementPi+0xa>  (rax now &listener)
   1c0d5:	4c 01 f8             	add    %r15,%rax  <- apply this adjustment
   1c0d8:	f6 c3 01             	test   $0x1,%bl   <- check if memptr is 2-aligned (??)
+--1c0db:	74 08                	je     1c0e5 <_ZNSt13bad_exceptionD0Ev+0x7f>  <- low bit set?
|  1c0dd:	48 8b 08             	mov    (%rax),%rcx             <- deref rax (total_/0) to rcx,
|  1c0e0:	48 8b 5c 19 ff       	mov    -0x1(%rcx,%rbx,1),%rbx  <- rbx = *(rcx + 1*rbx - 1)
|                                                                             = *(rbx - 1)
|                                                                         i.e. if ptr odd, subtract 1
+->1c0e5:	48 89 c7             	mov    %rax,%rdi
   1c0e8:	ff d3                	callq  *%rbx
   1c0ea:	48 83 c4 10          	add    $0x10,%rsp
   1c0ee:	5b                   	pop    %rbx
   1c0ef:	41 5e                	pop    %r14
   1c0f1:	41 5f                	pop    %r15
   1c0f3:	c3                   	retq   


This code for my standalone files is the same.  The alignment check for the memptr looks a bit suspect I guess? In the full example, Increment() starts on an odd address.

If I order other.cc before main.cc in my local example, then the only place where Increment starts at an odd address is if I build for linux64, not android. But it looks like that segfaults too! So maybe that's a workable repro.

Complete repro:

thakis@thakis:~/src/chrome/src/androidndk$ cat build.ninja
rule cxxl
  command = ../third_party/llvm-build/Release+Asserts/bin/clang++ -fPIC -fcolor-diagnostics -m64 -Os -ffunction-sections -o $out -c $in
  description = CC $out

rule ldl
  command = ../third_party/llvm-build/Release+Asserts/bin/clang++ -fuse-ld=gold -Wl,--icf=all -m64 $ldflags -o $out $in
  description = LD $out

build mainl.o: cxxl jni/main.cc
build kotherl.o: cxxl jni/kother.cc
build ndk-hellol.so: ldl kotherl.o mainl.o
  ldflags = -shared

build exel.o: cxxl jni/exe.cc
build ndk-hellol: ldl exel.o | ndk-hellol.so
  ldflags = -ldl
thakis@thakis:~/src/chrome/src/androidndk$ cat jni/main.cc
class Listener {
 public:
  Listener() : total_(0) {}
  void IncrementTotal() { total_++; }
  int total_;
};

void(Listener::*secret(void (Listener::*memptr)()))();
Listener* secret(Listener* l);

extern "C"
__attribute__((visibility("default"))) 
void entry() {
  Listener a;

  void (Listener::* memptr)() = &Listener::IncrementTotal;
  void (Listener::* memptr2)() = secret(memptr);
  (secret(&a)->*memptr2)();
}
thakis@thakis:~/src/chrome/src/androidndk$ cat jni/kother.cc 
__attribute__((visibility("default"))) void Increment(int* value) {
  (*value)++;
}

class Listener {
 public:
  Listener() : total_(0) {}
  void IncrementTotal() { total_++; }
  int total_;
};

void(Listener::*secret(void (Listener::*memptr)()))() { return memptr; }
Listener* secret(Listener* l) { return l; }
thakis@thakis:~/src/chrome/src/androidndk$ cat jni/exe.cc 
#include <dlfcn.h>
#include <stdio.h>

int main() {
  void* h = dlopen("./ndk-hellol.so", RTLD_NOW | RTLD_GLOBAL);
  if (!h) {
    printf("no lib %s\n", dlerror());
    return 1;
  }

  void* f = dlsym(h, "entry");

  if (!f) {
    printf("no dice\n");
    return 1;
  }

  reinterpret_cast<void(*)()>(f)();

  dlclose(h);
}
thakis@thakis:~/src/chrome/src/androidndk$ ninja ndk-hellol && ./ndk-hellol
ninja: no work to do.
Segmentation fault



$ objdump -d ndk-hellol.so | grep Increment -A 4
0000000000000815 <_Z9IncrementPi>:
 815:	ff 07                	incl   (%rdi)
 817:	c3                   	retq   



Doesn't happen if that function happens to start at an even address, e.g. when swapping the order of kotherl.o and mainl.o (which swaps them on the link line)

https://mentorembedded.github.io/cxx-abi/abi.html#member-pointers

"For a non-virtual function, this field is a simple function pointer. (Under current base Itanium psABI conventions, that is a pointer to a GP/function address pair.) For a virtual function, it is 1 plus the virtual table offset (in bytes) of the function, represented as a ptrdiff_t. The value zero represents a NULL pointer, independent of the adjustment field value below."

I suppose the check for 1 tests if this is a virtual function or not. clang probably takes care to align member functions with alignment of at least 2, but doesn't do the same for bare functions, and when gold ICFs them together it doesn't pay attention to this either.  So if we're unlucky, gold's ICF lets the "runtime" believe that a regular member function pointer is actually a pointer to a virtual member.

So why do we only hit this on Android? Because we build with -Os there, and with -O2/-O0 we apparently always align function entry points at 16 bytes.

(rnk/hans: just cc'ing you 'cause it's an interesting bug)

Turns out this also repros when using gcc instead of clang in my small repro.

(Why doesn't it repro in full android builds with gcc? Apparently we don't icf=all there: https://cs.chromium.org/chromium/src/build/config/compiler/BUILD.gn?q=icf%3Dall&sq=package:chromium&dr=C&l=375)

Looks like this is https://sourceware.org/bugzilla/show_bug.cgi?id=17704 . A fix for this was landed to gold exactly a month ago today. Maybe we can our bundled gold with this fix?

See also  issue 576197 

The fix for this ICF issue was backported into both Android and ChromiumOS binutils earlier this month:
-) https://android-review.googlesource.com/#/c/292548/
-) https://chromium-review.googlesource.com/#/c/410440/

The change is live in ChromiumOS development chroots, but Android still needs updating the toolchain prebuilts.

https://codereview.chromium.org/2524503002/

(I figured I'd dupe the rest of this into  bug 576197  once that cl lands)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aaa006d4a8d979e96d2ac248306240a60590b0bc

commit aaa006d4a8d979e96d2ac248306240a60590b0bc
Author: thakis <thakis@chromium.org>
Date: Tue Nov 22 05:55:22 2016

chrome/android/intel/clang: Use --icf=safe instead of --icf=all.

gold has a bug where it ignores section alignment when merging functions.
This means that member functions and regular functions with the same
contents can be merged even if the regular function has an odd address.
In the Itanium ABI, member function pointers with an odd address denote
virtual member function points, so if a non-virtual member function
gets folded into an identical non-member function that happens to be
at an odd address, the generated code will conclude at runtime that
the call is meant to be virtual, leading to a crash.

This only happens with -Os, at -O2 and -O0 functions are always
aligned at 16-byte boundaries.  This only happens with clang because
we currently never pass --icf to gold in android builds with gcc.

The gold bug is fixed upstream and is in the progress of being merged
into the NDK's gold, but it's not there yet.  Until it is, disable
full ICF on android (where we use -Os).

BUG= 663886 

Review-Url: https://codereview.chromium.org/2524503002
Cr-Commit-Position: refs/heads/master@{#433797}

[modify] https://crrev.com/aaa006d4a8d979e96d2ac248306240a60590b0bc/build/config/compiler/BUILD.gn

Note that the icf level (all vs. safe) is not directly related to the section alignment bug.

The workaround in #24 is probably only working because it disables icf on the functions that the section alignment bug was hitting. In fact, if you're building PIE binaries, --icf=safe is almost equivalent to --icf=none (https://bugs.chromium.org/p/chromium/issues/detail?id=641042).

This would explain why changing the icf level avoids the bug, as well as the increase in the binary size. It's unfortunate that the workaround negates the effect of -Os, which is what triggered the bug to begin with.

 bug 576197  tracks merging the ICF fix into our gold