RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

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

RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Dmitry Samersoff-2
Hello Everybody,

Please review small fix

http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.01/

CR:

https://bugs.openjdk.java.net/browse/JDK-8203481

Testing:

jfr tests that depends to safe_for_sender functionality

./jdk/jdk/jfr/api/consumer/TestRecordedFullStackTrace.java
./jdk/jdk/jfr/event/profiling/TestFullStackTrace.java

fails on AARCH64.

These tests passed after the fix.


--
Dmitry Samersoff
http://devnull.samersoff.net
* There will come soft rains ...

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Daniel D. Daugherty
Hi Dmitry,

I think something else must be going wrong here. The unextended SP
is typically documented like this:

   // This is the sp before any possible extension (adapter/locals).
   intptr_t* unextended_sp = interpreter_frame_sender_sp();

and like this:

   // stack frames shouldn't be much larger than max_stack elements
   // this test requires the use of unextended_sp which is the sp as seen by
   // the current frame, and not sp which is the "raw" pc which could point
   // further because of local variables of the callee method inserted after
   // method arguments
   if (fp() - unextended_sp() > 1024 +
m->max_stack()*Interpreter::stackElementSize) {
     return false;
   }

So I think this existing comment and assertion are correct:

     L72:   // unextended sp must be within the stack and above or equal sp
     L73:   bool unextended_sp_safe = (unextended_sp <
thread->stack_base()) &&
     L74:                             (unextended_sp >= sp);

Also, your proposed fix only changed this for two platforms. The same
logic exists on 'arm' and 'sparc' also.

Dan


On 5/21/18 9:44 AM, Dmitry Samersoff wrote:

> Hello Everybody,
>
> Please review small fix
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.01/
>
> CR:
>
> https://bugs.openjdk.java.net/browse/JDK-8203481
>
> Testing:
>
> jfr tests that depends to safe_for_sender functionality
>
> ./jdk/jdk/jfr/api/consumer/TestRecordedFullStackTrace.java
> ./jdk/jdk/jfr/event/profiling/TestFullStackTrace.java
>
> fails on AARCH64.
>
> These tests passed after the fix.
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Dmitry Samersoff-2
Dan,

Thank you!

I'll re-check what is happening on AArch64 and come back.

-Dmitry


On 21.05.2018 17:28, Daniel D. Daugherty wrote:

> Hi Dmitry,
>
> I think something else must be going wrong here. The unextended SP
> is typically documented like this:
>
>   // This is the sp before any possible extension (adapter/locals).
>   intptr_t* unextended_sp = interpreter_frame_sender_sp();
>
> and like this:
>
>   // stack frames shouldn't be much larger than max_stack elements
>   // this test requires the use of unextended_sp which is the sp as seen by
>   // the current frame, and not sp which is the "raw" pc which could point
>   // further because of local variables of the callee method inserted after
>   // method arguments
>   if (fp() - unextended_sp() > 1024 +
> m->max_stack()*Interpreter::stackElementSize) {
>     return false;
>   }
>
> So I think this existing comment and assertion are correct:
>
>     L72:   // unextended sp must be within the stack and above or equal sp
>     L73:   bool unextended_sp_safe = (unextended_sp <
> thread->stack_base()) &&
>     L74:                             (unextended_sp >= sp);
>
> Also, your proposed fix only changed this for two platforms. The same
> logic exists on 'arm' and 'sparc' also.
>
> Dan
>
>
> On 5/21/18 9:44 AM, Dmitry Samersoff wrote:
>> Hello Everybody,
>>
>> Please review small fix
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.01/
>>
>> CR:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8203481
>>
>> Testing:
>>
>> jfr tests that depends to safe_for_sender functionality
>>
>> ./jdk/jdk/jfr/api/consumer/TestRecordedFullStackTrace.java
>> ./jdk/jdk/jfr/event/profiling/TestFullStackTrace.java
>>
>> fails on AARCH64.
>>
>> These tests passed after the fix.
>>
>>
>


--
Dmitry Samersoff
http://devnull.samersoff.net
* There will come soft rains ...

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Dmitry Samersoff-2
In reply to this post by Daniel D. Daugherty
Dan,

I have an impression that _unextended_sp doesn't contain "unextended"
value at the time we constructing frame() object on both aarch64 and x86.

On x86 _unextended_sp always equals to _sp at this point, on aarch64
_unextended_sp is less or equal to _sp.

if I add

assert((_unextended_sp >= _sp), "DMS: unextended_sp_safe");

inside

frame::frame(intptr_t* sp, intptr_t* unextended_sp, intptr_t* fp,
address pc)

on aarhc64 java crashes immediately (java -version).


1.

#22 0x0000ffff905d7b9c in report_vm_error (
    file=file@entry=0xffff91361260
"/root/dsamersoff/esc/JDK-8203481/jdk/src/hotspot/cpu/aarch64/frame_aarch64.inline.hpp",
line=line@entry=81,
    error_msg=error_msg@entry=0xffff91371240 "assert((_unextended_sp >=
_sp)) failed", detail_fmt=detail_fmt@entry=0xffff91371228 "DMS:
unextended_sp_safe")
    at
/root/dsamersoff/esc/JDK-8203481/jdk/src/hotspot/share/utilities/debug.cpp:231
#23 0x0000ffff906e2230 in frame::frame (
    pc=0xffff79081400
"\264\003_\370\277\003\037\370\266\203[\370\270\003\\\370\272\203\\\370\254\203^\370\201\r@\371\241\003^\370\201\004",
    fp=0xffff8fc86f70, unextended_sp=0xffff8fc86ee0, sp=<optimized out>,
this=0xffff8fc866d0)
    at
/root/dsamersoff/esc/JDK-8203481/jdk/src/hotspot/cpu/aarch64/frame_aarch64.inline.hpp:81
#24 frame::sender_for_interpreter_frame (map=0xffff8fc86840,
this=0xffff8fc86808)
    at
/root/dsamersoff/esc/JDK-8203481/jdk/src/hotspot/cpu/aarch64/frame_aarch64.cpp:441
#25 frame::sender (this=this@entry=0xffff8fc86808,
map=map@entry=0xffff8fc86840)
    at
/root/dsamersoff/esc/JDK-8203481/jdk/src/hotspot/cpu/aarch64/frame_aarch64.cpp:493
#26 0x0000ffff90ac1f44 in vframeStreamCommon::next (this=0xffff8fc86800)
    at
/root/dsamersoff/esc/JDK-8203481/jdk/src/hotspot/share/runtime/vframe.inline.hpp:47
#27 JVM_GetStackAccessControlContext (env=0x0, cls=<optimized out>) at
/root/dsamersoff/esc/JDK-8203481/jdk/src/hotspot/share/prims/jvm.cpp:1357
#28 0x0000ffff79088b0c in ?? ()
#29 0x000000008b506a48 in ?? ()


(gdb) p _unextended_sp
$1 = (intptr_t *) 0xffff8fc86ee0
(gdb) p _sp
$2 = (intptr_t *) 0xffff8fc86f10

-Dmitry


On 05/21/2018 05:28 PM, Daniel D. Daugherty wrote:

> Hi Dmitry,
>
> I think something else must be going wrong here. The unextended SP
> is typically documented like this:
>
>   // This is the sp before any possible extension (adapter/locals).
>   intptr_t* unextended_sp = interpreter_frame_sender_sp();
>
> and like this:
>
>   // stack frames shouldn't be much larger than max_stack elements
>   // this test requires the use of unextended_sp which is the sp as seen by
>   // the current frame, and not sp which is the "raw" pc which could point
>   // further because of local variables of the callee method inserted after
>   // method arguments
>   if (fp() - unextended_sp() > 1024 +
> m->max_stack()*Interpreter::stackElementSize) {
>     return false;
>   }
>
> So I think this existing comment and assertion are correct:
>
>     L72:   // unextended sp must be within the stack and above or equal sp
>     L73:   bool unextended_sp_safe = (unextended_sp <
> thread->stack_base()) &&
>     L74:                             (unextended_sp >= sp);
>
> Also, your proposed fix only changed this for two platforms. The same
> logic exists on 'arm' and 'sparc' also.
>
> Dan
>
>
> On 5/21/18 9:44 AM, Dmitry Samersoff wrote:
>> Hello Everybody,
>>
>> Please review small fix
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.01/
>>
>> CR:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8203481
>>
>> Testing:
>>
>> jfr tests that depends to safe_for_sender functionality
>>
>> ./jdk/jdk/jfr/api/consumer/TestRecordedFullStackTrace.java
>> ./jdk/jdk/jfr/event/profiling/TestFullStackTrace.java
>>
>> fails on AARCH64.
>>
>> These tests passed after the fix.
>>
>>
>


--
Dmitry Samersoff
http://devnull.samersoff.net
* There will come soft rains ...

Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Andrew Haley
In reply to this post by Dmitry Samersoff-2
On 05/23/2018 08:16 AM, Dmitry Samersoff wrote:
>
> I'll re-check what is happening on AArch64 and come back.

I had a look.  I know what's happening.

While we're interpreting, the machine SP is always <= the Java
expression SP, ESP.  When we enter a new intrpreted frame, the old
machine SP is saved in interpreter_frame_sender_sp in the new frame
and the machine SP is adjusted so that it is just below ESP.
interpreter_frame_sender_sp is used to calculate the unextended_sp
while we're unwinding the frame.

So, A correct frame layout can look like this:

 0x000003ffb61bdf40: 0x000003ffb61bdfb0 #2 method java.security.AccessController.getContext()Ljava/security/AccessControlContext; @ 0
                                        - 1 locals 5 max stack
 0x000003ffb61bdf38: 0x000003ffb61bdf20 interpreter_frame_sender_sp
 0x000003ffb61bdf30: 0x000003ffb61bdef0 interpreter_frame_last_sp
 0x000003ffb61bdf28: 0x000003ff88fb0bc0 interpreter_frame_method
 0x000003ffb61bdf20: 0x0000000000000000 unextended_sp for #3
                                        interpreter_frame_mdp
 0x000003ffb61bdf18: 0x0000000000000000
 0x000003ffb61bdf10: 0x000000070ff06a48 interpreter_frame_mirror
 0x000003ffb61bdf08: 0x000003ff88fb0de8 interpreter_frame_cache
 0x000003ffb61bdf00: 0x000003ffb61bdf50 interpreter_frame_locals
 0x000003ffb61bdef8: 0x000003ff88fb0b90 interpreter_frame_bcp
 0x000003ffb61bdef0: 0x000003ffb61bdef0 interpreter_frame_initial_sp
 0x000003ffb61bdee8: 0x000000070ff06a48
 0x000003ffb61bdee0: 0x0000000000000000 sp for #2
 0x000003ffb61bded8: 0x000003ffa1081360

 0x000003ffb61bded0: 0x000003ffb61bdf40 #1 method java.security.AccessController.getStackAccessControlContext()Ljava/security/AccessControlContext; @ 0
                                        - 0 locals 1 max stack
 0x000003ffb61bdec8: 0x000003ffb61bdeb0 interpreter_frame_sender_sp
 0x000003ffb61bdec0: 0x0000000000000000 interpreter_frame_last_sp
 0x000003ffb61bdeb8: 0x000003ff88fb0a30 interpreter_frame_method
 0x000003ffb61bdeb0: 0x0000000000000000 unextended_sp for #2
                                        interpreter_frame_mdp
 0x000003ffb61bdea8: 0x0000000000000000
 0x000003ffb61bdea0: 0x000000070ff06a48 interpreter_frame_mirror
 0x000003ffb61bde98: 0x000003ff88fb0de8 interpreter_frame_cache
 0x000003ffb61bde90: 0x000003ffb61bdee8 interpreter_frame_locals
 0x000003ffb61bde88: 0x0000000000000000 interpreter_frame_bcp
 0x000003ffb61bde80: 0x000003ffb61bde80 sp for #1
                                        interpreter_frame_initial_sp
                                        unextended_sp for #1

Note that getStackAccessControlContext()'s saved sender SP from
AccessController.getContext() is 0x000003ffb61bdeb0: this really is
less than 0x000003ffb61bdee0, which was the SP before
getStackAccessControlContext()'s frame was created.  This is OK, and
explains why the assert failed for you.

Given that the unextended_sp can be greater or less than the saved SP,
I think the assert can be removed.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Dmitry Samersoff-2
In reply to this post by Dmitry Samersoff-2
Hello Everybody,

Please, review updated webrev:

http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.02

CR link:

https://bugs.openjdk.java.net/browse/JDK-8203481

-Dmitry

On 05/21/2018 04:44 PM, Dmitry Samersoff wrote:

> Hello Everybody,
>
> Please review small fix
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.01/
>
> CR:
>
> https://bugs.openjdk.java.net/browse/JDK-8203481
>
> Testing:
>
> jfr tests that depends to safe_for_sender functionality
>
> ./jdk/jdk/jfr/api/consumer/TestRecordedFullStackTrace.java
> ./jdk/jdk/jfr/event/profiling/TestFullStackTrace.java
>
> fails on AARCH64.
>
> These tests passed after the fix.
>
>


--
Dmitry Samersoff
http://devnull.samersoff.net
* There will come soft rains ...

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

David Holmes
Hi Dmitry,

I have to admit I still can't connect all the pieces here. When we are
operating in frame::safe_for_sender, what has set _unextended_sp? Can
you walk through a complete example? I can't quite connect Andrew's
example to the original problem.

It still seems to me that the current frame should have:

sp <= unextended_sp <= stack_base()

??

Thanks,
David

On 11/06/2018 2:54 AM, Dmitry Samersoff wrote:

> Hello Everybody,
>
> Please, review updated webrev:
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.02
>
> CR link:
>
> https://bugs.openjdk.java.net/browse/JDK-8203481
>
> -Dmitry
>
> On 05/21/2018 04:44 PM, Dmitry Samersoff wrote:
>> Hello Everybody,
>>
>> Please review small fix
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.01/
>>
>> CR:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8203481
>>
>> Testing:
>>
>> jfr tests that depends to safe_for_sender functionality
>>
>> ./jdk/jdk/jfr/api/consumer/TestRecordedFullStackTrace.java
>> ./jdk/jdk/jfr/event/profiling/TestFullStackTrace.java
>>
>> fails on AARCH64.
>>
>> These tests passed after the fix.
>>
>>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Dmitry Samersoff-2
David,

When we constructing frame [1] we get interpreter_frame_sender_sp from
fp()[unextended_sp_offset = -1] and store it as _unextended_sp.

Previously, interpreter stored caller_sp there.

But it's possible that caller_sp is below or above sp of current frame.

One of such cases is a top level frame when we iterate frames from
JVM_GetStackAccessControlContext. Iteration of recursive frames from JFR
FullStackTrace is the other case.


1.

frame frame::sender_for_interpreter_frame(RegisterMap* map) const {
  // SP is the raw SP from the sender after adapter or interpreter
  // extension.
  intptr_t* sender_sp = this->sender_sp();

  // This is the sp before any possible extension (adapter/locals).
  intptr_t* unextended_sp = interpreter_frame_sender_sp();

...

// sender_sp
intptr_t* frame::interpreter_frame_sender_sp() const {
  assert(is_interpreted_frame(), "interpreted frame expected");
  return (intptr_t*) at(interpreter_frame_sender_sp_offset);
}

-Dmitry\S



On 11.06.2018 00:05, David Holmes wrote:

> Hi Dmitry,
>
> I have to admit I still can't connect all the pieces here. When we are
> operating in frame::safe_for_sender, what has set _unextended_sp? Can
> you walk through a complete example? I can't quite connect Andrew's
> example to the original problem.
>
> It still seems to me that the current frame should have:
>
> sp <= unextended_sp <= stack_base()
>
> ??
>
> Thanks,
> David
>
> On 11/06/2018 2:54 AM, Dmitry Samersoff wrote:
>> Hello Everybody,
>>
>> Please, review updated webrev:
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.02
>>
>> CR link:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8203481
>>
>> -Dmitry
>>
>> On 05/21/2018 04:44 PM, Dmitry Samersoff wrote:
>>> Hello Everybody,
>>>
>>> Please review small fix
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.01/
>>>
>>> CR:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8203481
>>>
>>> Testing:
>>>
>>> jfr tests that depends to safe_for_sender functionality
>>>
>>> ./jdk/jdk/jfr/api/consumer/TestRecordedFullStackTrace.java
>>> ./jdk/jdk/jfr/event/profiling/TestFullStackTrace.java
>>>
>>> fails on AARCH64.
>>>
>>> These tests passed after the fix.
>>>
>>>
>>
>>


--
Dmitry Samersoff
http://devnull.samersoff.net
* There will come soft rains ...


Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Andrew Haley
In reply to this post by David Holmes
On 06/10/2018 10:05 PM, David Holmes wrote:
> I have to admit I still can't connect all the pieces here. When we are
> operating in frame::safe_for_sender, what has set _unextended_sp? Can
> you walk through a complete example? I can't quite connect Andrew's
> example to the original problem.
>
> It still seems to me that the current frame should have:
>
> sp <= unextended_sp <= stack_base()

No.  We save SP, then remove unused stack, then call int a method.
The address saved into the current method is the old SP, before stack
items were removed.  This is refereed to as the "unextended_sp" in th
eshared code, but it's actually *below* the SP when a method was
entered.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Daniel D. Daugherty
In reply to this post by Dmitry Samersoff-2
On 6/10/18 12:54 PM, Dmitry Samersoff wrote:
> Hello Everybody,
>
> Please, review updated webrev:
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.02

This is still very confusing. In one part of this thread, we're talking
like this is ARM only fix because X64 does things differently. In another
part of this thread, we're talking like this bug affects all platforms,
but I haven't seen failures on X64 due to this so...  Something is still
not making sense here.


The patch itself is inconsistent between platforms. Here's the key
logic line in all four platforms:

+  bool unextended_sp_safe = (unextended_sp < thread->stack_base());
+                             (unextended_sp <= thread->stack_base()));
+  bool unextended_sp_safe = (_UNEXTENDED_SP <= thread->stack_base());
+  bool unextended_sp_safe = (unextended_sp < thread->stack_base());

Two platforms use '<' and two use '<='.


Dan


>
> CR link:
>
> https://bugs.openjdk.java.net/browse/JDK-8203481
>
> -Dmitry
>
> On 05/21/2018 04:44 PM, Dmitry Samersoff wrote:
>> Hello Everybody,
>>
>> Please review small fix
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.01/
>>
>> CR:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8203481
>>
>> Testing:
>>
>> jfr tests that depends to safe_for_sender functionality
>>
>> ./jdk/jdk/jfr/api/consumer/TestRecordedFullStackTrace.java
>> ./jdk/jdk/jfr/event/profiling/TestFullStackTrace.java
>>
>> fails on AARCH64.
>>
>> These tests passed after the fix.
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Daniel D. Daugherty
In reply to this post by David Holmes
On 6/10/18 5:05 PM, David Holmes wrote:

> Hi Dmitry,
>
> I have to admit I still can't connect all the pieces here. When we are
> operating in frame::safe_for_sender, what has set _unextended_sp? Can
> you walk through a complete example? I can't quite connect Andrew's
> example to the original problem.
>
> It still seems to me that the current frame should have:
>
> sp <= unextended_sp <= stack_base()

David, you and I are in agreement here. At least I'm in agreement
for X64 (and SPARC).

Dan


>
> ??
>
> Thanks,
> David
>
> On 11/06/2018 2:54 AM, Dmitry Samersoff wrote:
>> Hello Everybody,
>>
>> Please, review updated webrev:
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.02
>>
>> CR link:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8203481
>>
>> -Dmitry
>>
>> On 05/21/2018 04:44 PM, Dmitry Samersoff wrote:
>>> Hello Everybody,
>>>
>>> Please review small fix
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.01/
>>>
>>> CR:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8203481
>>>
>>> Testing:
>>>
>>> jfr tests that depends to safe_for_sender functionality
>>>
>>> ./jdk/jdk/jfr/api/consumer/TestRecordedFullStackTrace.java
>>> ./jdk/jdk/jfr/event/profiling/TestFullStackTrace.java
>>>
>>> fails on AARCH64.
>>>
>>> These tests passed after the fix.
>>>
>>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Daniel D. Daugherty
In reply to this post by Dmitry Samersoff-2
On 6/14/18 7:36 AM, Dmitry Samersoff wrote:
> David,
>
> When we constructing frame [1] we get interpreter_frame_sender_sp from
> fp()[unextended_sp_offset = -1] and store it as _unextended_sp.
>
> Previously, interpreter stored caller_sp there.
>
> But it's possible that caller_sp is below or above sp of current frame.

That last sentence is not making sense to me.

At least on X64, that last sentence doesn't make sense to me.


So I'm in an interpreter frame and I have a caller SP value saved
in my frame. I'm about to make a call (create a new frame) and you
are saying that the current sp in my current frame might be below
or above the saved caller SP value. I expect it to be below where
below means cur SP <= saved caller SP.

Dan



>
> One of such cases is a top level frame when we iterate frames from
> JVM_GetStackAccessControlContext. Iteration of recursive frames from JFR
> FullStackTrace is the other case.
>
>
> 1.
>
> frame frame::sender_for_interpreter_frame(RegisterMap* map) const {
>    // SP is the raw SP from the sender after adapter or interpreter
>    // extension.
>    intptr_t* sender_sp = this->sender_sp();
>
>    // This is the sp before any possible extension (adapter/locals).
>    intptr_t* unextended_sp = interpreter_frame_sender_sp();
>
> ...
>
> // sender_sp
> intptr_t* frame::interpreter_frame_sender_sp() const {
>    assert(is_interpreted_frame(), "interpreted frame expected");
>    return (intptr_t*) at(interpreter_frame_sender_sp_offset);
> }
>
> -Dmitry\S
>
>
>
> On 11.06.2018 00:05, David Holmes wrote:
>> Hi Dmitry,
>>
>> I have to admit I still can't connect all the pieces here. When we are
>> operating in frame::safe_for_sender, what has set _unextended_sp? Can
>> you walk through a complete example? I can't quite connect Andrew's
>> example to the original problem.
>>
>> It still seems to me that the current frame should have:
>>
>> sp <= unextended_sp <= stack_base()
>>
>> ??
>>
>> Thanks,
>> David
>>
>> On 11/06/2018 2:54 AM, Dmitry Samersoff wrote:
>>> Hello Everybody,
>>>
>>> Please, review updated webrev:
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.02
>>>
>>> CR link:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8203481
>>>
>>> -Dmitry
>>>
>>> On 05/21/2018 04:44 PM, Dmitry Samersoff wrote:
>>>> Hello Everybody,
>>>>
>>>> Please review small fix
>>>>
>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.01/
>>>>
>>>> CR:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8203481
>>>>
>>>> Testing:
>>>>
>>>> jfr tests that depends to safe_for_sender functionality
>>>>
>>>> ./jdk/jdk/jfr/api/consumer/TestRecordedFullStackTrace.java
>>>> ./jdk/jdk/jfr/event/profiling/TestFullStackTrace.java
>>>>
>>>> fails on AARCH64.
>>>>
>>>> These tests passed after the fix.
>>>>
>>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Daniel D. Daugherty
In reply to this post by Andrew Haley
On 6/14/18 9:44 AM, Andrew Haley wrote:

> On 06/10/2018 10:05 PM, David Holmes wrote:
>> I have to admit I still can't connect all the pieces here. When we are
>> operating in frame::safe_for_sender, what has set _unextended_sp? Can
>> you walk through a complete example? I can't quite connect Andrew's
>> example to the original problem.
>>
>> It still seems to me that the current frame should have:
>>
>> sp <= unextended_sp <= stack_base()
> No.  We save SP, then remove unused stack, then call int a method.

Where does this "remove unused stack" come from? And is that only
an ARM thing? I don't remember seeing that on X64...


> The address saved into the current method is the old SP, before stack
> items were removed.  This is refereed to as the "unextended_sp" in th
> eshared code, but it's actually *below* the SP when a method was
> entered.

I can see how removing things from the stack (and incrementing SP) could
result in an SP value > saved SP value. It's the "removing things from
the stack" part that has me confused right now...

Also, we have a terminology problem in this thread. In this:

     sp <= unextended_sp <= stack_base()

I would say that 'sp' is below (or equal) to unextended_sp and that
'unextended_sp' is below (or equal) to stack_base(). To me "below"
is like "less than" and "above" is like "greater than".

Dan
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Dmitry Samersoff-2
In reply to this post by Daniel D. Daugherty
Dan,

On 06/14/2018 11:49 PM, Daniel D. Daugherty wrote:
> On 6/10/18 12:54 PM, Dmitry Samersoff wrote:
>> Hello Everybody,
>>
>> Please, review updated webrev:
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.02

> This is still very confusing. In one part of this thread, we're talking
> like this is ARM only fix because X64 does things differently. In another
> part of this thread, we're talking like this bug affects all platforms,
> but I haven't seen failures on X64 due to this so...  Something is still
> not making sense here.

jfr tracing testcase uses specific pattern for test, - deep recursion.

On AArch64 this case triggers assert.

On x86 unextended_sp  is equal to sp, in this case so no asserts is
triggered. So you don't have any crashes on x86.

I didn't look closely to x86 code and can't say why is this difference,
so I'm OK to keep this patch for AArh64 only until we experience a
problem on other platform.

> The patch itself is inconsistent between platforms. Here's the key
> logic line in all four platforms:
>
> +  bool unextended_sp_safe = (unextended_sp < thread->stack_base());
> +                             (unextended_sp <= thread->stack_base()));
> +  bool unextended_sp_safe = (_UNEXTENDED_SP <= thread->stack_base());
> +  bool unextended_sp_safe = (unextended_sp < thread->stack_base());
>
> Two platforms use '<' and two use '<='.

it's better to say - this code is not consistent across platforms,
I didn't touch this part of assert, just removed check of
unextended_sp against sp.

-Dmitry

>
>
> Dan
>
>
>>
>> CR link:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8203481
>>
>> -Dmitry
>>
>> On 05/21/2018 04:44 PM, Dmitry Samersoff wrote:
>>> Hello Everybody,
>>>
>>> Please review small fix
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.01/
>>>
>>> CR:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8203481
>>>
>>> Testing:
>>>
>>> jfr tests that depends to safe_for_sender functionality
>>>
>>> ./jdk/jdk/jfr/api/consumer/TestRecordedFullStackTrace.java
>>> ./jdk/jdk/jfr/event/profiling/TestFullStackTrace.java
>>>
>>> fails on AARCH64.
>>>
>>> These tests passed after the fix.
>>>
>>>
>>
>


--
Dmitry Samersoff
http://devnull.samersoff.net
* There will come soft rains ...

Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Dmitry Samersoff-2
In reply to this post by Dmitry Samersoff-2
Hello Everybody,

Third version of the fix, changes limited to AArch64 only.

http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.03/

-Dmitry

On 05/21/2018 04:44 PM, Dmitry Samersoff wrote:

> Hello Everybody,
>
> Please review small fix
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.01/
>
> CR:
>
> https://bugs.openjdk.java.net/browse/JDK-8203481
>
> Testing:
>
> jfr tests that depends to safe_for_sender functionality
>
> ./jdk/jdk/jfr/api/consumer/TestRecordedFullStackTrace.java
> ./jdk/jdk/jfr/event/profiling/TestFullStackTrace.java
>
> fails on AARCH64.
>
> These tests passed after the fix.
>
>


--
Dmitry Samersoff
http://devnull.samersoff.net
* There will come soft rains ...

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

David Holmes
In reply to this post by Daniel D. Daugherty
On 15/06/2018 7:02 AM, Daniel D. Daugherty wrote:

> On 6/14/18 9:44 AM, Andrew Haley wrote:
>> On 06/10/2018 10:05 PM, David Holmes wrote:
>>> I have to admit I still can't connect all the pieces here. When we are
>>> operating in frame::safe_for_sender, what has set _unextended_sp? Can
>>> you walk through a complete example? I can't quite connect Andrew's
>>> example to the original problem.
>>>
>>> It still seems to me that the current frame should have:
>>>
>>> sp <= unextended_sp <= stack_base()
>> No.  We save SP, then remove unused stack, then call int a method.
>
> Where does this "remove unused stack" come from? And is that only
> an ARM thing? I don't remember seeing that on X64...

That's my confusion too. It seems to me this is being looked at from two
different perspectives. I'm assuming that if we start with a given SP
and then _add_ the extra stuff we get a new SP (logically an "extended
SP" and the original SP is saved as the unextended_sp - and hence SP
must be below unextended SP. But I have no real understanding of the
mechanics here I'm just going off the description of unextended_sp.

David

>
>> The address saved into the current method is the old SP, before stack
>> items were removed.  This is refereed to as the "unextended_sp" in th
>> eshared code, but it's actually *below* the SP when a method was
>> entered.
>
> I can see how removing things from the stack (and incrementing SP) could
> result in an SP value > saved SP value. It's the "removing things from
> the stack" part that has me confused right now...
>
> Also, we have a terminology problem in this thread. In this:
>
>      sp <= unextended_sp <= stack_base()
>
> I would say that 'sp' is below (or equal) to unextended_sp and that
> 'unextended_sp' is below (or equal) to stack_base(). To me "below"
> is like "less than" and "above" is like "greater than".
>
> Dan
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Andrew Haley
In reply to this post by Dmitry Samersoff-2
On 06/17/2018 06:44 PM, Dmitry Samersoff wrote:
> Third version of the fix, changes limited to AArch64 only.
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.03/

I don't understand what that comment means, so no-one coming to the
port has any hope.

When we are running interpreted code the machine stack pointer, SP, is
set low enough so that the Java expression stack can grow and shrink
without ever exceeding the machine stack bounds.  So, ESP >= SP.

When we call out of an interpreted method, SP is incremented so that
the space between SP and ESP is removed.  The SP saved in the callee's
frame is the SP *before* this increment.  So, when we walk a stack of
interpreter frames the sender's SP saved in a frame might be less than
the SP at the point of call.

I''m wondering if I should change this code just to save confusion,
but we're very close to ramp down.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Dmitry Samersoff-2
Andrew,

I'll copy your explanation of what is happening to comments.

Are you ok with the fix it self?

-Dmitry

On 18.06.2018 12:19, Andrew Haley wrote:

> On 06/17/2018 06:44 PM, Dmitry Samersoff wrote:
>> Third version of the fix, changes limited to AArch64 only.
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8203481/webrev.03/
>
> I don't understand what that comment means, so no-one coming to the
> port has any hope.
>
> When we are running interpreted code the machine stack pointer, SP, is
> set low enough so that the Java expression stack can grow and shrink
> without ever exceeding the machine stack bounds.  So, ESP >= SP.
>
> When we call out of an interpreted method, SP is incremented so that
> the space between SP and ESP is removed.  The SP saved in the callee's
> frame is the SP *before* this increment.  So, when we walk a stack of
> interpreter frames the sender's SP saved in a frame might be less than
> the SP at the point of call.
>
> I''m wondering if I should change this code just to save confusion,
> but we're very close to ramp down.
>


--
Dmitry Samersoff
http://devnull.samersoff.net
* There will come soft rains ...

Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Andrew Haley
On 06/18/2018 01:46 PM, Dmitry Samersoff wrote:
> I'll copy your explanation of what is happening to comments.
>
> Are you ok with the fix it self?

Sure.  For AArch64 only.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR(S): JDK-8203481 Incorrect constraint for unextended_sp in frame:safe_for_sender

Dmitry Samersoff-2
Comments changed in-place.

On 18.06.2018 15:49, Andrew Haley wrote:
> On 06/18/2018 01:46 PM, Dmitry Samersoff wrote:
>> I'll copy your explanation of what is happening to comments.
>>
>> Are you ok with the fix it self?
>
> Sure.  For AArch64 only.
>


--
Dmitry Samersoff
http://devnull.samersoff.net
* There will come soft rains ...

12