Re: RFR: 8186780: clang-4.0 fastdebug assertion failure in os_linux_x86:os::verify_stack_alignment()

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186780: clang-4.0 fastdebug assertion failure in os_linux_x86:os::verify_stack_alignment()

David Holmes
Hi Martin,

On 20/06/2018 3:03 AM, Martin Buchholz wrote:
> (There's surely a better fix that involves refactoring os/cpu/compiler
> support)
>
> 8186780: clang-4.0 fastdebug assertion failure in
> os_linux_x86:os::verify_stack_alignment()
> http://cr.openjdk.java.net/~martin/webrevs/jdk/clang-verify_stack_alignment/
> https://bugs.openjdk.java.net/browse/JDK-8186780

I remain concerned about what it may mean for the stack pointer to not
be aligned. I would have thought stack pointer alignment was part of the
ABI for a CPU architecture, not something the compiler could choose at
will? What about all the other code that uses StackAlignmentInBytes ??

That aside your fix excludes the assert when building with clang for
linux x86 as intended. And I see that for BSD x86 (where we also use
clang) that verify_stack_alignment is empty.

Thanks,
David
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186780: clang-4.0 fastdebug assertion failure in os_linux_x86:os::verify_stack_alignment()

Erik Joelsson
Hello,

It's very probable that we have made several such mistakes with our
flags, since for the most part, we have a 1-1 mapping of compiler and
OS. We certainly appreciate correcting this whenever possible. I don't
have the expertise to verify your claim here, but I will take your word
for it.

The change looks ok, but there is already a big block of clang specific
stuff close by. Perhaps move this assignment there?

/Erik


On 2018-06-20 16:03, Martin Buchholz wrote:

> Hi David and build-dev folk,
>
> After way too much build/hotspot hacking, I have a better fix:
>
> 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.  Fix is obvious in hindsight:
>
> -address os::current_stack_pointer() {
> +NOINLINE address os::current_stack_pointer() {
>
> While working on this I noticed that the clang stack alignment flags on
> Linux are missing.  They should be moved from a macosx-specific check to a
> clang-specific check.  macosx is not synonymous with clang!
>
> --- a/make/autoconf/flags-cflags.m4
> +++ b/make/autoconf/flags-cflags.m4
> @@ -470,6 +470,14 @@ AC_DEFUN([FLAGS_SETUP_CFLAGS_HELPER],
>       # COMMON to gcc and clang
>       TOOLCHAIN_CFLAGS_JVM="-pipe -fno-rtti -fno-exceptions \
>           -fvisibility=hidden -fno-strict-aliasing -fno-omit-frame-pointer"
> +
> +    if test "x$TOOLCHAIN_TYPE" = xclang; then
> +      # In principle the stack alignment below is cpu- and ABI-dependent
> and
> +      # should agree with values of StackAlignmentInBytes in various
> +      # src/hotspot/cpu/*/globalDefinitions_*.hpp files, but this value
> +      # currently works for all platforms.
> +      TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM
> -mno-omit-leaf-frame-pointer -mstack-alignment=16"
> +    fi
>     fi
>
>     if test "x$TOOLCHAIN_TYPE" = xgcc; then
> @@ -601,10 +609,6 @@ AC_DEFUN([FLAGS_SETUP_CFLAGS_HELPER],
>       fi
>     fi
>
> -  if test "x$OPENJDK_TARGET_OS" = xmacosx; then
> -    OS_CFLAGS_JVM="$OS_CFLAGS_JVM -mno-omit-leaf-frame-pointer
> -mstack-alignment=16"
> -  fi
> -
>     # Optional POSIX functionality needed by the JVM
>     #
>     # Check if clock_gettime is available and in which library. This
> indicates
>
>
> 8186780: clang-4.0 fastdebug assertion failure in
> os_linux_x86:os::verify_stack_alignment()
> http://cr.openjdk.java.net/~martin/webrevs/jdk/clang-stack-alignment/
> https://bugs.openjdk.java.net/browse/JDK-8186780
>
> On Wed, Jun 20, 2018 at 12:30 AM, David Holmes <[hidden email]>
> wrote:
>
>> Hi Martin,
>>
>>
>> On 20/06/2018 3:03 AM, Martin Buchholz wrote:
>>
>>> (There's surely a better fix that involves refactoring os/cpu/compiler
>>> support)
>>>
>>> 8186780: clang-4.0 fastdebug assertion failure in
>>> os_linux_x86:os::verify_stack_alignment()
>>> http://cr.openjdk.java.net/~martin/webrevs/jdk/clang-verify_
>>> stack_alignment/
>>> https://bugs.openjdk.java.net/browse/JDK-8186780
>>>
>> I remain concerned about what it may mean for the stack pointer to not be
>> aligned. I would have thought stack pointer alignment was part of the ABI
>> for a CPU architecture, not something the compiler could choose at will?
>> What about all the other code that uses StackAlignmentInBytes ??
>>
>> That aside your fix excludes the assert when building with clang for linux
>> x86 as intended. And I see that for BSD x86 (where we also use clang) that
>> verify_stack_alignment is empty.
>>
>> Thanks,
>> David
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186780: clang-4.0 fastdebug assertion failure in os_linux_x86:os::verify_stack_alignment()

Erik Joelsson
Looks good to me.

/Erik


On 2018-06-20 16:49, Martin Buchholz wrote:

> Thanks Erik.
>
> On Wed, Jun 20, 2018 at 4:19 PM, Erik Joelsson
> <[hidden email] <mailto:[hidden email]>> wrote:
>
>     Hello,
>
>     It's very probable that we have made several such mistakes with
>     our flags, since for the most part, we have a 1-1 mapping of
>     compiler and OS. We certainly appreciate correcting this whenever
>     possible. I don't have the expertise to verify your claim here,
>     but I will take your word for it.
>
>     The change looks ok, but there is already a big block of clang
>     specific stuff close by. Perhaps move this assignment there?
>
>
> Yes, that does look like a better place:
>
>  --- a/make/autoconf/flags-cflags.m4
> +++ b/make/autoconf/flags-cflags.m4
> @@ -470,14 +470,6 @@ AC_DEFUN([FLAGS_SETUP_CFLAGS_HELPER],
>      # COMMON to gcc and clang
>      TOOLCHAIN_CFLAGS_JVM="-pipe -fno-rtti -fno-exceptions \
>          -fvisibility=hidden -fno-strict-aliasing -fno-omit-frame-pointer"
> -
> -    if test "x$TOOLCHAIN_TYPE" = xclang; then
> -      # In principle the stack alignment below is cpu- and
> ABI-dependent and
> -      # should agree with values of StackAlignmentInBytes in various
> -      # src/hotspot/cpu/*/globalDefinitions_*.hpp files, but this value
> -      # currently works for all platforms.
> -      TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM
> -mno-omit-leaf-frame-pointer -mstack-alignment=16"
> -    fi
>    fi
>    if test "x$TOOLCHAIN_TYPE" = xgcc; then
> @@ -499,6 +491,12 @@ AC_DEFUN([FLAGS_SETUP_CFLAGS_HELPER],
>      # (see http://llvm.org/bugs/show_bug.cgi?id=7554)
>      TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM -flimit-debug-info"
> +    # In principle the stack alignment below is cpu- and
> ABI-dependent and
> +    # should agree with values of StackAlignmentInBytes in various
> +    # src/hotspot/cpu/*/globalDefinitions_*.hpp files, but this value
> +    # currently works for all platforms.
> +    TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM
> -mno-omit-leaf-frame-pointer -mstack-alignment=16"
> +
>      if test "x$OPENJDK_TARGET_OS" = xlinux; then
>        TOOLCHAIN_CFLAGS_JDK="-pipe"
>  TOOLCHAIN_CFLAGS_JDK_CONLY="-fno-strict-aliasing" # technically NOT
> for CXX
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186780: clang-4.0 fastdebug assertion failure in os_linux_x86:os::verify_stack_alignment()

David Holmes
In reply to this post by David Holmes
On 21/06/2018 10:05 AM, Martin Buchholz wrote:

> On Wed, Jun 20, 2018 at 4:03 PM, Martin Buchholz <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Hi David and build-dev folk,
>
>     After way too much build/hotspot hacking, I have a better fix:
>
>     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.  Fix is
>     obvious in hindsight:
>
>     -address os::current_stack_pointer() {
>     +NOINLINE address os::current_stack_pointer() {
>
>
> If y'all like the addition of NOINLINE, it should probably be added to
> all of the 14 variants of os::current_stack_pointer.
> Gives me a chance to try out the submit repo.

I can't help but think other platforms actually rely on it being inlined
so that it really does return the stack pointer of the method calling
os::current_stack_pointer!

David
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186780: clang-4.0 fastdebug assertion failure in os_linux_x86:os::verify_stack_alignment()

Thomas Stüfe-2
On Thu, Jun 21, 2018 at 1:27 PM, David Holmes <[hidden email]> wrote:

> On 21/06/2018 10:05 AM, Martin Buchholz wrote:
>>
>> On Wed, Jun 20, 2018 at 4:03 PM, Martin Buchholz <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>     Hi David and build-dev folk,
>>
>>     After way too much build/hotspot hacking, I have a better fix:
>>
>>     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.  Fix is
>>     obvious in hindsight:
>>
>>     -address os::current_stack_pointer() {
>>     +NOINLINE address os::current_stack_pointer() {
>>
>>
>> If y'all like the addition of NOINLINE, it should probably be added to all
>> of the 14 variants of os::current_stack_pointer.
>> Gives me a chance to try out the submit repo.
>
>
> I can't help but think other platforms actually rely on it being inlined so
> that it really does return the stack pointer of the method calling
> os::current_stack_pointer!
>

But we only inline today if caller is in the same translation unit and
builds with optimization, no?

..Thomas

> David
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186780: clang-4.0 fastdebug assertion failure in os_linux_x86:os::verify_stack_alignment()

David Holmes
On 21/06/2018 10:37 PM, Thomas Stüfe wrote:

> On Thu, Jun 21, 2018 at 1:27 PM, David Holmes <[hidden email]> wrote:
>> On 21/06/2018 10:05 AM, Martin Buchholz wrote:
>>>
>>> On Wed, Jun 20, 2018 at 4:03 PM, Martin Buchholz <[hidden email]
>>> <mailto:[hidden email]>> wrote:
>>>
>>>      Hi David and build-dev folk,
>>>
>>>      After way too much build/hotspot hacking, I have a better fix:
>>>
>>>      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.  Fix is
>>>      obvious in hindsight:
>>>
>>>      -address os::current_stack_pointer() {
>>>      +NOINLINE address os::current_stack_pointer() {
>>>
>>>
>>> If y'all like the addition of NOINLINE, it should probably be added to all
>>> of the 14 variants of os::current_stack_pointer.
>>> Gives me a chance to try out the submit repo.
>>
>>
>> I can't help but think other platforms actually rely on it being inlined so
>> that it really does return the stack pointer of the method calling
>> os::current_stack_pointer!
>>
>
> But we only inline today if caller is in the same translation unit and
> builds with optimization, no?

Don't know, but Martin's encountering a case where it is being inlined -
is that likely to be unique for some reason?

I never assume the compiler does the obvious these days :) or that there
can't be clever link-time tricks as well.

Maybe the safest thing to do is to only make a change for the clang case
and leave everything else alone.

David
-----

>
> ..Thomas
>
>> David
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186780: clang-4.0 fastdebug assertion failure in os_linux_x86:os::verify_stack_alignment()

Thomas Stüfe-2
On Fri, Jun 22, 2018 at 1:57 PM, David Holmes <[hidden email]> wrote:

> On 21/06/2018 10:37 PM, Thomas Stüfe wrote:
>>
>> On Thu, Jun 21, 2018 at 1:27 PM, David Holmes <[hidden email]>
>> wrote:
>>>
>>> On 21/06/2018 10:05 AM, Martin Buchholz wrote:
>>>>
>>>>
>>>> On Wed, Jun 20, 2018 at 4:03 PM, Martin Buchholz <[hidden email]
>>>> <mailto:[hidden email]>> wrote:
>>>>
>>>>      Hi David and build-dev folk,
>>>>
>>>>      After way too much build/hotspot hacking, I have a better fix:
>>>>
>>>>      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.  Fix is
>>>>      obvious in hindsight:
>>>>
>>>>      -address os::current_stack_pointer() {
>>>>      +NOINLINE address os::current_stack_pointer() {
>>>>
>>>>
>>>> If y'all like the addition of NOINLINE, it should probably be added to
>>>> all
>>>> of the 14 variants of os::current_stack_pointer.
>>>> Gives me a chance to try out the submit repo.
>>>
>>>
>>>
>>> I can't help but think other platforms actually rely on it being inlined
>>> so
>>> that it really does return the stack pointer of the method calling
>>> os::current_stack_pointer!
>>>
>>
>> But we only inline today if caller is in the same translation unit and
>> builds with optimization, no?
>
>
> Don't know, but Martin's encountering a case where it is being inlined - is
> that likely to be unique for some reason?
>

Okay I may have confused myself.

My original reasoning was: A lot of calls to
os::current_stack_pointer() today happen not-inlined. That includes
calls from outside os_<os>_<cpu>.cpp, and calls from inside
os_<os>_<cpu>.cpp if slowdebug. Hence, since the VM - in particular
the slowdebug one - seems to work fine, it should be okay to mark
os::current_stack_pointer() unconditionally as NOINLINE.

However, then I saw that the only "real" function (real meaning not
just asserting something) using os::current_stack_pointer() and living
in the same translation unit is os::current_frame(), e.g. on linux:

frame os::current_frame() {
  intptr_t* fp = _get_previous_fp();
  frame myframe((intptr_t*)os::current_stack_pointer(),
                (intptr_t*)fp,
                CAST_FROM_FN_PTR(address, os::current_frame));
  if (os::is_first_C_frame(&myframe)) {
    // stack is not walkable
    return frame();
  } else {
    return os::get_sender_for_C_frame(&myframe);
  }
}

how does this even work if os::current_stack_pointer() is not inlined?
In slowdebug? Would the frame object in this case not contain the SP
from the frame built up for os::current_stack_pointer()?

So, now I wonder if making os::current_stack_pointer() NOINLINE would
break os::current_frame() - make if produce frames with a slightly-off
SP. os::current_frame() is used e.g. in NMT. So, I wonder if NMT still
works if os::current_stack_pointer is made NOINLINE, or in slowdebug.

> I never assume the compiler does the obvious these days :) or that there
> can't be clever link-time tricks as well.

Oh. I did not think of that at all, you are right.

>
> Maybe the safest thing to do is to only make a change for the clang case and
> leave everything else alone.
>

It would be better to know for sure, though.

..Thomas

> David
> -----
>
>>
>> ..Thomas
>>
>>> David
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186780: clang-4.0 fastdebug assertion failure in os_linux_x86:os::verify_stack_alignment()

Martin Buchholz-3
(I keep trying not to become a hotspot engineer...)

I would define current_stack_pointer as a macro using expression
statements: prototype is:

#if defined(__clang__)
#define sp_3() ({ intptr_t rsp; __asm__ __volatile__ ("mov %% rsp,
%0":"=r"(rsp):); rsp; })
#else
#define sp_3() ({ register intptr_t rsp asm ("rsp"); rsp; })
#endif

Then we can call that everywhere and actually get the right answer.
(Other compilers and cpu architectures left as an exercise for the reader)
(Probably we won't be able to do this for every compiler we'd like to use)

Then we can make all the
os::verify_stack_alignment functions in non-product mode NOINLINE so that
they have a real stack frame with a real stack pointer.

But that's starting to be a big change and a hotspot culture change, so can
hotspot engineers please take over ?!

On Fri, Jun 22, 2018 at 8:27 AM, Thomas Stüfe <[hidden email]>
wrote:

> On Fri, Jun 22, 2018 at 1:57 PM, David Holmes <[hidden email]>
> wrote:
> > On 21/06/2018 10:37 PM, Thomas Stüfe wrote:
> >>
> >> On Thu, Jun 21, 2018 at 1:27 PM, David Holmes <[hidden email]>
> >> wrote:
> >>>
> >>> On 21/06/2018 10:05 AM, Martin Buchholz wrote:
> >>>>
> >>>>
> >>>> On Wed, Jun 20, 2018 at 4:03 PM, Martin Buchholz <[hidden email]
> >>>> <mailto:[hidden email]>> wrote:
> >>>>
> >>>>      Hi David and build-dev folk,
> >>>>
> >>>>      After way too much build/hotspot hacking, I have a better fix:
> >>>>
> >>>>      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.  Fix
> is
> >>>>      obvious in hindsight:
> >>>>
> >>>>      -address os::current_stack_pointer() {
> >>>>      +NOINLINE address os::current_stack_pointer() {
> >>>>
> >>>>
> >>>> If y'all like the addition of NOINLINE, it should probably be added to
> >>>> all
> >>>> of the 14 variants of os::current_stack_pointer.
> >>>> Gives me a chance to try out the submit repo.
> >>>
> >>>
> >>>
> >>> I can't help but think other platforms actually rely on it being
> inlined
> >>> so
> >>> that it really does return the stack pointer of the method calling
> >>> os::current_stack_pointer!
> >>>
> >>
> >> But we only inline today if caller is in the same translation unit and
> >> builds with optimization, no?
> >
> >
> > Don't know, but Martin's encountering a case where it is being inlined -
> is
> > that likely to be unique for some reason?
> >
>
> Okay I may have confused myself.
>
> My original reasoning was: A lot of calls to
> os::current_stack_pointer() today happen not-inlined. That includes
> calls from outside os_<os>_<cpu>.cpp, and calls from inside
> os_<os>_<cpu>.cpp if slowdebug. Hence, since the VM - in particular
> the slowdebug one - seems to work fine, it should be okay to mark
> os::current_stack_pointer() unconditionally as NOINLINE.
>
> However, then I saw that the only "real" function (real meaning not
> just asserting something) using os::current_stack_pointer() and living
> in the same translation unit is os::current_frame(), e.g. on linux:
>
> frame os::current_frame() {
>   intptr_t* fp = _get_previous_fp();
>   frame myframe((intptr_t*)os::current_stack_pointer(),
>                 (intptr_t*)fp,
>                 CAST_FROM_FN_PTR(address, os::current_frame));
>   if (os::is_first_C_frame(&myframe)) {
>     // stack is not walkable
>     return frame();
>   } else {
>     return os::get_sender_for_C_frame(&myframe);
>   }
> }
>
> how does this even work if os::current_stack_pointer() is not inlined?
> In slowdebug? Would the frame object in this case not contain the SP
> from the frame built up for os::current_stack_pointer()?
>
> So, now I wonder if making os::current_stack_pointer() NOINLINE would
> break os::current_frame() - make if produce frames with a slightly-off
> SP. os::current_frame() is used e.g. in NMT. So, I wonder if NMT still
> works if os::current_stack_pointer is made NOINLINE, or in slowdebug.
>
> > I never assume the compiler does the obvious these days :) or that there
> > can't be clever link-time tricks as well.
>
> Oh. I did not think of that at all, you are right.
>
> >
> > Maybe the safest thing to do is to only make a change for the clang case
> and
> > leave everything else alone.
> >
>
> It would be better to know for sure, though.
>
> ..Thomas
>
> > David
> > -----
> >
> >>
> >> ..Thomas
> >>
> >>> David
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186780: clang-4.0 fastdebug assertion failure in os_linux_x86:os::verify_stack_alignment()

Martin Buchholz-3
Since the question of how to get the stack pointer in hotspot is
unexpectedly difficult, I propose we split into two separate changes and we
can submit the make/ changes that Erik is happy with.