RFR: 8276563: Undefined Behaviour in class Assembler [v3]

[Andrew Haley]

On Sat, 13 Nov 2021 07:47:18 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Andrew Haley has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Re-establish the FloatRegister::successor() hack.
>
> src/hotspot/cpu/aarch64/register_aarch64.cpp line 56:
> 
>> 54: 
>> 55: const char* FloatRegisterImpl::name() const {
>> 56:   static const char *const names[number_of_registers] = {
> 
> While reading this code I noticed that this method is sensitive to changes to is_valid, therefore care has to be taken when changing its semantics. Or, maybe just extend the array to number_of_declared_registers and add strings for ZR and SP, just to be safe.
> 
> Update: my comment is in the wrong place, I meant to comment RegisterImpl::name().

Great catch! I'll have a look at how this function is used. It's a little bit awkward that there is not a simple 1-to-1 mapping between register numbers and register names.

> src/hotspot/cpu/aarch64/register_aarch64.hpp line 44:
> 
>> 42:   inline friend const Register as_Register(int encoding);
>> 43: 
>> 44: private:
> 
> nit: private not needed

OK.

> src/hotspot/cpu/aarch64/register_aarch64.hpp line 63:
> 
>> 61:   // accessors
>> 62:   int encoding() const             { assert(is_valid(), "invalid register"); return encoding_nocheck(); }
>> 63:   bool is_valid() const            { return this >= first() && this < invalid(); }
> 
> is_valid() now returns true for ZR, SP. Was this intended? This affects other functions too, e.g. `RegisterImpl::name()`.

I think so, yes. SP and ZR are valid registers. `is_valid()` should only return `false` for `noreg`.

> src/hotspot/cpu/aarch64/register_aarch64.hpp line 64:
> 
>> 62:   int encoding() const             { assert(is_valid(), "invalid register"); return encoding_nocheck(); }
>> 63:   bool is_valid() const            { return this >= first() && this < invalid(); }
>> 64:   bool has_byte_register() const   { return is_valid(); }
> 
> Same here, semantics changed to include 32 and 33

I guess it would be "safer" to go back to excluding SP and ZR, but it was never intentional to exclude them. I'll go through all the uses of is_valid() to see which is best.

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

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