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

[Thomas Stuefe]

On Tue, 6 Oct 2020 23:50:22 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> This change moves the significant amount of stack overflow related code (with ascii art!) out of thread files into a
>> new file.  Many of the functions are static functions and some go through JavaThread::_stack_overflow_state where
>> needed.   All functions are moved and not modified except for qualification.  I also added a delegating constructor to
>> JavaThread::JavaThread so reordered the assignments as initializers from JavaThread::initialize.
>> Tested with tier1-6 and builds on arm32, ppc, s390 and zero.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix comments

Hi Coleen,

this is a nice cleanup. Not a full review yet.

Also some of my remarks are probably follow ups (if that), I leave that up to you how much you want to take from my
review.

Cheers, Thomas

src/hotspot/share/runtime/stackOverflow.hpp line 135:

> 133:     return _stack_red_zone_size;
> 134:   }
> 135:   static void set_stack_red_zone_size(size_t s) {

Could we merge these individual setters for set_xxx_zone_size() into one function:

static initialize_dimensions(size, size, ..)
?

Would make it clearer that these are not to be touched outside initialization.

That function also should live in the .cpp file.

src/hotspot/share/runtime/stackOverflow.hpp line 1:

> 1: /*

There are a number of newline issues throughout this file. Mostly code too tight (newlines missing between method
blocks) acc. to hotspot style guide.

src/hotspot/share/runtime/stackOverflow.hpp line 125:

> 123:   // These values are derived from flags StackRedPages, StackYellowPages,
> 124:   // StackReservedPages and StackShadowPages. The zone size is determined
> 125:   // ergonomically if page_size > 4K.

I think the last sentence is superfluous (the first says it all) and slightly wrong too.

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.

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

src/hotspot/share/runtime/stackOverflow.hpp line 121:

> 119:   //  (large addresses)
> 120:   //
> 121:

Nice ascii art :)

I wish though it could communicate better the openness of the ranges. E.g.:

+----------------+
|                |   <-- stack_end()
| red zone       |
|                |
+----------------+
|                |   <-- red_zone_base()
| yellow zone    |
|                |
     ....
|                |
+----------------+
                     <-- stack_base()

Maybe its just me but I always have to think a bit more here. With downward growing stacks normal range thinking is
reversed wrt to openness, so stack_base() points outside the stack and stack_end() is in the stack. This is true for
all base values - they point to locations outside the zone they base.

Maybe that is clear to all others but it sometimes surprises me.

src/hotspot/share/runtime/stackOverflow.hpp line 176:

> 174:     return (a <= stack_reserved_zone_base()) &&
> 175:            (a >= (address)((intptr_t)stack_reserved_zone_base() - stack_reserved_zone_size()));
> 176:   }

Same here, a==reserved_zone_base is strictly speaking outside the reserved zone.

src/hotspot/share/runtime/stackOverflow.hpp line 141:

> 139:     _stack_red_zone_size = s;
> 140:   }
> 141:   address stack_red_zone_base() {

could be const (as could a couple of others, I won't mark them individually)

src/hotspot/share/runtime/stackOverflow.hpp line 131:

> 129:   static size_t _stack_shadow_zone_size;
> 130:  public:
> 131:   static size_t stack_red_zone_size() {

Naming: since you moved these formerly-Thread-methods into this enclosing class all the "_stack" and "stack" prefixes
for members and methods are not needed anymore.

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.

src/hotspot/share/runtime/stackOverflow.hpp line 79:

> 77:   // Stack overflow support
> 78:   //
> 79:   //  (small addresses)

s/small/low ?

src/hotspot/share/runtime/stackOverflow.hpp line 119:

> 117:   //  --  <-- stack_base()
> 118:   //
> 119:   //  (large addresses)

s/large/high ?

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 ?

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?

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

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