RFR: 8340009: Improve the output from assert_different_registers

[David Holmes]

On Thu, 12 Sep 2024 12:56:13 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

> `assert_different_registers` is a mechanism we use to ensure that we don't use the same register in different variables. When the assert triggers it is not immediately clear where and why the assert failed.
> 
> For example, if I introduce an intentional violation:
> 
> diff --git a/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp b/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
> index fde868a64b3..551878ac09d 100644
> --- a/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
> +++ b/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
> @@ -1188,7 +1188,8 @@ void MacroAssembler::lookup_interface_method(Register recv_klass,
>                                               Register scan_temp,
>                                               Label& L_no_such_interface,
>                           bool return_method) {
> -  assert_different_registers(recv_klass, intf_klass, scan_temp);
> +  Register joker = intf_klass;
> +  assert_different_registers(recv_klass, intf_klass, scan_temp, joker);
>    assert_different_registers(method_result, intf_klass, scan_temp);
>    assert(recv_klass != method_result || !return_method,
>       "recv_klass can be destroyed when method isn't needed");
> 
> I get this error message:
> 
> #  Internal Error (src/hotspot/share/asm/register.hpp:287), pid=42568, tid=9731
> #  assert(!regs[i]->is_valid() || regs[i] != regs[j]) failed: Multiple uses of register: c_rarg0
> 
> The indicated file and line number refers to the `assert_different_registers` implementation and not the offending call site. More over, it's unclear from the assert which of the four variables contain the same register.
> 
> I'd like to propose a few changes:
> 1) That we report the indices of the conflicting registers
> 2) That we report the correct file and line number
> 3) That we hide the is_valid() check to lower the noise in the output. Not strictly necessary, but I think it looks nicer.
> 
> After these suggestions we'll get error messages that look like this:
> 
> #  Internal Error (src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp:1187), pid=59065, tid=8963
> #  assert(regs[i] != regs[j]) failed: regs[1] and regs[3] are both: c_rarg0
> 
> Which makes it easy to see that variables 1 and 3 are conflicting and by looking at the indicated file and line, it is clear that it is `intf_klass` and `joker` that are the offending variables.
> 
> There might be a way to use more macros to propagate the variable names, but I propose that we start with this incremental improvement.

Looks good. That isn't an assert I've had to deal with but this is certainly a huge improvement in its usability. :)

Thanks

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

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20965#pullrequestreview-2302115777