RFR: 8206454: [8u] os::current_stack_pointer() fails to compile on later Windows compilers (warning C4172: returning address of local variable)

[David Holmes]

Hi Kevin,

I note we currently never use push/pop on Windows, rather we use the 
pattern:

#pragma warning( disable : NNN )
<code>
#pragma warning( default : NNN )

Do older 8u compilers support the push/pop pragma warning capability?

Thanks,
David

On 7/07/2018 5:22 PM, Kevin Walls wrote:
> No problem!  Thanks for bringing this up for clarification.
> 
> 
> On 06/07/2018 23:32, Kim Barrett wrote:
>>> On Jul 6, 2018, at 4:37 PM, Kevin Walls <kevin.walls at oracle.com> wrote:
>>>
>>> Hi Kim -
>>>
>>> Good question. 8-)  Yes the same code is there in the latest version 
>>> when I looked, but address os::current_stack_pointer() is within an 
>>> #ifndef AMD64, so I suspect have we never built 32-bit with the later 
>>> VS2017 compiler…
>> Oh, you are right.  I mis-read that as #ifdef AMD64.
>>
>> Yeah, we (Oracle) don’t seem to be doing 32bit Windows builds 
>> anymore.  Presumably
>> nobody else is doing so with recent versions of VS and the JDK, else 
>> the problem would
>> have already been known (and probably dealt with).  Since I’m not a 
>> big fan of adding
>> untested code, I’m okay with just addressing this in jdk8u, and 
>> leaving the current code
>> alone until someone complains about it.
>>
>> Unfortunately, while this change looks okay to me, I’m not even a 
>> committer for jdk8,
>> so you still need a Reviewer.
>>
>>> More file context around the 8u suggestion below..
>>>
>>> Thanks
>>> Kevin
>>>
>>> src/os_cpu/windows_x86/vm/os_windows_x86.cpp
>>>
>>>     452
>>>     453  #ifndef AMD64
>>>     454  // Returns an estimate of the current stack pointer. Result 
>>> must be guaranteed
>>>     455  // to point into the calling threads stack, and be no lower 
>>> than the current
>>>     456  // stack pointer.
>>>     457  #if defined(_MSC_VER) && _MSC_VER >= 1900
>>>     458  // warning C4172: returning address of local variable or 
>>> temporary: dummy
>>>     459  #pragma warning( push )
>>>     460  #pragma warning( disable: 4172 )
>>>     461  #endif
>>>     462  address os::current_stack_pointer() {
>>>     463    int dummy;
>>>     464    address sp = (address)&dummy;
>>>     465    return sp;
>>>     466  }
>>>     467  #if defined(_MSC_VER) && _MSC_VER >= 1900
>>>     468  #pragma warning( pop )
>>>     469  #endif
>>>     470  #else
>>>     471  // Returns the current stack pointer. Accurate value needed for
>>>     472  // os::verify_stack_alignment().
>>>     473  address os::current_stack_pointer() {
>>>     474    typedef address get_sp_func();
>>>     475    get_sp_func* func = CAST_TO_FN_PTR(get_sp_func*,
>>>     476 StubRoutines::x86::get_previous_sp_entry());
>>>     477    return (*func)();
>>>     478  }
>>>     479  #endif
>>>     480
>>>
>>>
>>>
>>>
>>> On 06/07/2018 21:09, Kim Barrett wrote:
>>>>> On Jul 6, 2018, at 5:54 AM, Kevin Walls <kevin.walls at oracle.com> 
>>>>> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> I'd like to get a review of this change for 8u:
>>>>>
>>>>> 8206454: [8u] os::current_stack_pointer() fails to compile on later 
>>>>> Windows compilers (warning C4172: returning address of local variable)
>>>>> https://bugs.openjdk.java.net/browse/JDK-8206454
>>>>>
>>>>> This is part of getting jdk8u to accept later Windows compilers. 
>>>>> Not changing the code, but adding a conditional  #pragma to let the 
>>>>> compiler accept the code.
>>>> Why is this only being done for jdk8u, and why isn’t it a problem in 
>>>> (say) jdk11 or jdk12?
>>>> The code being patched below is still the same.
>>>>
>>>>> I'll include a diff in text below (can do a webrev if anybody would 
>>>>> like one...).
>>>>>
>>>>> Many thanks!
>>>>> Kevin
>>>>>
>>>>>
>>>>> bash-4.2$ hg diff src/os_cpu/windows_x86/vm/os_windows_x86.cpp
>>>>> diff -r 5792d995ed26 src/os_cpu/windows_x86/vm/os_windows_x86.cpp
>>>>> --- a/src/os_cpu/windows_x86/vm/os_windows_x86.cpp      Wed Jun 27 
>>>>> 03:04:33 2018 -0700
>>>>> +++ b/src/os_cpu/windows_x86/vm/os_windows_x86.cpp      Fri Jul 06 
>>>>> 02:46:48 2018 -0700
>>>>> @@ -454,11 +454,19 @@
>>>>>   // Returns an estimate of the current stack pointer. Result must 
>>>>> be guaranteed
>>>>>   // to point into the calling threads stack, and be no lower than 
>>>>> the current
>>>>>   // stack pointer.
>>>>> +#if defined(_MSC_VER) && _MSC_VER >= 1900
>>>>> +// warning C4172: returning address of local variable or 
>>>>> temporary: dummy
>>>>> +#pragma warning( push )
>>>>> +#pragma warning( disable: 4172 )
>>>>> +#endif
>>>>>   address os::current_stack_pointer() {
>>>>>     int dummy;
>>>>>     address sp = (address)&dummy;
>>>>>     return sp;
>>>>>   }
>>>>> +#if defined(_MSC_VER) && _MSC_VER >= 1900
>>>>> +#pragma warning( pop )
>>>>> +#endif
>>>>>   #else
>>>>>   // Returns the current stack pointer. Accurate value needed for
>>>>>   // os::verify_stack_alignment().
>>
>