RFR: 8307423: [s390x] Represent Registers as values

Martin Doerr mdoerr at openjdk.org
Thu May 4 19:56:18 UTC 2023


On Thu, 4 May 2023 15:08:57 GMT, Amit Kumar <amitkumar at openjdk.org> wrote:

> The recent Register implementation uses wild pointer (including null pointer) dereferences which exhibit undefined behavior. We should migrate away from pointer-based representation of Register values as it was done for x86 ([JDK-8292153](https://bugs.openjdk.org/browse/JDK-8292153)). Problems exist when trying to build with GCC 11.3.0 ([JDK-8307093](https://bugs.openjdk.org/browse/JDK-8307093)).
> 
> Tested `fastdebug, slowdebug, optimized, release build` , `tier1-test` on fastdebug build and build with GCC-9.5.0 as well.

LGTM. Please consider my minor suggestions.

src/hotspot/cpu/s390/assembler_s390.hpp line 196:

> 194:     _index(index),
> 195:     _disp(disp) {}
> 196: 

I can live with the removal, but I guess it may be useful at some point of time. s390 supports specifying both, index and disp (unlike PPC64).

src/hotspot/cpu/s390/register_s390.hpp line 186:

> 184: 
> 185:   // tester
> 186:   constexpr bool  is_valid()      const { return 0 <= _encoding && _encoding < number_of_registers; }

Indentation is different than for the other register types. I suggest to adapt it, here.

src/hotspot/cpu/s390/register_s390.hpp line 433:

> 431: 
> 432: // Temporary registers to be used within frame manager. We can use
> 433: // the nonvolatile because the call stub has saved them.

Better: "nonvolatile ones"

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

Marked as reviewed by mdoerr (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13805#pullrequestreview-1413798190
PR Review Comment: https://git.openjdk.org/jdk/pull/13805#discussion_r1185448158
PR Review Comment: https://git.openjdk.org/jdk/pull/13805#discussion_r1185450644
PR Review Comment: https://git.openjdk.org/jdk/pull/13805#discussion_r1185443064


More information about the hotspot-dev mailing list