RFR: 8263676: AArch64: one potential bug in C1 LIRGenerator::generate_address()

[Andrew Haley]

On Wed, 17 Mar 2021 10:09:07 GMT, Andrew Dinn <adinn at openjdk.org> wrote:

>> Noticed this issue when I am trying to backport: https://bugs.openjdk.java.net/browse/JDK-8263425
>> 
>> Around line 180 we have:
>> 
>>          __ add(index, LIR_OprFact::intptrConst(large_disp), tmp);
>>          index = tmp;
>>        } else {
>>          __ move(tmp, LIR_OprFact::intptrConst(large_disp));      <========
>>          __ add(tmp, index, tmp);
>>          index = tmp;
>>        }
>> 
>> This is supposed to be calculating "tmp = large_disp" but it actually does "large_disp = tmp".
>> Looks like this is missed by JDK-8263425.
>> Tested tier1  with -XX:TieredStopAtLevel=1 on AArch64 Linux.
>
> Nice catch!

I think we're not being curious enough. It doesn't make sense for anyone to check in code they can't test, and from what I understand this code path can never have been executed.

The mistake seems to date from the very early days of the AArch64-port project. The logic seems to have been based on this code from the SPARC port:
  if (index->is_register()) {
    // apply the shift and accumulate the displacement
    if (shift > 0) {
      LIR_Opr tmp = new_pointer_register();
      __ shift_left(index, shift, tmp);
      index = tmp;
    }
    if (disp != 0) {
      LIR_Opr tmp = new_pointer_register();
      if (Assembler::is_simm13(disp)) {
        __ add(tmp, LIR_OprFact::intptrConst(disp), tmp);
        index = tmp;
      } else {
        __ move(LIR_OprFact::intptrConst(disp), tmp);
        __ add(tmp, index, tmp);
        index = tmp;
      }
      disp = 0;
    }
  } else if (disp != 0 && !Assembler::is_simm13(disp)) {
    // index is illegal so replace it with the displacement loaded into a register
    index = new_pointer_register();
    __ move(LIR_OprFact::intptrConst(disp), index);
    disp = 0;
  }


So the mistake is pretty obvious, but does it make any sense to correct some code that has never been executed without finding out  _why_ it is never executed? We shouldn't check in any changes we don't test.

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

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