cpu/{arm|thumb}(chore): change how branch disassembly happens

Signed-off-by: Amneesh Singh <natto@weirdnatto.in>

include/cpu/arm/instruction.hh

@@ -26,7 +26,7 @@ struct BranchAndExchange {
 
 struct Branch {
     bool link;
-    uint32_t offset;
+    int32_t offset;
 };
 
 struct Multiply {

include/cpu/thumb/instruction.hh

@@ -282,7 +282,7 @@ struct Instruction {
     void exec(Cpu& cpu);
 
 #ifdef DISASSEMBLER
-    std::string disassemble(uint32_t pc = 0);
+    std::string disassemble();
 #endif
 
     InstructionData data;

src/cpu/arm/disassembler.cc

@@ -1,7 +1,6 @@
 #include "cpu/arm/instruction.hh"
 #include "util/bits.hh"
 #include <format>
-#include <string>
 
 namespace matar::arm {
 std::string
@@ -14,8 +13,10 @@ Instruction::disassemble() {
             return std::format("BX{} R{:d}", condition, data.rn);
         },
             [condition](Branch& data) {
-            return std::format(
+            return std::format("B{}{} {:#06x}",
-              "B{}{} 0x{:06X}", (data.link ? "L" : ""), condition, data.offset);
+                               (data.link ? "L" : ""),
+                               condition,
+                               static_cast<int32_t>(data.offset + 2 * INSTRUCTION_SIZE));
         },
         [condition](Multiply& data) {
             if (data.acc) {

src/cpu/arm/exec.cc

@@ -40,17 +40,14 @@ Instruction::exec(Cpu& cpu) {
             if (state == State::Arm)
                 rst_bit(cpu.pc, 1);
 
-            // pc is affected so flush the pipeline
+            // PC is affected so flush the pipeline
             cpu.is_flushed = true;
         },
         [&cpu](Branch& data) {
             if (data.link)
                 cpu.gpr[14] = cpu.pc - INSTRUCTION_SIZE;
 
-            // data.offset accounts for two instructions ahead when
+            cpu.pc += data.offset;
-            // disassembling, so need to adjust
-            cpu.pc =
-              static_cast<int32_t>(cpu.pc) - 2 * INSTRUCTION_SIZE + data.offset;
 
             // pc is affected so flush the pipeline
             cpu.is_flushed = true;

src/cpu/arm/instruction.cc

@@ -1,6 +1,5 @@
 #include "cpu/arm/instruction.hh"
 #include "util/bits.hh"
-#include <iterator>
 
 namespace matar::arm {
 Instruction::Instruction(uint32_t insn)
@@ -14,12 +13,10 @@ Instruction::Instruction(uint32_t insn)
         // Branch
     } else if ((insn & 0x0E000000) == 0x0A000000) {
         bool link      = get_bit(insn, 24);
-        uint32_t offset = bit_range(insn, 0, 23);
+        int32_t offset = static_cast<int32_t>(bit_range(insn, 0, 23));
 
         // lsh 2 and sign extend the 26 bit offset to 32 bits
-        offset = (static_cast<int32_t>(offset) << 8) >> 6;
+        offset = (offset << 8) >> 6;
-
-        offset += 2 * INSTRUCTION_SIZE;
 
         data = Branch{ .link = link, .offset = offset };
 

src/cpu/thumb/disassembler.cc

@@ -4,7 +4,7 @@
 
 namespace matar::thumb {
 std::string
-Instruction::disassemble(uint32_t pc) {
+Instruction::disassemble() {
     return std::visit(
       overloaded{
         [](MoveShiftedRegister& data) {
@@ -133,16 +133,16 @@ Instruction::disassemble(uint32_t pc) {
         [](SoftwareInterrupt& data) {
             return std::format("SWI {:d}", data.vector);
         },
-        [pc](ConditionalBranch& data) {
+        [](ConditionalBranch& data) {
             return std::format(
               "B{} #{:d}",
               stringify(data.condition),
-              static_cast<int32_t>(data.offset + pc + 2 * INSTRUCTION_SIZE));
+              static_cast<int32_t>(data.offset + 2 * INSTRUCTION_SIZE));
         },
-        [pc](UnconditionalBranch& data) {
+        [](UnconditionalBranch& data) {
             return std::format(
               "B #{:d}",
-              static_cast<int32_t>(data.offset + pc + 2 * INSTRUCTION_SIZE));
+              static_cast<int32_t>(data.offset + 2 * INSTRUCTION_SIZE));
         },
         [](LongBranchWithLink& data) {
             // duh this manual be empty for H = 0

tests/cpu/arm/exec.cc

@@ -24,18 +24,25 @@ TEST_CASE_METHOD(CpuFixture, "Branch", TAG) {
     InstructionData data = Branch{ .link = false, .offset = 3489748 };
     Branch* branch       = std::get_if<Branch>(&data);
 
+    // set PC to 48
+    setr(15, 48);
+
     exec(data);
 
-    CHECK(getr(15) == 3489748);
+    // 48 + offset
+    CHECK(getr(15) == 3489796);
     CHECK(getr(14) == 0);
 
     // with link
     reset();
+    setr(15, 48);
     branch->link = true;
     exec(data);
 
-    CHECK(getr(15) == 3489748);
+    // 48 + offset
-    CHECK(getr(14) == 0 + INSTRUCTION_SIZE);
+    CHECK(getr(15) == 3489796);
+    // pc was set to 48
+    CHECK(getr(14) == 48 - INSTRUCTION_SIZE);
 }
 
 TEST_CASE_METHOD(CpuFixture, "Multiply", TAG) {

tests/cpu/arm/instruction.cc

@@ -31,15 +31,16 @@ TEST_CASE("Branch", TAG) {
 
     // last 24 bits = 8748995
     // (8748995 << 8) >> 6 sign extended = 0xFE15FF0C
-    // Also +8 since PC is two instructions ahead
+    CHECK(b->offset == static_cast<int32_t>(0xfe15ff0c));
-    CHECK(b->offset == 0xFE15FF14);
     CHECK(b->link == true);
 
 #ifdef DISASSEMBLER
-    CHECK(instruction.disassemble() == "BL 0xFE15FF14");
+    // take prefetch into account
+    // offset + 8 = 0xfe15ff0c + 8 = -0x1ea00e4 + 8
+    CHECK(instruction.disassemble() == "BL -0x1ea00ec");
 
     b->link = false;
-    CHECK(instruction.disassemble() == "B 0xFE15FF14");
+    CHECK(instruction.disassemble() == "B -0x1ea00ec");
 #endif
 }
 

tests/cpu/thumb/exec.cc

@@ -1,6 +1,5 @@
 #include "cpu/cpu-fixture.hh"
 #include "cpu/thumb/instruction.hh"
-#include "util/bits.hh"
 #include <catch2/catch_test_macros.hpp>
 
 using namespace matar;
@@ -531,8 +530,9 @@ TEST_CASE_METHOD(CpuFixture, "PC Relative Load", TAG) {
     InstructionData data = PcRelativeLoad{ .word = 0x578, .rd = 0 };
 
     setr(15, 0x3003FD5);
-    // 0x3003FD5 + 0x578
+    // setting bit 0 for 0x3003FD5, we get 0x3003FD4
-    bus.write_word(0x300454D, 489753492);
+    // 0x3003FD4 + 0x578
+    bus.write_word(0x300454C, 489753492);
 
     CHECK(getr(0) == 0);
     exec(data);

tests/cpu/thumb/instruction.cc

@@ -412,7 +412,8 @@ TEST_CASE("Conditional Branch", TAG) {
     CHECK(b->condition == Condition::LS);
 
 #ifdef DISASSEMBLER
-    // (-76 << 1) + PC (0) + 4
+    // take prefetch into account
+    // offset + 4 = -152 + 4
     CHECK(instruction.disassemble() == "BLS #-148");
 #endif
 }
@@ -439,7 +440,8 @@ TEST_CASE("Unconditional Branch") {
     REQUIRE(b->offset == -410);
 
 #ifdef DISASSEMBLER
-    // (2147483443 << 1) + PC(0) + 4
+    // take prefetch into account
+    // offset + 4 = -410 + 4
     CHECK(instruction.disassemble() == "B #-406");
 #endif
 }