Fix undefined behavior in PeepholeOptimizer.

[linzj]

Summary

Caused by optimizeExtInstr function uses uninitialized SubIdx.

[void]

I think SubIdx is supposed to be set by TII->isCoalescableExtInstr(). If it's not doing that, then the error's probably in that call and not in this function.

[hans]

A test case that shows the problem would also be good.

[linzj]

I think SubIdx is supposed to be set by TII->isCoalescableExtInstr(). If it's not doing that, then the error's probably in that call and not in this function.

Then most target instr infos need to patch.

A test case that shows the problem would also be good.

valgrind shows me this bug. I don't think any testcase is able to reproduce an expected value in this circumstance.

[void]

In D81632#2092363, @linzj wrote:

I think SubIdx is supposed to be set by TII->isCoalescableExtInstr(). If it's not doing that, then the error's probably in that call and not in this function.

Then most target instr infos need to patch.

A test case that shows the problem would also be good.

valgrind shows me this bug. I don't think any testcase is able to reproduce an expected value in this circumstance.

Which platform did you run valgrind on? The x86 platform should set SubIdx on all paths through it...

[linzj]

Which platform did you run valgrind on? The x86 platform should set SubIdx on all paths through it...

ARM. armv8-linux-android.

[void]

In D81632#2094672, @linzj wrote:

Which platform did you run valgrind on? The x86 platform should set SubIdx on all paths through it...

ARM. armv8-linux-android.

Strange. SubIdx is initialized on all paths that return true in that function. So I'm not sure why valgrind complained...

My one worry with the change in this patch is that 0 is ostensibly a valid index, right? So how do we distinguish it from an invalid one if isCoalescableExtInstr returns true without properly initializing SubIdx?

bool AArch64InstrInfo::isCoalescableExtInstr(const MachineInstr &MI,
                                             Register &SrcReg, Register &DstReg,
                                             unsigned &SubIdx) const {
  switch (MI.getOpcode()) {
  default:
    return false;
  case AArch64::SBFMXri: // aka sxtw
  case AArch64::UBFMXri: // aka uxtw
    // Check for the 32 -> 64 bit extension case, these instructions can do
    // much more.
    if (MI.getOperand(2).getImm() != 0 || MI.getOperand(3).getImm() != 31)
      return false;
    // This is a signed or unsigned 32 -> 64 bit extension.
    SrcReg = MI.getOperand(1).getReg();
    DstReg = MI.getOperand(0).getReg();
    SubIdx = AArch64::sub_32;
    return true;
  }
}

[arsenm]

In D81632#2096458, @void wrote:

In D81632#2094672, @linzj wrote:

Which platform did you run valgrind on? The x86 platform should set SubIdx on all paths through it...

ARM. armv8-linux-android.

Strange. SubIdx is initialized on all paths that return true in that function. So I'm not sure why valgrind complained...

My one worry with the change in this patch is that 0 is ostensibly a valid index, right? So how do we distinguish it from an invalid one if isCoalescableExtInstr returns true without properly initializing SubIdx?

No, NoSubRegister is defined as 0

[MatzeB]

Back to author. We should fix the relevant isCoalescableExtInstr in the targets instead of just default initializing the field. That said all implementations of that callback on open-source trunk look fine to me too.

[MatzeB]

In D81632#2092363, @linzj wrote:

I think SubIdx is supposed to be set by TII->isCoalescableExtInstr(). If it's not doing that, then the error's probably in that call and not in this function.

Then most target instr infos need to patch.

SubIdx only needs to be set the function ends up doing return true;.

llvm/lib/CodeGen/PeepholeOptimizer.cpp

 //===- PeepholeOptimizer.cpp - Peephole Optimizations ---------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //

 ///
 /// Do not generate an EXTRACT that is used only in a debug use, as this changes
 /// the code. Since this code does not currently share EXTRACTs, just ignore all
 /// debug uses.
 bool PeepholeOptimizer::
 optimizeExtInstr(MachineInstr &MI, MachineBasicBlock &MBB,
                  SmallPtrSetImpl<MachineInstr*> &LocalMIs) {
   Register SrcReg, DstReg;
-  unsigned SubIdx;
+  unsigned SubIdx = 0;
   if (!TII->isCoalescableExtInstr(MI, SrcReg, DstReg, SubIdx))
     return false;
 
   if (DstReg.isPhysical() || SrcReg.isPhysical())
     return false;
 
   if (MRI->hasOneNonDBGUse(SrcReg))
     // No other uses.