RFR: 8206454: [8u] os::current_stack_pointer() fails to compile on later Windows compilers (warning C4172: returning address of local variable)
David Holmes
david.holmes at oracle.com
Mon Jul 9 11:19:57 UTC 2018
Hi Kevin,
On 9/07/2018 9:00 PM, Kevin Walls wrote:
>
> Hi David, sure no problem we can do the "default" after the code we want
> to affect here.
Okay if that works then that seems fine.
Thanks,
David
> I only wanted to put the #pragma within the #if for compiler level, not
> to change what the older compiler does. (I have (now) run the existing
> VS2010 build always doing the #pragma, and also using the push/pop, and
> they are all OK too. I'd still like to go with with the condition and
> not change anything about existing builds.)
>
> Updated diff below.
>
> Thanks!
> Kevin
>
> bash-4.2$ hg diff
> diff -r ad057f2e3211 src/os_cpu/windows_x86/vm/os_windows_x86.cpp
> --- a/src/os_cpu/windows_x86/vm/os_windows_x86.cpp Wed Jul 04
> 03:02:43 2018 -0400
> +++ b/src/os_cpu/windows_x86/vm/os_windows_x86.cpp Mon Jul 09
> 01:10:18 2018 -0700
> @@ -454,11 +454,18 @@
> // 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(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(default: 4172)
> +#endif
> #else
> // Returns the current stack pointer. Accurate value needed for
> // os::verify_stack_alignment().
>
>
>
>
> On 09/07/2018 03:38, David Holmes wrote:
>> 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().
>>>>
>>>
>
More information about the hotspot-dev
mailing list