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

[Andrew Dinn]

On Wed, 17 Mar 2021 11:21:31 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> 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.

The lack of exercise for this specific branch appears to be a fortuitous /contingency/.

Currently, generate_address is only called /directly/ with small values for disp e.g.

    src/hotspot/share/gc/shared/c1/barrierSetC1.cpp:    addr_opr = LIR_OprFact::address(gen->generate_address(base.result(), offset, 0, 0, access.type()));
    src/hotspot/share/c1/c1_LIRGenerator.cpp:    addr = generate_address(base_op, index_op, log2_scale, 0, dst_type);
    . . .

Also, many of those uses requires GENERATE_ADDRESS_IS_PREFERRED to be defined on tghe compile line.

generate_address is also called indirectly from LIRGenerator::cmp_mem_int and LIRGenerator::cmp_reg_mem. These are only used in generic C1 code to plant range checks for arrays and nio buffers so, once again, the displacement is a known small constant.

However, none of that guarantees that future uses will be guaranteed to employ such small constant displacements. I suggest we push this patch just in case someone decides to use it with a larger displacement. An assert might otherwise be in order.

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

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