dead compiler code found during the JDK-8272058 code review

During the code review for the following fix:

    JDK-8272058 25 Null pointer dereference defect groups in 4 files

some dead compiler code was found.

Details are found here:

https://github.com/openjdk/jdk18/pull/51#discussion_r774922087

Here's a paste of those comments:

 phedlin 22 hours ago •

I believe this code is dead, in both MacroAssembler::pd_patch_instruction() and MacroAssembler::target_addr_for_insn(), since the poll read is PIC, we are not patching and should not need to acquire any target to update.

The associated fix_relocation_after_move() will not invoke any of the above on the poll read since the maybe_cpool_ref() condition is used:

void poll_Relocation::fix_relocation_after_move(const CodeBuffer* src, CodeBuffer* dest) {
  if (NativeInstruction::maybe_cpool_ref(addr())) {
    address old_addr = old_addr_for(addr(), src, dest);
    MacroAssembler::pd_patch_instruction(addr(), MacroAssembler::target_addr_for_insn(old_addr));
  }
}

Should be sufficient to simply remove these cases, possibly you could move an assert into the "unreachable" branch (for "precision").

} else {
  assert(!NativeInstruction::is_ldrw_to_zr(<addr>), "Unexpected poll read");
  ShouldNotReachHere();
}

(The code above in fix_relocation_after_move() might also be dead...)

Chip in @theRealAph ?


Member Author
@dcubed-ojdk dcubed-ojdk 20 hours ago

@phedlin - Thanks for chiming in on this review thread.


Member
@bulasevich bulasevich 8 hours ago

I wondered if target_addr_for_insn ever gets the ldrw_to_zr instruction.
Anyway, I am Ok to move it to target_addr_for_insn_allow_null_ret (target_addr_for_insn_or_null?).
Thank you.


Member
@theRealAph theRealAph 7 hours ago

Since we moved to using per-thread safepoints, I don't think we'll ever see a reloc against a safepoint poll. I've just looked through the AArch64 back end, and as far as I can see the old safepoint code is gone.


Member
@theRealAph theRealAph 7 hours ago

    @bulasevich - I've made another pass through the code and checked the calls to MacroAssembler::target_addr_for_insn() and I don't see any of those calls in areas related to safepoint polling. From the other POV, I took a look at the aarch64 safepoint polling code and I don't see a way into MacroAssembler::target_addr_for_insn() .

    It is entirely possible that I'm missing something here and would appreciate it if you could sanity check my conclusions here.

I think you're right. As far as I can see, this code is dead since
8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing

which took out the code that generated safepoint relocs, but not the code here that relocated them.