RFR: 8289046: Undefined Behaviour in x86 class Assembler

[Aleksey Shipilev]

On Thu, 23 Jun 2022 14:52:54 GMT, Andrew Haley <aph at openjdk.org> wrote:

> All instances of type Register exhibit UB in the form of wild pointer (including null pointer) dereferences. This isn't very hard to fix: we should make Registers pointers to something rather than aliases of small integers. 
> 
> Here's an example of what was happening:
> 
>    ` rax->encoding();`
> 
> Where rax is defined as `(Register *)0`.
> 
> This patch things so that rax is now defined as a pointer to the start of a static array of RegisterImpl.
> 
> 
> typedef const RegisterImpl* Register;
> extern RegisterImpl all_Registers[RegisterImpl::number_of_declared_registers + 1] ;
> inline constexpr Register RegisterImpl::first() { return all_Registers + 1; };
> inline constexpr Register as_Register(int encoding) { return RegisterImpl::first() + encoding; }
> constexpr Register rax = as_register(0);

Cursory review:

src/hotspot/cpu/x86/globalDefinitions_x86.hpp line 78:

> 76: #endif
> 77: 
> 78: #define USE_POINTERS_TO_REGISTER_IMPL_ARRAY

So, what's the use for this symbol? I see AArch64 code conditionalize definition macros, but this patch does not have such conditionalization. Should it?

src/hotspot/cpu/x86/register_x86.hpp line 70:

> 68:   // accessors
> 69:   int   raw_encoding() const                     { return this - first(); }
> 70:   int   encoding() const                         { assert(is_valid(), "invalid register"); return (this - first()); }

Suggestion:

  int   encoding() const                         { assert(is_valid(), "invalid register"); return raw_encoding(); }

src/hotspot/cpu/x86/register_x86.hpp line 76:

> 74: };
> 75: 
> 76: // The implementation of integer registers for the ia32 architecture

This comment seems unrelated, copy-paste error?

src/hotspot/cpu/x86/register_x86.hpp line 130:

> 128:   // accessors
> 129:   int   raw_encoding() const                      { return this - first(); }
> 130:   int   encoding() const                          { assert(is_valid(), "invalid register"); return this - first(); }

Suggestion:

  int   encoding() const                          { assert(is_valid(), "invalid register"); return raw_encoding(); }

src/hotspot/cpu/x86/register_x86.hpp line 171:

> 169:   // accessors
> 170:   int raw_encoding() const                       { return this - first(); }
> 171:   int   encoding() const                          { assert(is_valid(), "invalid register (%d)", (int)raw_encoding() ); return raw_encoding(); }

In the other cases, we don't do this kind of printing assert. Stick to one style? I think it is fine to have a shorter assert, because `!is_valid()` basically implies `noreg`?

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

Changes requested by shade (Reviewer).

PR: https://git.openjdk.org/jdk/pull/9261