From 60dae85ef3d04f93868e40f4e2921d2167bcd434 Mon Sep 17 00:00:00 2001 From: Kristof Beyls Date: Sat, 23 Sep 2023 20:12:53 +0200 Subject: [PATCH 1/2] Correctly print preferred disassembly for instructions with an annotation. This patch makes sure that BinaryContext::printInstruction prints the preferred disassembly. Preferred disassembly only gets printed when there are no annotations on the MCInst. Therefore, this patch temporarily removes the annotations before printing it. A few examples of before and after on AArch64 instructions are as follows: ``` BEFORE AFTER (preferred disassembly) ret x30 ret orr x30, xzr, x0 mov x30, x0 hint #29 autiasp hint #12 autia1716 ``` Clearly, the preferred disassembly is easier for developers to read, and is the disassembly that tools should be printing. This patch is motivated as part of future work on the llvm-bolt-binary-analysis tool, making sure that the reports it prints do use preferred disassembly. This patch was cherry-picked from https://github.com/kbeyls/llvm-project/tree/bolt-gadget-scanner-prototype. In this current patch, this only affects existing RISCV test cases. This patch also does improve test cases in future patches that will introduce a binary analysis for llvm-bolt-binary-analysis that checks for correct application of pac-ret (pointer authentication on return addresses). --- bolt/lib/Core/BinaryContext.cpp | 10 +++++++++- bolt/test/RISCV/call-annotations.s | 8 ++++---- bolt/test/RISCV/relax.s | 4 ++-- bolt/test/RISCV/reloc-branch.s | 2 +- bolt/test/RISCV/reloc-jal.s | 2 +- 5 files changed, 17 insertions(+), 9 deletions(-) diff --git a/bolt/lib/Core/BinaryContext.cpp b/bolt/lib/Core/BinaryContext.cpp index ac96b836ed579..0fcbf7452cf88 100644 --- a/bolt/lib/Core/BinaryContext.cpp +++ b/bolt/lib/Core/BinaryContext.cpp @@ -1961,7 +1961,15 @@ void BinaryContext::printInstruction(raw_ostream &OS, const MCInst &Instruction, OS << "\tjit\t" << MIB->getTargetSymbol(Instruction)->getName() << " # ID: " << DynamicID; } else { - InstPrinter->printInst(&Instruction, 0, "", *STI, OS); + // If there are annotations on the instruction, the MCInstPrinter will fail to + // print the preferred alias as it only does so when the number of operands is + // as expected. See + // https://github.com/llvm/llvm-project/blob/782f1a0d895646c364a53f9dcdd6d4ec1f3e5ea0/llvm/lib/MC/MCInstPrinter.cpp#L142 + // Therefore, create a temporary copy of the Inst from which the annotations are + // removed, and print that Inst. + MCInst InstNoAnnot = Instruction; + MIB->stripAnnotations(InstNoAnnot); + InstPrinter->printInst(&InstNoAnnot, 0, "", *STI, OS); } if (MIB->isCall(Instruction)) { if (MIB->isTailCall(Instruction)) diff --git a/bolt/test/RISCV/call-annotations.s b/bolt/test/RISCV/call-annotations.s index ff99e0f1dfd84..f876544e214ca 100644 --- a/bolt/test/RISCV/call-annotations.s +++ b/bolt/test/RISCV/call-annotations.s @@ -16,13 +16,13 @@ f: // CHECK-LABEL: Binary Function "_start" after building cfg { // CHECK: auipc ra, f -// CHECK-NEXT: jalr ra, -0x4(ra) # Offset: 4 -// CHECK-NEXT: jal ra, f # Offset: 8 -// CHECK-NEXT: jal zero, f # TAILCALL # Offset: 12 +// CHECK-NEXT: jalr -0x4(ra) # Offset: 4 +// CHECK-NEXT: jal f # Offset: 8 +// CHECK-NEXT: j f # TAILCALL # Offset: 12 // CHECK-LABEL: Binary Function "long_tail" after building cfg { // CHECK: auipc t1, f -// CHECK-NEXT: jalr zero, -0x18(t1) # TAILCALL # Offset: 8 +// CHECK-NEXT: jr -0x18(t1) # TAILCALL # Offset: 8 // CHECK-LABEL: Binary Function "compressed_tail" after building cfg { // CHECK: jr a0 # TAILCALL # Offset: 0 diff --git a/bolt/test/RISCV/relax.s b/bolt/test/RISCV/relax.s index ec390ea76b5c7..74f049b8f8dd9 100644 --- a/bolt/test/RISCV/relax.s +++ b/bolt/test/RISCV/relax.s @@ -5,9 +5,9 @@ // RUN: llvm-objdump -d %t.bolt | FileCheck --check-prefix=OBJDUMP %s // CHECK: Binary Function "_start" after building cfg { -// CHECK: jal ra, near_f +// CHECK: jal near_f // CHECK-NEXT: auipc ra, far_f -// CHECK-NEXT: jalr ra, 0xc(ra) +// CHECK-NEXT: jalr 0xc(ra) // CHECK-NEXT: j near_f // CHECK: Binary Function "_start" after fix-riscv-calls { diff --git a/bolt/test/RISCV/reloc-branch.s b/bolt/test/RISCV/reloc-branch.s index 6a8b5a28e19d9..780d978f36f44 100644 --- a/bolt/test/RISCV/reloc-branch.s +++ b/bolt/test/RISCV/reloc-branch.s @@ -7,7 +7,7 @@ .p2align 1 // CHECK: Binary Function "_start" after building cfg { _start: -// CHECK: beq zero, zero, .Ltmp0 +// CHECK: beqz zero, .Ltmp0 beq zero, zero, 1f nop // CHECK: .Ltmp0 diff --git a/bolt/test/RISCV/reloc-jal.s b/bolt/test/RISCV/reloc-jal.s index ce54265fac05e..62a2f1dbea03a 100644 --- a/bolt/test/RISCV/reloc-jal.s +++ b/bolt/test/RISCV/reloc-jal.s @@ -14,7 +14,7 @@ f: .globl _start .p2align 1 _start: -// CHECK: jal ra, f +// CHECK: jal f jal ra, f ret .size _start, .-_start From c1b2afa1b1bc128240f5405c803e1f6ccb83d308 Mon Sep 17 00:00:00 2001 From: Kristof Beyls Date: Thu, 19 Dec 2024 18:03:35 +0000 Subject: [PATCH 2/2] clang-format --- bolt/lib/Core/BinaryContext.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/bolt/lib/Core/BinaryContext.cpp b/bolt/lib/Core/BinaryContext.cpp index 0fcbf7452cf88..f88e34b8e8962 100644 --- a/bolt/lib/Core/BinaryContext.cpp +++ b/bolt/lib/Core/BinaryContext.cpp @@ -1961,12 +1961,12 @@ void BinaryContext::printInstruction(raw_ostream &OS, const MCInst &Instruction, OS << "\tjit\t" << MIB->getTargetSymbol(Instruction)->getName() << " # ID: " << DynamicID; } else { - // If there are annotations on the instruction, the MCInstPrinter will fail to - // print the preferred alias as it only does so when the number of operands is - // as expected. See + // If there are annotations on the instruction, the MCInstPrinter will fail + // to print the preferred alias as it only does so when the number of + // operands is as expected. See // https://github.com/llvm/llvm-project/blob/782f1a0d895646c364a53f9dcdd6d4ec1f3e5ea0/llvm/lib/MC/MCInstPrinter.cpp#L142 - // Therefore, create a temporary copy of the Inst from which the annotations are - // removed, and print that Inst. + // Therefore, create a temporary copy of the Inst from which the annotations + // are removed, and print that Inst. MCInst InstNoAnnot = Instruction; MIB->stripAnnotations(InstNoAnnot); InstPrinter->printInst(&InstNoAnnot, 0, "", *STI, OS);