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

[Kevin Walls]

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