Hi Andrew,
Sorry for the delay, I've put an updated webrev here:
http://cr.openjdk.java.net/~ngasson/8224851/webrev.1/
Changes since the last patch:
* Replaced ~((1<<12) - 1) with -1u<<12
* Use __atomic_add_fetch in Atomic::PlatformAdd #ifdef __clang__
* Use __builtin_frame_address(0) in os::current_frame()
* Use placement new / copy assignment in pf()
I also replaced the call to _get_previous_fp() in os::current_frame() with
*(intptr_t **)__builtin_frame_address(0);
As it generates the same code and avoids the `register intptr_t **fp
__asm__ (SPELL_REG_FP);' declaration which clang doesn't support. Also
the following comment in _get_previous_fp seems to be wrong:
// fp is for this frame (_get_previous_fp). We want the fp for the
// caller of os::current_frame*(), so go up two frames. However, for
// optimized builds, _get_previous_fp() will be inlined, so only go
// up 1 frame in that case.
#ifdef _NMT_NOINLINE_
return **(intptr_t***)fp;
#else
return *fp;
#endif
Even on -O0 gcc won't generate a stack frame for this function so if
_NMT_NOINLINE_ is defined you get the caller's caller's FP rather than
the caller's FP. I just deleted the function as it's not used anywhere else.
BTW we can't use __builtin_frame_address(1) as GCC gives a warning when
this is called with a non-zero argument (-Wframe-address).
Tested jtreg hotspot_all_no_apps and jdk_core (gcc 7.3).
Thanks,
Nick
On 03/06/2019 19:03, Andrew Haley wrote:
Hi,
On 6/3/19 11:36 AM, Nick Gasson wrote:
We need to know what it's used for to know which solution is right. I'm
guessing the primary use case is asynchronous runtime stack probes, used
by debugging tools.
Yes, we also have os::stack_shadow_pages_available() which uses it to
calculate how much space there is between the current SP and the end of
the stack. In this case I think it's ok as long as we don't return a
value that's *above* the actual stack pointer. And os::current_frame(),
which constructs a `frame' object using the FP of the caller, but the SP
will point into the frame of os::current_frame(), so it seems it's
already inaccurate.
Eww.
There's also a comment in os_linux.cpp that says "Don't use
os::current_stack_pointer(), as its result can be slightly below current
stack pointer". So I'm wondering if we can simplify this a lot and use
__builtin_frame_address(0) which will give us the FP in
os::current_stack_pointer (ought to be the caller's SP - 16). Zero does
this currently. And maybe we can replace _get_previous_fp() with
__builtin_frame_address(1)?
Let's make os::current_stack_pointer() noinline, then make it return
__builtin_frame_address(0).
