RFR: 8224851: AArch64: fix warnings and errors with Clang and GCC 8.3

[Nick Gasson]

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).
>