Bug ID: JDK-8186780 clang fastdebug assertion failure in os_linux_x86:os::verify_stack

JDK 11	JDK 15
11.0.15-oracleFixed	15 b18Fixed

Fix Request (11u). Applies cleanly net of context.
08-12-2021
URL: https://hg.openjdk.java.net/jdk/jdk/rev/7abfcec00e7d User: martin Date: 2020-03-31 21:20:58 +0000
31-03-2020
Magnus, thanks for confirming!
17-03-2020
[~jiangli] We do not support building with anything older than gcc 5.0.
17-03-2020
gcc-4.1.0-k8 is a very old version (>=13 year old). Hopefully no one is using it today. Martin also pointed out that __builtin_frame_address(0) is already being used unconditionally elsewhere in hotspot, for example in os_linux_zero.cpp. So looks like it can be used safely for gcc as well.
17-03-2020
Martin, I think the change looks good in general (and your webrev should be sent to hotspot-runtime-dev@openjdk.java.net for review). My only concern is enabling it for non-Clang case. According to https://github.com/google/glog/blob/master/src/stacktrace_x86-inl.h#L113, __builtin_frame_address(0) can return the wrong address on gcc-4.1.0-k8.
17-03-2020
Time to make another attempt to fix this. The proposed fix is: --- a/src/hotspot/os_cpu/linux_x86/os_linux_x86.cpp +++ b/src/hotspot/os_cpu/linux_x86/os_linux_x86.cpp @@ -98,13 +98,8 @@ void esp; __asm__("mov %%" SPELL_REG_SP ", %0":"=r"(esp)); return (address) ((char)esp + sizeof(long)2); -#elif defined(__clang__) - void esp; - __asm__ __volatile__ ("mov %%" SPELL_REG_SP ", %0":"=r"(esp):); - return (address) esp; #else - register void *esp __asm__ (SPELL_REG_SP); - return (address) esp; + return (address)__builtin_frame_address(0); #endif } The idiom return (address)__builtin_frame_address(0); is already in use to implement os::current_stack_pointer in os_linux_aarch64.cpp
09-03-2020
Then I'm moving this to hotspot/runtime, which I believe is the closest match. If they are not happy, they are free to pass it on to a more suitable component.
31-01-2020
I haven't been able to find any engineer (or even a component!) in hotspot land to own this. How many decades will I need to wait before I can use C++11 in hotspot sources?
30-01-2020
[~martin] The way the discussion went, I think this sounds (once again) more like a hotspot problem than a build problem. Tweaking os::verify_stack_alignment() is very much out of scope for the build team. Can you please find a better component to file the bug under? If it turns out that build changes are needed eventually, we'll of course be happy to assist.
30-01-2020
I recently was dealing with stack alignment issues in the bsd-port of 11u which uses clang 8 to build on OpenBSD and FreeBSD. The most straightforward way to enable os::verify_stack_alignment() for clang is to disable optimizations on os::current_stack_pointer() which does the following: - prevents inlining of os::current_stack_pointer() into os::verify_stack_alignment() which then is optimized and the current stack pointer alignment is not maintained – as noted in comment: https://bugs.openjdk.java.net/browse/JDK-8186780?focusedCommentId=14194930&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14194930 - on x86 (32bit) os::current_stack_pointer() optimization also looses the current stack pointer alignment so optnone prevents this issue as well. See the following for diffs: https://github.com/battleblow/openjdk-jdk11u/pull/49/files
24-07-2019
https://openjdk.markmail.org/thread/yvkbgli746dekcrt
22-03-2019
Unfortunately, our Makefiles currently add "-std=gnu++98" to g++ flags (pointed out by jcbeyler), and that would nullify my proposed check for __cplusplus. I'm hoping that we can finally: - adopt JEP 347: Adopt C++14 Language Features in HotSpot - remove "-std=gnu++98" from the Makefiles or update it to "-std=gnu++14" - bless the use of C++11 alignof and friends (and BTW: my pet C++11 feature: C++ atomics) - use alignof to check stack alignment without the compiler version checking from my previous comment https://bugs.openjdk.java.net/browse/JDK-8186780?focusedCommentId=14250378&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14250378
15-03-2019
> Is implementing it as an assembly function (in a .S file) an option, where we have full control over registers etc.? That would be (yet another) way to fix the assertion failure. But we should always treat use of assembly as a last resort, and a single arch-independent solution is achievable.
11-03-2019
Is implementing it as an assembly function (in a .S file) an option, where we have full control over registers etc.?
11-03-2019
Removed clang version from bug summary. Assertion failure observed in clang-4.0 through pre-release clang-9.
11-03-2019
I've now come to believe we should stop all of the non-portable checking of stack registers and instead use the alignment checking features in C++11. Here's a proof of concept: // Are C++11 alignof, alignas and std::max_align_t available? // This could instead be implemented via a configure check. // The __cplusplus macro is not reliable on Microsoft Visual C++: // https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/ #if __cplusplus >= 201103L \|\| (defined(_MSC_VER) && _MSC_VER >= 1900) #define HAVE_ALIGNOF 1 #endif #ifndef PRODUCT void os::verify_stack_alignment() { #ifdef HAVE_ALIGNOF STATIC_ASSERT((StackAlignmentInBytes & (alignof(std::max_align_t) - 1)) == 0); { char jiggle_alignment; std::max_align_t object; assert(((uintptr_t)&object & (alignof(std::max_align_t) - 1)) == 0, "incorrect stack alignment"); } { char jiggle_alignment; alignas(StackAlignmentInBytes) char object; assert(((uintptr_t)&object & (StackAlignmentInBytes - 1)) == 0, "incorrect stack alignment"); } #endif } #endif
11-03-2019
Martin suggested to use __builtin_frame_address() instead of the assembly code for the alignment check. After some experiments, I'm in agreement with Martin about using __builtin_frame_address() for the check, at least for the Clang case. The Clang generated code for a fastdebug build looks as the following when __builtin_frame_address() is used: (gdb) x/i $pc-4 0x7ffff748d820 <os::verify_stack_alignment()>: push %rbp (gdb) 0x7ffff748d821 <os::verify_stack_alignment()+1>: mov %rsp,%rbp (gdb) => 0x7ffff748d824 <os::verify_stack_alignment()+4>: test $0xf,%bpl (gdb) %bpl is the lower 8-bit of %rbp. With the __builtin_frame_address(), it seems to force the compiler to emit 'push %rbp; mov %rsp, %rbp', which does the necessary stack adjustment before the test.
09-03-2019
Some more info on the issue. address os::current_stack_pointer() { ... #elif defined(__clang__) intptr_t* esp; __asm__ __volatile__ ("mov %%" SPELL_REG_SP ", %0":"=r"(esp):); return (address) esp; #else ... } There is a sequence issue here when Clang is used. The code assumes that the stack adjustment is done before the 'mov %rsp ...' instruction and it would get the right value for the assert test. With Clang, this however is not the case. The necessary stack adjustment instructions are emitted only before C function call (or other necessary cases) at a later point in this case: (gdb) x/i $pc => 0x7ffff748d820 <os::verify_stack_alignment()>: mov %rsp,%rdx <<<<<<<<<<<<<<<<<< obtain rsp (gdb) 0x7ffff748d823 <os::verify_stack_alignment()+3>: mov %edx,%ecx (gdb) 0x7ffff748d825 <os::verify_stack_alignment()+5>: and $0xf,%ecx (gdb) 0x7ffff748d828 <os::verify_stack_alignment()+8>: je 0x7ffff748d891 <os::verify_stack_alignment()+113> (gdb) 0x7ffff748d82a <os::verify_stack_alignment()+10>: push %rbp <<<<<<<<<<<<<<<<<<<<< (gdb) 0x7ffff748d82b <os::verify_stack_alignment()+11>: mov %rsp,%rbp (gdb) 0x7ffff748d82e <os::verify_stack_alignment()+14>: lea 0x9133f3(%rip),%rax # 0x7ffff7da0c28 <tty>
09-03-2019
There were some code that I inserted in os::verify_stack_alignment, which made the 'rsp' always align at 16-byte before the assertion check and masked the problem. I'm now able to always reproduce the assertion failure with Clang build. Here is some info I found after digging around this morning: With a fastdebug hotspot build using Clang --------------------------------------------------------- The assertion is always reproducible. The caller of the os::verify_stack_alignment() is InterpreterRuntime::build_method_counters() when the assertion occurs. Here is the disassembled code and additional info obtained from gdb: (gdb) x/i $pc => 0x7ffff6f20327 <InterpreterRuntime::build_method_counters(JavaThread, Method)+103>: callq 0x7ffff748d820 <os::verify_stack_alignment()> (gdb) p/x $rsp $1 = 0x7ffff60fc4e0 (gdb) si Thread 2 "java" hit Breakpoint 2, os::current_stack_pointer () at /usr/local/google/home/jianglizhou/openjdk/local_fastdebug/srcs/src/hotspot/os_cpu/linux_x86/os_linux_x86.cpp:843 843 void os::verify_stack_alignment() { (gdb) p/x $rsp $3 = 0x7ffff60fc4d8 (gdb) x/i $pc => 0x7ffff748d820 <os::verify_stack_alignment()>: mov %rsp,%rdx (gdb) 0x7ffff748d823 <os::verify_stack_alignment()+3>: mov %edx,%ecx (gdb) 0x7ffff748d825 <os::verify_stack_alignment()+5>: and $0xf,%ecx (gdb) 0x7ffff748d828 <os::verify_stack_alignment()+8>: je 0x7ffff748d891 <os::verify_stack_alignment()+113> The stack pointer (rsp) is aligned at 16-byte (0x7ffff60fc4e0) before the callq instruction. The callq instruction pushes the return address on to the stack and 'rsp' is adjusted by -8. So rsp becomes 0x7ffff60fc4d8 after callq. Within the callee function, os::verify_stack_alignment(), the value of 'rsp' is immediately obtained for the check, hence the assertion. With a slowdebug hotspot build using Clang ---------------------------------------------------------- The 'rsp' is also adjusted and not aligned at 16-byte after the callq instruction. In the callee os::verify_stack_alignment() function, the compiler emits additional instructions to push the rbp, which then causes 'rsp' to be adjusted by -8 and aligned at 16-byte. 0x7ffff730b090 <os::verify_stack_alignment()>: push %rbp (gdb) 0x7ffff730b091 <os::verify_stack_alignment()+1>: mov %rsp,%rbp (gdb) 0x7ffff730b094 <os::verify_stack_alignment()+4>: sub $0x10,%rsp With a fastdebug hotspot build using gcc ----------------------------------------------------------- I also stepped through the code with a fastdebug build compiled using gcc. Gcc also emits 'push %rbp' in os::verify_stack_alignment, which made the 'rsp' to be aligned at 16-byte before the assertion check. 816 void os::verify_stack_alignment() { (gdb) x/i $pc => 0x7ffff63c9150 <os::verify_stack_alignment()>: push %rbp (gdb) 0x7ffff63c9151 <os::verify_stack_alignment()+1>: test $0x8,%spl (gdb) 0x7ffff63c9155 <os::verify_stack_alignment()+5>: mov %rsp,%rbp (gdb) 0x7ffff63c9158 <os::verify_stack_alignment()+8>:
08-03-2019
I can still reproduce the crashing assert with jdk/jdk tip and both clang-4.0 and a very recent clang near clang tip.
08-03-2019
The LLVM/Clang expert confirmed that the following code in the VM is doing the right thing for stack alignment check. #ifdef AMD64 #define SPELL_REG_SP "rsp" #else #define SPELL_REG_SP "esp #endif address os::current_stack_pointer() { ... #elif defined(__clang__) void* esp; __asm__ __volatile__ ("mov %%" SPELL_REG_SP ", %0":"=r"(esp):); return (address) esp; #else ... } We can try Clang 4.0 again to see if the assertion is reproducible then go from there.
07-03-2019
I did some experiments today with JDK 11 and clang (the last stable build that I can access) by enabling the assert for stack alignment check. I couldn't reproduce the assertion with slowdebug and fastdebug builds. I'm also checking with LLVM/Clang people on what's the recommended way for doing stack alignment check from C/C++ code. Will update this bug report after I hear back from them.
07-03-2019
[~martin] Please post a RFR on a suitable mailing list if you want hotspot engineers to engage.
12-12-2018
[~ihse] This bug is stalled on getting some attention from hotspot engineers. It's the last known patch needed for clang support.
12-12-2018
[~martin] This bug has been "in progress" for a very long time now. Is it really in progress? Should you target a specific release, or revert from the "in progress" status?
12-12-2018
I'm thinking all of this fiddling with esp is bogus, and that os::verify_stack_alignment() is more trouble than it's worth. But I'm still looking for something that will be approved. How about simply checking that __builtin_frame_address(0) is always aligned, at least with clang and gcc? #ifndef PRODUCT -void os::verify_stack_alignment() { +NOINLINE void os::verify_stack_alignment() { #ifdef AMD64 - assert(((intptr_t)os::current_stack_pointer() & (StackAlignmentInBytes-1)) == 0, "incorrect stack alignment"); + +#ifdef SPARC_WORKS + register void esp; + __asm__("mov %%" SPELL_REG_SP ", %0":"=r"(esp)); +#else + // __builtin_frame_address supported by both gcc and clang + void esp = __builtin_frame_address(0); +#endif + + assert(((intptr_t)esp & (StackAlignmentInBytes-1)) == 0, "incorrect stack alignment"); #endif } #endif
11-07-2018
True, but since the prologue of os::current_stack_pointer will make it 16-byte aligned as well (by subtraction) it will be wrong if it was wrong in the caller.
11-07-2018
clang inlined os::current_stack_pointer into its caller __in the same translation unit__ (that could be fixed in a separate change) so of course in this case it didn't have to follow the ABI. One possible fix is obvious in hindsight: -address os::current_stack_pointer() { +NOINLINE address os::current_stack_pointer() { BUT logically a call like current_stack_pointer should return the stack pointer of the __current__ frame, so should probably be a macro that does inline assembly instead of doing a function call.
11-07-2018
#ifndef PRODUCT void os::verify_stack_alignment() { -#ifdef AMD64 + // os::current_stack_pointer() is not aligned when building with clang-4.0 + // or later and optimization enabled +#if defined(AMD64) && ! defined(__clang__) assert(((intptr_t)os::current_stack_pointer() & (StackAlignmentInBytes-1)) == 0, "incorrect stack alignment"); #endif }
19-06-2018
Just disable this check when building with clang
19-06-2018
Thanks, Erik. I just confirmed that the problem is still there, and Google seems to be the organization that cares about clang on Linux, so I'll try to push a fix.
19-06-2018
That change only moved code around, it had no intention of changing anything.
19-06-2018
Magnus, I suspect you've fixed this by adding + elif test "x$TOOLCHAIN_TYPE" = xclang; then + BASIC_LDFLAGS_JVM_ONLY="-mno-omit-leaf-frame-pointer -mstack-alignment=16 \ in changeset: 49120:c04d813140dc user: ihse date: 2018-03-02 10:59 +0100 8198724: Refactor FLAGS handling in configure Reviewed-by: erikj or did that one just move code around?
19-06-2018
If there turns out to be agreement that the correct change is to modify the compiler flags instead of the hotspot source, please change the component to infrastructure/build.
14-11-2017
https://wiki.openjdk.java.net/display/Build/Supported+Build+Platforms Priority dropped to P4 and re-targeted to JDK 11, and unassigned.
14-11-2017
David, the reality is that I don't have a vested interest in getting clang-4.0 working either, at least not right now. I think the openjdk project should be buildable with various clang releases, and failures to do so should be treated as serious bugs, but I'm not willing to go beyond bug-reporting myself to make that happen.
14-11-2017
~martin: you filed a bug report relating to an "unofficial" build environment. Typically these only get handled by people with a vested interest in getting them handled - or people who can easily try the build environment and get a fix.Whether a suitable person will view the bug report is completely unknown. Hence my suggestion to ask on hotspot-dev to get the answer about stack alignment assumptions/requirements.
14-11-2017
David, I never asked about this on hotspot-dev. I filed a good bug report - shouldn't that be enough? George, you could try to find an owner for this issue, but maybe build team and hotspot team need to work together? I have scripts that apply my easy workaround, so it's not blocking me.
14-11-2017
[~martin], will you be able to complete this change for jdk10 before the deadline (approx. 01-Dec-2017)? Otherwise, please move this issue to jdk11. Thanks.
13-11-2017
I suggest asking on hotspot-dev. It's unlikely people with an answer will happen to see this bug report without being directed to it.
30-08-2017
>> Please move to infrastructure->build if this does seem to be a clang configuration issue. I think it's more likely a cross-team issue. It's not at all clear what sort of stack alignment hotspot would like to see from the compiler. I'm looking for a statement like "hotspot requires 16-byte stack alignment from its native compiler on x86_64, but only 8 on 32-bit x86" I also don't know how hotspot's generated code stack alignment interacts with the build compiler's stack alignment and the alignment of the compiler used to build jni code. The fact that simply suppressing the assert seems to work suggests that the stack alignment doesn't matter that much ?!
30-08-2017
We are very likely to have made mistakes regarding what's mac specific and clang specific. If you think this flag is relevant on linux with clang, then changing the conditional seems like the correct thing to do.
29-08-2017
I think there is an assumption that if building OSX then clang must be compiler. There may well be inconsistencies between OSX settings, that happen to use clang, and the clang specific settings. Please move to infrastructure->build if this does seem to be a clang configuration issue. Thanks.
29-08-2017
Since this is looking more like a build team problem, adding selected members as watchers to this bug.
28-08-2017
There seems to be a lingering assumption in some of the config machinery that macosx === clang. In flags.m4 I see: elif test "x$OPENJDK_$1_OS" = xmacosx; then ... $2JVM_CFLAGS="[$]$2JVM_CFLAGS ... -mstack-alignment=16 which seems doubly wrong. -mstack-alignment is a clang, not gcc flag, so any use of it should be guarded by toolchain type == clang. OTOH, when clang is used on linux x86_64, then we probably want that same flag, which doesn't seem to get added. I'm currently thinking this is a build infrastructure error.
28-08-2017
I have to wonder what the implications are of a stack that does not match StackAlignmentInBytes?
28-08-2017
One possibility is to exempt clang from the stack alignment requirement, which was probably designed as a sanity check for gcc compilations: --- a/src/os_cpu/linux_x86/vm/os_linux_x86.cpp +++ b/src/os_cpu/linux_x86/vm/os_linux_x86.cpp @@ -821,7 +821,7 @@ #ifndef PRODUCT void os::verify_stack_alignment() { -#ifdef AMD64 +#if defined(AMD64) && ! defined(__clang__) assert(((intptr_t)os::current_stack_pointer() & (StackAlignmentInBytes-1)) == 0, "incorrect stack alignment"); #endif } Another possibility is to fiddle with clang's stack alignment flags and to insist on using them somehow. $ clang-4.0 --help \|& grep stack.*align -mstack-alignment=<value> Set the stack alignment -mstackrealign Force realign the stack at entry to every function but you don't want to do that just to fix this assertion failure.
25-08-2017
Workaround is to add to make flags: BUILD_LIBJVM_os_linux_x86.cpp_OPTIMIZATION=NONE
25-08-2017

Relates :	JDK-8186787 - clang-4.0 SIGSEGV in Unsafe_PutByte
Relates :	JDK-8174050 - Compilation errors with clang-4.0