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

Segmentation fault with clang-x86 #635

Closed
vok1980 opened this issue Feb 8, 2018 · 5 comments
Closed

Segmentation fault with clang-x86 #635

vok1980 opened this issue Feb 8, 2018 · 5 comments
Assignees

Comments

@vok1980
Copy link

vok1980 commented Feb 8, 2018

Description

Found code with static initialization that causes segmentation fault when build with clang for x86.
clang-arm works well.

class Handler;
class Boo
{
public:
	Boo& foo(bool (Handler::*handler)()) { return *this; }
	void (Handler::*a)() = nullptr;
	bool (Handler::*b)() = nullptr;
};

static const auto boo01 = Boo().foo(nullptr);

Testcase with buildscript can be found here:
https://github.com/vok1980/segfault-android-clang-x86

Environment Details

  • NDK Version: 16.1.4479499
  • Build system: custom build script
  • Host OS: Ubuntu 16.04.3
  • Compiler: clang
  • ABI: x86
  • STL: ---
  • NDK API level: 19
  • Device API level: 23
@rprichard rprichard self-assigned this Feb 8, 2018
@rprichard
Copy link
Collaborator

rprichard commented Feb 8, 2018

When the C++ initializer runs, the stack is aligned to 4 mod 16. There's an SSE movaps instruction that accesses a misaligned stack location, which causes the segfault. The stack should be 16-byte aligned in the Linux x86 ABI.

There's a platform bug here that seems to exist in (at least) 19 through 23. API 25 seems to be OK, or maybe I'm getting lucky.

Reduced test case:

#include <stdio.h>

struct Aligned16 {
  char buf[16];
} __attribute__((aligned(16)));

static bool init() {
  Aligned16 a16;
  printf("&a16 = %p\n", &a16);
  return true;
}

static bool dummy = init();

int main() {
  return 0;
}

Broken output (23):

&a16 = 0xbfc459b4

Good output (25):

&a16 = 0xbfbe88a0

See https://issuetracker.google.com/37116296 and https://issuetracker.google.com/37119345.

Our fix for this issue is to compile with -mstackrealign, which directs the compiler to align the stack in the prologues of functions. (The realignment forces call frame setup and then uses a single instruction, e.g. andl $-16, %esp.) We already pass this flag in the NDK's CMake toolchain file and via the ndk-build system.

The standalone toolchain doesn't pass the flag. Presumably it also should?

Aside: I'm looking at the output of -mstackrealign on godbolt, and the flag seems to mean different things between Clang and GCC.

  • Clang: assume SP isn't aligned at-all, not even to 4 bytes. The flag also applies to x86_64 and ARM32.
  • GCC: assume SP is aligned to only 8 bytes. The flag is rejected on ARM32.

Edit: Remove "The flag is ignored for x86_64". It looks like the flag does affect x86_64.

@vok1980
Copy link
Author

vok1980 commented Feb 9, 2018

Thanks, -mstackrealign helps.

@rprichard
Copy link
Collaborator

The standalone toolchain targeting 32-bit x86 passes -mstackrealign now:

miodragdinic pushed a commit to MIPS/ndk that referenced this issue Apr 17, 2018
Test: ./checkbuild.py && ./run_tests.py
Bug: android/ndk#635
Change-Id: I68e724453baf5b07a4e28ca5078e8fcb0c5ddcc3
(cherry picked from commit c87bf19428313bc21c3e0aeee4f773ad4e523129)
@pirama-arumuga-nainar
Copy link
Collaborator

@rprichard Ryan, a clarification. You mention:

There's a platform bug here that seems to exist in (at least) 19 through 23. API 25 seems to be OK, or maybe I'm getting lucky.

Is the platform bug that the stack is sometimes not aligned to 16 bytes or is it something different?

@rprichard
Copy link
Collaborator

rprichard commented May 21, 2018

Is the platform bug that the stack is sometimes not aligned to 16 bytes or is it something different?

Yes, the bug here was that ESP was misaligned on dynamic linker startup, and also misaligned when the dynamic linker called constructors while loading the executable. I had commented that the bug seemed fixed as of API 25. AFAIK, the NDK could omit -mstackrealign when targeting API >= 25.

I fixed the root cause of this misalignment for P (API 28) in https://android-review.googlesource.com/c/platform/bionic/+/615665/. I believe the issue was fixed in previous platform versions by two changes:

  • First, we built everything in the platform with -mstackrealign.
  • Later, we switching from GCC to Clang. Clang's -mstackrealign emits realignment code into every function, not just those that require a 16-byte-aligned stack.

The combination of those changes masked the misalignment fixed by my Gerrit CL above.


Edit: It looks like API 24 is also fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants