From 9b05a6f097cef7acc59d9266f464a7d14142943e Mon Sep 17 00:00:00 2001 From: Craig Topper Date: Fri, 20 Sep 2024 21:50:08 -0700 Subject: [PATCH 1/2] [RISCV} Don't delete all fixups in RISCVMCCodeEmitter::expandLongCondBr. The Fixups vector passed into this function may already have fixups in it from earlier instructions. We should not erase those. We just want to erase fixups added by this function. Fixes #108612. --- .../RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp | 7 ++++++- .../MC/RISCV/long-conditional-jump-crash.s | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 llvm/test/MC/RISCV/long-conditional-jump-crash.s diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp index 75323632dd533..66394dc8cd138 100644 --- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp +++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp @@ -283,13 +283,18 @@ void RISCVMCCodeEmitter::expandLongCondBr(const MCInst &MI, Offset = 4; } + // Save the number fixups. + size_t N = Fixups.size(); + // Emit an unconditional jump to the destination. MCInst TmpInst = MCInstBuilder(RISCV::JAL).addReg(RISCV::X0).addOperand(SrcSymbol); uint32_t Binary = getBinaryCodeForInstr(TmpInst, Fixups, STI); support::endian::write(CB, Binary, llvm::endianness::little); - Fixups.clear(); + // Drop any fixup added so we can add the correct one. + Fixups.resize(N); + if (SrcSymbol.isExpr()) { Fixups.push_back(MCFixup::create(Offset, SrcSymbol.getExpr(), MCFixupKind(RISCV::fixup_riscv_jal), diff --git a/llvm/test/MC/RISCV/long-conditional-jump-crash.s b/llvm/test/MC/RISCV/long-conditional-jump-crash.s new file mode 100644 index 0000000000000..bac0036ca5568 --- /dev/null +++ b/llvm/test/MC/RISCV/long-conditional-jump-crash.s @@ -0,0 +1,19 @@ +# RUN: llvm-mc %s -mc-relax-all -triple=riscv64 -filetype=obj \ +# RUN: | llvm-objdump -d -M no-aliases - \ +# RUN: | FileCheck --check-prefix=CHECK %s + +# This test previously crashed because expanding a conditional branch deleted +# all fixups in the fragment. + +# CHECK: beq s0, zero, 0x8 +# CHECK-NEXT: jal zero, 0x14 +# CHECK-NEXT: jal zero, 0x14 +# CHECK-NEXT: bne s0, zero, 0x14 +# CHECK-NEXT: jal zero, 0x14 + +# CHECK: jalr zero, 0x0(ra) + bnez s0, .foo + j .foo + beqz s0, .foo +.foo: + ret From de87c275f7d4021c307d75eec24d6c758b5c1083 Mon Sep 17 00:00:00 2001 From: Craig Topper Date: Sun, 22 Sep 2024 16:49:19 -0700 Subject: [PATCH 2/2] fixup! address review comments. --- .../RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp | 4 ++-- .../MC/RISCV/long-conditional-jump-crash.s | 19 ------------------- llvm/test/MC/RISCV/rv64-relax-all.s | 6 ++++++ 3 files changed, 8 insertions(+), 21 deletions(-) delete mode 100644 llvm/test/MC/RISCV/long-conditional-jump-crash.s diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp index 66394dc8cd138..eb21498d15e86 100644 --- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp +++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp @@ -284,7 +284,7 @@ void RISCVMCCodeEmitter::expandLongCondBr(const MCInst &MI, } // Save the number fixups. - size_t N = Fixups.size(); + size_t FixupStartIndex = Fixups.size(); // Emit an unconditional jump to the destination. MCInst TmpInst = @@ -293,7 +293,7 @@ void RISCVMCCodeEmitter::expandLongCondBr(const MCInst &MI, support::endian::write(CB, Binary, llvm::endianness::little); // Drop any fixup added so we can add the correct one. - Fixups.resize(N); + Fixups.resize(FixupStartIndex); if (SrcSymbol.isExpr()) { Fixups.push_back(MCFixup::create(Offset, SrcSymbol.getExpr(), diff --git a/llvm/test/MC/RISCV/long-conditional-jump-crash.s b/llvm/test/MC/RISCV/long-conditional-jump-crash.s deleted file mode 100644 index bac0036ca5568..0000000000000 --- a/llvm/test/MC/RISCV/long-conditional-jump-crash.s +++ /dev/null @@ -1,19 +0,0 @@ -# RUN: llvm-mc %s -mc-relax-all -triple=riscv64 -filetype=obj \ -# RUN: | llvm-objdump -d -M no-aliases - \ -# RUN: | FileCheck --check-prefix=CHECK %s - -# This test previously crashed because expanding a conditional branch deleted -# all fixups in the fragment. - -# CHECK: beq s0, zero, 0x8 -# CHECK-NEXT: jal zero, 0x14 -# CHECK-NEXT: jal zero, 0x14 -# CHECK-NEXT: bne s0, zero, 0x14 -# CHECK-NEXT: jal zero, 0x14 - -# CHECK: jalr zero, 0x0(ra) - bnez s0, .foo - j .foo - beqz s0, .foo -.foo: - ret diff --git a/llvm/test/MC/RISCV/rv64-relax-all.s b/llvm/test/MC/RISCV/rv64-relax-all.s index 70a3f77540c99..6705d6ecfb5b6 100644 --- a/llvm/test/MC/RISCV/rv64-relax-all.s +++ b/llvm/test/MC/RISCV/rv64-relax-all.s @@ -14,3 +14,9 @@ c.beqz a0, NEAR # INSTR: c.j 0x0 # RELAX-INSTR: jal zero, 0x0 c.j NEAR + +bnez s0, .foo +j .foo +beqz s0, .foo +.foo: +ret