RFR: 8340009: Improve the output from assert_different_registers

[Stefan Karlsson]

On Thu, 12 Sep 2024 13:03:15 GMT, Axel Boldt-Christmas <aboldtch 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 incre...
>
> A nice improvement.

Thanks for reviewing! @xmas92 has a prototype that prints the "actual" source names, but it uses large macro expansions, so I'm not sure we would take that in. Maybe he can show it as a curiosity. :)

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

PR Comment: https://git.openjdk.org/jdk/pull/20965#issuecomment-2348887162