RFR: 8277926: [aarch64] Address constructors are lacking initialisation.

[Andrew Haley]

On Sun, 23 Jan 2022 15:54:35 GMT, Patric Hedlin <phedlin at openjdk.org> wrote:

> Please review changes to secure proper initialisation in Address constructors.
> 
> Changes include (with additional clean-up):
> 
> Renamed '**literal**' to '**addr_literal**'.
> Renamed '**_ext**' to '**_extend**'.
> Renamed Address::mode enum to Address::addr_mode.
> Renamed Address::getMode() to Address::mode().
> Reorder _mode.
> 
> Initialise '**_extend**' to **lsl(0)**.
> Initialise '**_target**' to **nullptr**.
> 
> Added preconditions to constructors.
> Added preconditions to check 'target' address.
> 
> Removed '**pcrel**' addressing mode (unused).
> Removed direct attribute use in Address.
> 
> Minor clean-up to RuntimeAddress use.
> 
> ---8<---
> 
> Testing: tier1-6 (Linux), tier1-3 (MacOSX)

In general, this is a large patch for a fairly simple problem. Some of the changes are good, some are neutral, and some are pointless. Or at least it seems to me they are pointless: perhaps there's some reason I'm missing.

You must distinguish between code that needs cleaning up because it's obscure or error prone, and code that simply wasn't written in the way that you would have written it. Also, it's hard to find substantive changes inside the rash of cleanups, and we don't want every minor change to be accompanied by cleanups that are ten times the size of the actual change. Apart from anything else, this makes it hard to see mistakes.

> Changes include (with additional clean-up):
> 
> Renamed '**literal**' to '**addr_literal**'. Renamed '**_ext**' to '**_extend**'. Renamed Address::mode enum to Address::addr_mode. Renamed Address::getMode() to Address::mode(). Reorder _mode.

Most of this doesn't seem to do anything very much, and the renamings will make future backports more likely to cause conflicts.

> Initialise '**_extend**' to **lsl(0)**. Initialise '**_target**' to **nullptr**.

Do we ever read these fields? Wouldn't this be an Undefined Behaviour bug if so?

If we're going to move to `nullptr` rather than `NULL` in the AArch64 back end, especially in the assembler, we should perhaps try to be consistent about it. I notice a couple of usages of `nullptr` have crept in already elsewhere. However, for the sake of people doing archaeology on the code, so many non-functional changes make life difficult.

> Added preconditions to constructors. Added preconditions to check 'target' address.

These look good.

> Removed '**pcrel**' addressing mode (unused). Removed direct attribute use in Address.

Removing unused `pcrel` makes sense. What change does this "direct attribute" refer to?

> Minor clean-up to RuntimeAddress use.

I don't believe these changes are cleaner: they're just different.

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

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