Page MenuHomePhabricator

Fix undefined behavior in PeepholeOptimizer.
Needs ReviewPublic

Authored by linzj on Jun 10 2020, 11:39 PM.

Details

Reviewers
MatzeB
hans
void
Summary

Caused by optimizeExtInstr function uses uninitialized SubIdx.

Diff Detail

Event Timeline

linzj created this revision.Jun 10 2020, 11:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2020, 11:39 PM
void added a comment.Jun 12 2020, 12:51 PM

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 added a comment.Jun 15 2020, 1:30 AM

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

linzj added a comment.Jun 15 2020, 2:22 AM

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 added a comment.Jun 15 2020, 7:03 PM

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 added a comment.Jun 15 2020, 8:18 PM

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

ARM. armv8-linux-android.

void added a comment.Jun 16 2020, 12:14 PM

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 added a subscriber: arsenm.Jun 16 2020, 12:42 PM

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