RFR: 8253717: Relocate stack overflow code out of thread.hpp/cpp [v3]

[Coleen Phillimore]

On Wed, 7 Oct 2020 06:20:37 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fix comments
>
> src/hotspot/share/runtime/stackOverflow.hpp line 145:
> 
>> 143:   }
>> 144:   bool in_stack_red_zone(address a) {
>> 145:     return a <= stack_red_zone_base() && a >= stack_end();
> 
> Note that this method conflicts with
> 
> in_stack_yellow_reserved_zone()
> 
> since both in_stack_yellow_reserved_zone() and is_stack_red_zone() return true for a=red zone base.
> 
> Strictly speaking it is wrong here since red zone base is not in red zone.

I moved this code and didn't change the zone calculations.   The code that calls this is in the signal handler and is
irritatingly similar for all platforms.  I'll file an RFE to consolidate this and check that this boundary makes
sense.  I'm not going to change this here.  https://bugs.openjdk.java.net/browse/JDK-8254158

> src/hotspot/share/runtime/stackOverflow.hpp line 181:
> 
>> 179:     return _stack_yellow_zone_size + _stack_reserved_zone_size;
>> 180:   }
>> 181:   bool in_stack_yellow_reserved_zone(address a) {
> 
> I always have to look what this actually does since there is no "yellow reserved" zone. A slight rename would help,
> e.g.: "is_stack_yellow_or_reserved_zone()".

I'm not going to rename this in this change.

> src/hotspot/share/runtime/stackOverflow.hpp line 182:
> 
>> 180:   }
>> 181:   bool in_stack_yellow_reserved_zone(address a) {
>> 182:     return (a <= stack_reserved_zone_base()) && (a >= stack_red_zone_base());
> 
> Strictly speaking stack_reserved_zone_base is outside the reserved zone.

We'll have to work on these ranges in a future change.

> src/hotspot/share/runtime/stackOverflow.hpp line 79:
> 
>> 77:   // Stack overflow support
>> 78:   //
>> 79:   //  (small addresses)
> 
> s/small/low ?

Fixed.

> src/hotspot/share/runtime/stackOverflow.hpp line 119:
> 
>> 117:   //  --  <-- stack_base()
>> 118:   //
>> 119:   //  (large addresses)
> 
> s/large/high ?

fixed.

> src/hotspot/share/runtime/stackOverflow.hpp line 95:
> 
>> 93:   //  |                                      |
>> 94:   //  |  reserved pages                      |
>> 95:   //  |                                      |
> 
> Can we use use "zone" instead of "pages" since that term is used e.g. in https://openjdk.java.net/jeps/270 ?

Sure, I changed this to zone.

> src/hotspot/share/runtime/stackOverflow.hpp line 75:
> 
>> 73:
>> 74:   address stack_end()  const           { return _stack_end; }
>> 75:   address stack_base() const           { assert(_stack_base != nullptr,"Sanity check"); return _stack_base; }
> 
> We now keep stack  base and stack size/end both here and in Thread. Could we merge this and use this as a data holder
> for Thread?

I thought about doing this but I'd rather carry the information than a pointer back to Thread.  It's only one extra
word but doesn't invite abuse.

-------------

PR: https://git.openjdk.java.net/jdk/pull/522