RFR: 8221639: [x32] expand_exec_shield_cs_limit workaround is undefined code after JDK-8199717

[David Holmes]

On 26/04/2019 7:15 pm, Severin Gehwolf wrote:
> Hi David,
> 
> Can I consider webrev 02 reviewed by yourself?

Of course. Thanks for adding the comment.

David

> Thanks,
> Severin
> 
> On Wed, 2019-04-24 at 15:23 +0200, Severin Gehwolf wrote:
>> Hi David,
>>
>> On Wed, 2019-04-24 at 19:13 +1000, David Holmes wrote:
>>> Hi Severin,
>>>
>>> Sorry for the delay, this slipped off my rader after the break.
>>
>> No worries.
>>
>>> On 19/04/2019 12:02 am, David Holmes wrote:
>>>> Hi Severin,
>>>>
>>>> I'll have to delve into this more deeply (but I'm on Easter break for
>>>> the next 4 days). I don't recall ever being aware of
>>>> "suppress_primordial_thread_resolution"!
>>>
>>> Well I have a bad memory as I reviewed that particular optimisation.
>>>
>>> So, the exec_shield workaround requires knowing the process's primordial
>>> thread stack regardless of whether the JVM is loaded on it or not. So it
>>> assumes the initial_thread_stack has been determined, while we now avoid
>>> doing that unnecessarily.
>>>
>>> Okay the suggested fix is okay to achieve that - though alternatively it
>>> could be handled inside workaround_expand_exec_shield_cs_limit() via:
>>>
>>>       // Need to ensure we've determined the process's initial stack to
>>>       // perform the workaround
>>>       if (suppress_primordial_thread_resolution) {
>>>         Linux::capture_initial_stack(JavaThread::stack_size_at_create());
>>>       }
>>>
>>> I would like to see a comment like the above regardless of where you
>>> make the change.
>>
>> Thanks for the review! Updated webrev with the comment:
>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8221639/02/webrev/
>>
>> Would this be a trivial change or do I need a second reviewer? FWIW,
>> jdk/submit testing came back clean with this.
>>
>> Thanks,
>> Severin
>>
>>> Thanks,
>>> David
>>> ------
>>>
>>>> David
>>>>
>>>> On 18/04/2019 11:41 pm, Severin Gehwolf wrote:
>>>>> Hi,
>>>>>
>>>>> Could I please get reviews for this Linux x32 fix? JDK-8199717 added a
>>>>> performance optimization to only capture the initial stack size when
>>>>> launched via non-java launchers. However, on Linux x32, there is old
>>>>> code being executed to work around the exec shield CS limit. That code
>>>>> depends on the initial stack size being captured. Right now this is
>>>>> undefined code: Pointer artithmetic on NULL pointer.
>>>>>
>>>>> src/hotspot/os_cpu/linux_x86/os_linux_x86.cpp:884:73: runtime error:
>>>>> pointer index expression with base 0x00000000 overflowed to 0xffffb000
>>>>>
>>>>> I propose to not perform the optimization of JDK-8199717 for Linux x32.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8221639
>>>>> webrev:
>>>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8221639/01/webrev/
>>>>>
>>>>> Testing: release/fastdebug builds on x32 Linux, inspecting
>>>>> -Xlog:os=info messages, currently running through jdkd/submit. Tier 1
>>>>> tests on Linux x86_64
>>>>>
>>>>> Thoughts?
>>>>>
>>>>> Thanks,
>>>>> Severin
>>>>>
>