[RISCV] Add bounds check before use on returned iterator during frame lowering.

[anmolparalkar-nxp]

Summary

Check iterator validity before use; fixes a crash seen in RISCVFrameLowering
when executing the Prologue/Epilogue Insertion & Frame Finalization pass.
Note: -mattr=+zcmp is essential to the reproducibility of the issue.

[craig.topper]

Is this the same as D158256?

[anmolparalkar-nxp]

In D158379#4602315, @craig.topper wrote:

Is this the same as D158256?

Oh, Vow, thank you! Yes, the change to llvm/lib/Target/RISCV/RISCVFrameLowering.cpp is identical indeed.
The test case, is derived using bugpoint, on an internal benchmark.

[wangpc]

In D158379#4602317, @anmolparalkar-nxp wrote:

In D158379#4602315, @craig.topper wrote:

Is this the same as D158256?

Oh, Vow, thank you! Yes, the change to llvm/lib/Target/RISCV/RISCVFrameLowering.cpp is identical indeed.
The test case, is derived using bugpoint, on an internal benchmark.

Can we merge these two patches into one? You can add tests in this patch to D158256 and close this one.

[anmolparalkar-nxp]

Merged in test from D158256

PS: @wangpc Phabricator was not allowing me to update D158256 so I updated the patch here;
@garvitgupta08 please do upload this re-spin patch into D158256 so that the frame-lowering-crash.ll test gets merged in.

I am closing D158379.

[anmolparalkar-nxp]

@garvitgupta08 kindly upload current diff to D158256, thank you.

[anmolparalkar-nxp]

@garvitgupta08

Addressed review comments to llvm/test/CodeGen/RISCV/zcmp-prolog-epilog-crash.ll from D158256:

craig.topper:

Do we need the hidden and local_unnamed_addr attributes?
DONE

icmp slt i32 poison, poison doesn't make sense. The compiler would be allowed to delete it. So a future optimization could break this test.
DONE

store i32 0, ptr null, align 4 is a store to null which is undefined behavior. So the compiler is allowed to delete it. Again a future optimization could delete it and break this test.
DONE

jrtc27:

Use update_llc_test_checks.py
DONE

Though why is there an IR test and a MIR test? The MIR test seems preferable, surely?
Please feel free to drop: llvm/test/CodeGen/RISCV/zcmp-prolog-epilog-crash.ll

Though it looks like the same code is generated for both so can share one prefix?
Actually, there is a difference ...

nounwind
DONE

Space between declaration and definition
DONE

Clean up the IR of these comments?
DONE

Dangling attribute reference
DONE

This seems overly-reduced
Asked for clarification ...

Fix // No newline at end of file
DONE?

[anmolparalkar-nxp]

@garvitgupta08 respun post minor cleanup. Please use this latest version. Earlier checklist holds. Thanks!

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

if (FirstSPAdjustAmount) {
     RI->adjustReg(MBB, LastFrameDestroy, DL, SPReg, SPReg,
                   StackOffset::getFixed(SecondSPAdjustAmount),
                   MachineInstr::FrameDestroy, getStackAlign());
   }
 
   if (FirstSPAdjustAmount)
     StackSize = FirstSPAdjustAmount;
 
-  if (RVFI->isPushable(MF) && MBBI->getOpcode() == RISCV::CM_POP) {
+  if (RVFI->isPushable(MF) && MBBI != MBB.end() &&
+      MBBI->getOpcode() == RISCV::CM_POP) {
     // Use available stack adjustment in pop instruction to deallocate stack
     // space.
     unsigned PushStack = RVFI->getRVPushRegs() * (STI.getXLen() / 8);
     unsigned SpImmBase = RVFI->getRVPushStackSize();
     StackSize = adjSPInPushPop(MBBI, StackSize, (SpImmBase - PushStack), true);
   }
 
   // Deallocate stack

llvm/test/CodeGen/RISCV/frame-lowering-crash.ll

+; RUN: llc -mattr=+zcmp -verify-machineinstrs  \
+; RUN: -mtriple=riscv32 -target-abi ilp32 < %s \
+; RUN: | FileCheck %s -check-prefixes=RV32I
+; RUN: llc -mattr=+zcmp -verify-machineinstrs  \
+; RUN: -mtriple=riscv64 -target-abi lp64 < %s  \
+; RUN: | FileCheck %s -check-prefixes=RV64I
+
+; This input source exposed a crash in RISC-V RISCVFrameLowering when executing
+; the Prologue/Epilogue Insertion & Frame Finalization pass. The root cause was:
+; Not doing a bounds check before using a returned iterator.
+; NOTE: -mattr=+zcmp is essential to the reproducibility of the issue.
+
+declare hidden void @f1() local_unnamed_addr
+define hidden void @f0() local_unnamed_addr {
+; RV32I-LABEL: f0:                                     # @f0
+; RV32I:        .cfi_startproc
+; RV32I-NEXT: # %bb.0:                                # %entry
+; RV32I-NEXT:         beqz    zero, .LBB0_2
+; RV32I-NEXT: .LBB0_1:                                # %cleanup
+; RV32I-NEXT:         ret
+; RV32I-NEXT: .LBB0_2:                                # %if.end
+; RV32I-NEXT:         beqz    zero, .LBB0_5
+; RV32I-NEXT: # %bb.3:                                # %for.cond
+; RV32I-NEXT:         bnez    zero, .LBB0_1
+; RV32I-NEXT: .LBB0_4:                                # %for.body
+; RV32I-NEXT:                                         # =>This Inner Loop Header: Depth=1
+; RV32I-NEXT:         sw      zero, 0(zero)
+; RV32I-NEXT:         beqz    zero, .LBB0_4
+; RV32I-NEXT:         j       .LBB0_1
+; RV32I-NEXT: .LBB0_5:                                # %if.then2
+; RV32I-NEXT:         tail    f1
+; RV32I-NEXT: .Lfunc_end0:
+; RV32I-NEXT:         .size   f0, .Lfunc_end0-f0
+; RV32I-NEXT:         .cfi_endproc
+
+; RV64I-LABEL: f0:                                     # @f0
+; RV64I:        .cfi_startproc
+; RV64I-NEXT: # %bb.0:                                # %entry
+; RV64I-NEXT:         beqz    zero, .LBB0_2
+; RV64I-NEXT: .LBB0_1:                                # %cleanup
+; RV64I-NEXT:         ret
+; RV64I-NEXT: .LBB0_2:                                # %if.end
+; RV64I-NEXT:         beqz    zero, .LBB0_5
+; RV64I-NEXT: # %bb.3:                                # %for.cond
+; RV64I-NEXT:         bnez    zero, .LBB0_1
+; RV64I-NEXT: .LBB0_4:                                # %for.body
+; RV64I-NEXT:                                         # =>This Inner Loop Header: Depth=1
+; RV64I-NEXT:         sw      zero, 0(zero)
+; RV64I-NEXT:         beqz    zero, .LBB0_4
+; RV64I-NEXT:         j       .LBB0_1
+; RV64I-NEXT: .LBB0_5:                                # %if.then2
+; RV64I-NEXT:         tail    f1
+; RV64I-NEXT: .Lfunc_end0:
+; RV64I-NEXT:         .size   f0, .Lfunc_end0-f0
+; RV64I-NEXT:         .cfi_endproc
+
+entry:
+  br i1 poison, label %cleanup, label %if.end
+
+if.end:                                           ; preds = %entry
+  br i1 poison, label %if.end3, label %if.then2
+
+if.then2:                                         ; preds = %if.end
+  tail call void @f1() #4
+  br label %cleanup
+
+if.end3:                                          ; preds = %if.end
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.body, %if.end3
+  %cmp6 = icmp slt i32 poison, poison
+  br i1 %cmp6, label %for.body, label %for.cond8
+
+for.body:                                         ; preds = %for.cond
+  store i32 0, ptr null, align 4
+  br label %for.cond
+
+for.cond8:                                        ; preds = %for.cond
+  %cmp10 = icmp slt i32 poison, poison
+  br label %cleanup
+
+cleanup:                                          ; preds = %for.cond8, %if.then2, %entry
+  ret void
+}

llvm/test/CodeGen/RISCV/prolog-epilog-crash.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 2
+# REQUIRES: asserts
+# RUN: llc  %s -o - -mtriple=riscv32 -mattr=+zcmp -target-abi ilp32 -run-pass=prologepilog \
+# RUN:   -simplify-mir -verify-machineinstrs | FileCheck %s
+
+--- |
+  define hidden void @f(fp128 %a) local_unnamed_addr #0 {
+  entry:
+    %0 = bitcast fp128 %a to i128
+    %and.i = lshr i128 %0, 112
+    %1 = trunc i128 %and.i to i32
+    %2 = and i32 %1, 32767
+    %or.i = or i128 poison, 5192296858534827628530496329220096
+    br label %if.end.i
+
+  if.end.i:                                         ; preds = %entry
+    br i1 poison, label %exit, label %if.then12.i
+
+  if.then12.i:                                      ; preds = %if.end.i
+    %sub13.i = sub nuw nsw i32 16495, %2
+    %sh_prom.i = zext i32 %sub13.i to i128
+    %shr14.i = lshr i128 %or.i, %sh_prom.i
+    %conv15.i = trunc i128 %shr14.i to i32
+    br label %exit
+
+  exit:                                             ; preds = %if.then12.i, %if.end.i
+    %retval.0.i = phi i32 [ %conv15.i, %if.then12.i ], [ -1, %if.end.i ]
+    ret void
+  }
+...
+---
+name:            f
+alignment:       2
+tracksRegLiveness: true
+tracksDebugUserValues: true
+liveins:
+  - { reg: '$x10' }
+frameInfo:
+  maxAlignment:    1
+  localFrameSize:  32
+  savePoint:       '%bb.2'
+  restorePoint:    '%bb.2'
+stack:
+  - { id: 0, size: 32, alignment: 1, local-offset: -32 }
+machineFunctionInfo:
+  varArgsFrameIndex: 0
+  varArgsSaveSize: 0
+body:             |
+  ; CHECK-LABEL: name: f
+  ; CHECK: bb.0.entry:
+  ; CHECK-NEXT:   liveins: $x10
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   renamable $x10 = ADDI $x0, -1
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.1.if.end.i:
+  ; CHECK-NEXT:   liveins: $x10
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   BNE $x0, $x0, %bb.3
+  ; CHECK-NEXT:   PseudoBR %bb.2
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.2.if.then12.i:
+  ; CHECK-NEXT:   liveins: $x10
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   $x2 = frame-setup ADDI $x2, -32
+  ; CHECK-NEXT:   frame-setup CFI_INSTRUCTION def_cfa_offset 32
+  ; CHECK-NEXT:   SB $x0, $x2, 31 :: (store (s8) into %stack.0 + 31)
+  ; CHECK-NEXT:   SB $x0, $x2, 30 :: (store (s8) into %stack.0 + 30)
+  ; CHECK-NEXT:   SB $x0, $x2, 29 :: (store (s8) into %stack.0 + 29)
+  ; CHECK-NEXT:   SB $x0, $x2, 28 :: (store (s8) into %stack.0 + 28)
+  ; CHECK-NEXT:   SB $x0, $x2, 27 :: (store (s8) into %stack.0 + 27)
+  ; CHECK-NEXT:   SB $x0, $x2, 26 :: (store (s8) into %stack.0 + 26)
+  ; CHECK-NEXT:   SB $x0, $x2, 25 :: (store (s8) into %stack.0 + 25)
+  ; CHECK-NEXT:   SB $x0, $x2, 24 :: (store (s8) into %stack.0 + 24)
+  ; CHECK-NEXT:   SB $x0, $x2, 23 :: (store (s8) into %stack.0 + 23)
+  ; CHECK-NEXT:   SB $x0, $x2, 22 :: (store (s8) into %stack.0 + 22)
+  ; CHECK-NEXT:   SB $x0, $x2, 21 :: (store (s8) into %stack.0 + 21)
+  ; CHECK-NEXT:   SB $x0, $x2, 20 :: (store (s8) into %stack.0 + 20)
+  ; CHECK-NEXT:   SB $x0, $x2, 19 :: (store (s8) into %stack.0 + 19)
+  ; CHECK-NEXT:   SB $x0, $x2, 18 :: (store (s8) into %stack.0 + 18)
+  ; CHECK-NEXT:   SB $x0, $x2, 17 :: (store (s8) into %stack.0 + 17)
+  ; CHECK-NEXT:   SB $x0, $x2, 16 :: (store (s8) into %stack.0 + 16)
+  ; CHECK-NEXT:   SB renamable $x10, $x2, 0 :: (store (s8) into %stack.0)
+  ; CHECK-NEXT:   SB renamable $x10, $x2, 4 :: (store (s8) into %stack.0 + 4)
+  ; CHECK-NEXT:   renamable $x11 = SRLI renamable $x10, 24
+  ; CHECK-NEXT:   SB renamable $x11, $x2, 3 :: (store (s8) into %stack.0 + 3)
+  ; CHECK-NEXT:   renamable $x12 = SRLI renamable $x10, 16
+  ; CHECK-NEXT:   SB renamable $x12, $x2, 2 :: (store (s8) into %stack.0 + 2)
+  ; CHECK-NEXT:   renamable $x13 = SRLI renamable $x10, 8
+  ; CHECK-NEXT:   SB renamable $x13, $x2, 1 :: (store (s8) into %stack.0 + 1)
+  ; CHECK-NEXT:   SB renamable $x10, $x2, 8 :: (store (s8) into %stack.0 + 8)
+  ; CHECK-NEXT:   SB renamable $x11, $x2, 7 :: (store (s8) into %stack.0 + 7)
+  ; CHECK-NEXT:   SB renamable $x12, $x2, 6 :: (store (s8) into %stack.0 + 6)
+  ; CHECK-NEXT:   SB renamable $x13, $x2, 5 :: (store (s8) into %stack.0 + 5)
+  ; CHECK-NEXT:   SB killed renamable $x10, $x2, 12 :: (store (s8) into %stack.0 + 12)
+  ; CHECK-NEXT:   SB renamable $x11, $x2, 11 :: (store (s8) into %stack.0 + 11)
+  ; CHECK-NEXT:   SB renamable $x12, $x2, 10 :: (store (s8) into %stack.0 + 10)
+  ; CHECK-NEXT:   SB renamable $x13, $x2, 9 :: (store (s8) into %stack.0 + 9)
+  ; CHECK-NEXT:   SB killed renamable $x11, $x2, 15 :: (store (s8) into %stack.0 + 15)
+  ; CHECK-NEXT:   SB killed renamable $x12, $x2, 14 :: (store (s8) into %stack.0 + 14)
+  ; CHECK-NEXT:   SB killed renamable $x13, $x2, 13 :: (store (s8) into %stack.0 + 13)
+  ; CHECK-NEXT:   $x2 = frame-destroy ADDI $x2, 32
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.3.exit:
+  ; CHECK-NEXT:   PseudoRET
+  bb.0.entry:
+    liveins: $x10
+
+    renamable $x10 = ADDI $x0, -1
+
+  bb.1.if.end.i:
+    liveins: $x10
+
+    BNE $x0, $x0, %bb.3
+    PseudoBR %bb.2
+
+  bb.2.if.then12.i:
+    liveins: $x10
+
+    SB $x0, %stack.0, 31 :: (store (s8) into %stack.0 + 31)
+    SB $x0, %stack.0, 30 :: (store (s8) into %stack.0 + 30)
+    SB $x0, %stack.0, 29 :: (store (s8) into %stack.0 + 29)
+    SB $x0, %stack.0, 28 :: (store (s8) into %stack.0 + 28)
+    SB $x0, %stack.0, 27 :: (store (s8) into %stack.0 + 27)
+    SB $x0, %stack.0, 26 :: (store (s8) into %stack.0 + 26)
+    SB $x0, %stack.0, 25 :: (store (s8) into %stack.0 + 25)
+    SB $x0, %stack.0, 24 :: (store (s8) into %stack.0 + 24)
+    SB $x0, %stack.0, 23 :: (store (s8) into %stack.0 + 23)
+    SB $x0, %stack.0, 22 :: (store (s8) into %stack.0 + 22)
+    SB $x0, %stack.0, 21 :: (store (s8) into %stack.0 + 21)
+    SB $x0, %stack.0, 20 :: (store (s8) into %stack.0 + 20)
+    SB $x0, %stack.0, 19 :: (store (s8) into %stack.0 + 19)
+    SB $x0, %stack.0, 18 :: (store (s8) into %stack.0 + 18)
+    SB $x0, %stack.0, 17 :: (store (s8) into %stack.0 + 17)
+    SB $x0, %stack.0, 16 :: (store (s8) into %stack.0 + 16)
+    SB renamable $x10, %stack.0, 0 :: (store (s8) into %stack.0)
+    SB renamable $x10, %stack.0, 4 :: (store (s8) into %stack.0 + 4)
+    renamable $x11 = SRLI renamable $x10, 24
+    SB renamable $x11, %stack.0, 3 :: (store (s8) into %stack.0 + 3)
+    renamable $x12 = SRLI renamable $x10, 16
+    SB renamable $x12, %stack.0, 2 :: (store (s8) into %stack.0 + 2)
+    renamable $x13 = SRLI renamable $x10, 8
+    SB renamable $x13, %stack.0, 1 :: (store (s8) into %stack.0 + 1)
+    SB renamable $x10, %stack.0, 8 :: (store (s8) into %stack.0 + 8)
+    SB renamable $x11, %stack.0, 7 :: (store (s8) into %stack.0 + 7)
+    SB renamable $x12, %stack.0, 6 :: (store (s8) into %stack.0 + 6)
+    SB renamable $x13, %stack.0, 5 :: (store (s8) into %stack.0 + 5)
+    SB killed renamable $x10, %stack.0, 12 :: (store (s8) into %stack.0 + 12)
+    SB renamable $x11, %stack.0, 11 :: (store (s8) into %stack.0 + 11)
+    SB renamable $x12, %stack.0, 10 :: (store (s8) into %stack.0 + 10)
+    SB renamable $x13, %stack.0, 9 :: (store (s8) into %stack.0 + 9)
+    SB killed renamable $x11, %stack.0, 15 :: (store (s8) into %stack.0 + 15)
+    SB killed renamable $x12, %stack.0, 14 :: (store (s8) into %stack.0 + 14)
+    SB killed renamable $x13, %stack.0, 13 :: (store (s8) into %stack.0 + 13)
+
+  bb.3.exit:
+    PseudoRET
+
+...