From 800c76a4fe0b2d34cd7b098241df357f264ac4b9 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Thu, 9 Nov 2023 11:55:04 -0500 Subject: [PATCH 1/3] Capture and respond to IDIV_REP. --- InstructionSets/x86/Decoder.cpp | 2 +- .../x86/Implementation/Arithmetic.hpp | 11 +++++++++-- .../Implementation/PerformImplementation.hpp | 9 +++++---- InstructionSets/x86/Instruction.cpp | 17 +++++++++-------- InstructionSets/x86/Instruction.hpp | 9 +++++++++ 5 files changed, 33 insertions(+), 15 deletions(-) diff --git a/InstructionSets/x86/Decoder.cpp b/InstructionSets/x86/Decoder.cpp index 32d676b1d..4b6c788d9 100644 --- a/InstructionSets/x86/Decoder.cpp +++ b/InstructionSets/x86/Decoder.cpp @@ -33,7 +33,7 @@ std::pair::InstructionT> Decoder::decode(con /// Sets the operation and verifies that the current repetition, if any, is compatible, discarding it otherwise. #define SetOperation(op) \ - operation_ = rep_operation(op, repetition_); + operation_ = rep_operation(op, repetition_); /// Helper macro for those that follow. #define SetOpSrcDestSize(op, src, dest, size) \ diff --git a/InstructionSets/x86/Implementation/Arithmetic.hpp b/InstructionSets/x86/Implementation/Arithmetic.hpp index 971a877cb..ecc36508b 100644 --- a/InstructionSets/x86/Implementation/Arithmetic.hpp +++ b/InstructionSets/x86/Implementation/Arithmetic.hpp @@ -227,7 +227,7 @@ void div( destination_high = dividend % source; } -template +template void idiv( modify_t destination_high, modify_t destination_low, @@ -279,7 +279,14 @@ void idiv( // TEMPORARY HACK. Will not work with DWords. using sIntT = typename std::make_signed::type; const int32_t dividend = (sIntT(destination_high) << (8 * sizeof(IntT))) + destination_low; - const auto result = dividend / sIntT(source); + auto result = dividend / sIntT(source); + + // An 8086 quirk: rep IDIV performs an IDIV that switches the sign on its result, + // due to reuse of an internal flag. + if constexpr (invert) { + result = -result; + } + if(sIntT(result) != result) { interrupt(Interrupt::DivideError, context); return; diff --git a/InstructionSets/x86/Implementation/PerformImplementation.hpp b/InstructionSets/x86/Implementation/PerformImplementation.hpp index a28073a1b..1808366e4 100644 --- a/InstructionSets/x86/Implementation/PerformImplementation.hpp +++ b/InstructionSets/x86/Implementation/PerformImplementation.hpp @@ -207,10 +207,11 @@ template < Primitive::test(destination_r(), source_r(), context); return; - case Operation::MUL: Primitive::mul(pair_high(), pair_low(), source_r(), context); return; - case Operation::IMUL_1: Primitive::imul(pair_high(), pair_low(), source_r(), context); return; - case Operation::DIV: Primitive::div(pair_high(), pair_low(), source_r(), context); return; - case Operation::IDIV: Primitive::idiv(pair_high(), pair_low(), source_r(), context); return; + case Operation::MUL: Primitive::mul(pair_high(), pair_low(), source_r(), context); return; + case Operation::IMUL_1: Primitive::imul(pair_high(), pair_low(), source_r(), context); return; + case Operation::DIV: Primitive::div(pair_high(), pair_low(), source_r(), context); return; + case Operation::IDIV: Primitive::idiv(pair_high(), pair_low(), source_r(), context); return; + case Operation::IDIV_REP: Primitive::idiv(pair_high(), pair_low(), source_r(), context); return; case Operation::INC: Primitive::inc(destination_rmw(), context); break; case Operation::DEC: Primitive::dec(destination_rmw(), context); break; diff --git a/InstructionSets/x86/Instruction.cpp b/InstructionSets/x86/Instruction.cpp index d66a43d3c..edcf2b738 100644 --- a/InstructionSets/x86/Instruction.cpp +++ b/InstructionSets/x86/Instruction.cpp @@ -105,14 +105,15 @@ std::string InstructionSet::x86::to_string(Operation operation, DataSize size, M case Operation::HLT: return "hlt"; case Operation::WAIT: return "wait"; - case Operation::ADC: return "adc"; - case Operation::ADD: return "add"; - case Operation::SBB: return "sbb"; - case Operation::SUB: return "sub"; - case Operation::MUL: return "mul"; - case Operation::IMUL_1: return "imul"; - case Operation::DIV: return "div"; - case Operation::IDIV: return "idiv"; + case Operation::ADC: return "adc"; + case Operation::ADD: return "add"; + case Operation::SBB: return "sbb"; + case Operation::SUB: return "sub"; + case Operation::MUL: return "mul"; + case Operation::IMUL_1: return "imul"; + case Operation::DIV: return "div"; + case Operation::IDIV: return "idiv"; + case Operation::IDIV_REP: return "idiv"; case Operation::INC: return "inc"; case Operation::DEC: return "dec"; diff --git a/InstructionSets/x86/Instruction.hpp b/InstructionSets/x86/Instruction.hpp index 86326657f..23239bb66 100644 --- a/InstructionSets/x86/Instruction.hpp +++ b/InstructionSets/x86/Instruction.hpp @@ -231,6 +231,8 @@ enum class Operation: uint8_t { SETMOC, /// Set destination to ~0. SETMO, + /// Perform an IDIV and negative the result. + IDIV_REP, // // 80186 additions. @@ -474,8 +476,15 @@ enum class Repetition: uint8_t { }; /// @returns @c true if @c operation supports repetition mode @c repetition; @c false otherwise. +template constexpr Operation rep_operation(Operation operation, Repetition repetition) { switch(operation) { + case Operation::IDIV: + if constexpr (model == Model::i8086) { + return repetition != Repetition::None ? Operation::IDIV_REP : Operation::IDIV; + } + [[fallthrough]]; + default: return operation; case Operation::INS: From ed3922e458b7c590a5ee6743f0f5c121d4959935 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Thu, 9 Nov 2023 11:55:36 -0500 Subject: [PATCH 2/3] Switch to accepting failures only with a reason. --- OSBindings/Mac/Clock SignalTests/8088Tests.mm | 124 +++++++++++------- 1 file changed, 75 insertions(+), 49 deletions(-) diff --git a/OSBindings/Mac/Clock SignalTests/8088Tests.mm b/OSBindings/Mac/Clock SignalTests/8088Tests.mm index 0cf44421f..f53a51688 100644 --- a/OSBindings/Mac/Clock SignalTests/8088Tests.mm +++ b/OSBindings/Mac/Clock SignalTests/8088Tests.mm @@ -387,16 +387,17 @@ struct FailedExecution { @implementation i8088Tests { std::vector execution_failures; + std::vector permitted_failures; ExecutionSupport execution_support; } - (NSArray *)testFiles { NSString *path = [NSString stringWithUTF8String:TestSuiteHome]; NSSet *allowList = [NSSet setWithArray:@[ - // Current execution failures: -// @"D4.json.gz", // AAM -// @"F6.7.json.gz", // IDIV -// @"F7.7.json.gz", // IDIV + // Current execution failures, albeit all permitted: + @"D4.json.gz", // AAM + @"F6.7.json.gz", // IDIV byte + @"F7.7.json.gz", // IDIV word ]]; NSSet *ignoreList = nil; @@ -639,53 +640,82 @@ struct FailedExecution { const bool registersEqual = intended_registers == execution_support.registers; const bool flagsEqual = (intended_flags.get() & flags_mask) == (execution_support.flags.get() & flags_mask); - if(!flagsEqual || !registersEqual || !ramEqual) { - FailedExecution failure; - failure.instruction = decoded.second; - failure.test_name = std::string([test[@"name"] UTF8String]); + // Exit if no issues were found. + if(flagsEqual && registersEqual && ramEqual) { + return; + } - NSMutableArray *reasons = [[NSMutableArray alloc] init]; - if(!flagsEqual) { - Flags difference; - difference.set((intended_flags.get() ^ execution_support.flags.get()) & flags_mask); - [reasons addObject: - [NSString stringWithFormat:@"flags differs; errors in %s", - difference.to_string().c_str()]]; + // Presume this is a genuine failure. + std::vector *failure_list = &execution_failures; + + // Redirect it if it's an acceptable failure. + using Operation = InstructionSet::x86::Operation; + + // AAM 00h throws its exception only after modifying flags in an undocumented manner; + // I'm not too concerned about this because AAM 00h is an undocumented usage of 00h, + // not even supported by NEC amongst others, and the proper exception is being thrown. + if(decoded.second.operation() == Operation::AAM && !decoded.second.operand()) { + failure_list = &permitted_failures; + } + + // IDIV_REP has a couple of cases... + if(decoded.second.operation() == Operation::IDIV_REP) { + // For reasons I don't understand, sometimes the test set doesn't increment the IP + // across a REP_IDIV. I don't think (?) this correlates to real 8086 behaviour. + // More research required, but for now I'm not treating this as a roadblock. + Registers advanced_registers = intended_registers; + advanced_registers.ip_ += decoded.first; + if(advanced_registers == execution_support.registers && ramEqual && flagsEqual) { + failure_list = &permitted_failures; } - if(!registersEqual) { - NSMutableArray *registers = [[NSMutableArray alloc] init]; + } + + // Record a failure. + FailedExecution failure; + failure.instruction = decoded.second; + failure.test_name = std::string([test[@"name"] UTF8String]); + + NSMutableArray *reasons = [[NSMutableArray alloc] init]; + if(!flagsEqual) { + Flags difference; + difference.set((intended_flags.get() ^ execution_support.flags.get()) & flags_mask); + [reasons addObject: + [NSString stringWithFormat:@"flags differs; errors in %s", + difference.to_string().c_str()]]; + } + if(!registersEqual) { + NSMutableArray *registers = [[NSMutableArray alloc] init]; #define Reg(x) \ if(intended_registers.x() != execution_support.registers.x()) \ [registers addObject: \ [NSString stringWithFormat: \ @#x" is %04x rather than %04x", execution_support.registers.x(), intended_registers.x()]]; - Reg(ax); - Reg(cx); - Reg(dx); - Reg(bx); - Reg(sp); - Reg(bp); - Reg(si); - Reg(di); - Reg(ip); - Reg(es); - Reg(cs); - Reg(ds); - Reg(ss); + Reg(ax); + Reg(cx); + Reg(dx); + Reg(bx); + Reg(sp); + Reg(bp); + Reg(si); + Reg(di); + Reg(ip); + Reg(es); + Reg(cs); + Reg(ds); + Reg(ss); #undef Reg - [reasons addObject:[NSString stringWithFormat: - @"registers don't match: %@", [registers componentsJoinedByString:@", "] - ]]; - } - if(!ramEqual) { - [reasons addObject:@"RAM contents don't match"]; - } - - failure.reason = std::string([reasons componentsJoinedByString:@"; "].UTF8String); - execution_failures.push_back(std::move(failure)); + [reasons addObject:[NSString stringWithFormat: + @"registers don't match: %@", [registers componentsJoinedByString:@", "] + ]]; } + if(!ramEqual) { + [reasons addObject:@"RAM contents don't match"]; + } + + failure.reason = std::string([reasons componentsJoinedByString:@"; "].UTF8String); + failure_list->push_back(std::move(failure)); } - (void)printFailures:(NSArray *)failures { @@ -744,20 +774,16 @@ struct FailedExecution { } // Lock in current failure rate. - XCTAssertLessThanOrEqual(execution_failures.size(), 4009); - - // Current accepted failures: - // * 2484 instances of LEA from a register, which officially has undefined results; - // * 42 instances of AAM 00h for which I can't figure out what to do with flags; and - // * 1486 instances of IDIV, most either with a rep or repne that on the 8086 specifically negatives the result, - // but some admittedly still unexplained (primarily setting overflow even though the result doesn't overflow; - // a couple of online 8086 emulators also didn't throw so maybe this is an 8086 quirk?) + XCTAssertLessThanOrEqual(execution_failures.size(), 0); for(const auto &failure: execution_failures) { NSLog(@"Failed %s — %s", failure.test_name.c_str(), failure.reason.c_str()); } + for(const auto &failure: permitted_failures) { + NSLog(@"Permitted failure of %s — %s", failure.test_name.c_str(), failure.reason.c_str()); + } - NSLog(@"Files with failures were: %@", failures); + NSLog(@"Files with failures, permitted or otherwise, were: %@", failures); } @end From e78e5c8101aa066ddfdd3b07f97a698d3ac8b428 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Thu, 9 Nov 2023 12:26:40 -0500 Subject: [PATCH 3/3] Add remaining acceptable error cases. --- InstructionSets/x86/Instruction.hpp | 3 ++ OSBindings/Mac/Clock SignalTests/8088Tests.mm | 36 ++++++++++++++++--- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/InstructionSets/x86/Instruction.hpp b/InstructionSets/x86/Instruction.hpp index 23239bb66..589e6d330 100644 --- a/InstructionSets/x86/Instruction.hpp +++ b/InstructionSets/x86/Instruction.hpp @@ -470,6 +470,9 @@ enum class Source: uint8_t { /// getter is used). IndirectNoBase = Indirect - 1, }; +constexpr bool is_register(Source source) { + return source < Source::None; +} enum class Repetition: uint8_t { None, RepE, RepNE, Rep, diff --git a/OSBindings/Mac/Clock SignalTests/8088Tests.mm b/OSBindings/Mac/Clock SignalTests/8088Tests.mm index f53a51688..0310acab2 100644 --- a/OSBindings/Mac/Clock SignalTests/8088Tests.mm +++ b/OSBindings/Mac/Clock SignalTests/8088Tests.mm @@ -658,11 +658,10 @@ struct FailedExecution { failure_list = &permitted_failures; } - // IDIV_REP has a couple of cases... + // IDIV_REP: for reasons I don't understand, sometimes the test set doesn't increment + // the IP across a REP_IDIV. I don't think (?) this correlates to real 8086 behaviour. + // More research required, but for now I'm not treating this as a roadblock. if(decoded.second.operation() == Operation::IDIV_REP) { - // For reasons I don't understand, sometimes the test set doesn't increment the IP - // across a REP_IDIV. I don't think (?) this correlates to real 8086 behaviour. - // More research required, but for now I'm not treating this as a roadblock. Registers advanced_registers = intended_registers; advanced_registers.ip_ += decoded.first; if(advanced_registers == execution_support.registers && ramEqual && flagsEqual) { @@ -670,6 +669,35 @@ struct FailedExecution { } } + // IDIV[_REP] byte: the test cases sometimes throw even when I can't see why they should, + // and other x86 emulations also don't throw. I guess — guess! — an 8086-specific oddity + // deviates from the x86 average here. So I'm also permitting these for now. + if( + decoded.second.operation_size() == InstructionSet::x86::DataSize::Byte && + (decoded.second.operation() == Operation::IDIV_REP || decoded.second.operation() == Operation::IDIV) + ) { + if(intended_registers.sp() == execution_support.registers.sp() - 6) { + Registers non_exception_registers = intended_registers; + non_exception_registers.ip() = execution_support.registers.ip(); + non_exception_registers.sp() = execution_support.registers.sp(); + non_exception_registers.ax() = execution_support.registers.ax(); + non_exception_registers.cs() = execution_support.registers.cs(); + + if(non_exception_registers == execution_support.registers) { + failure_list = &permitted_failures; + } + } + } + + // LEA from a register is undefined behaviour and throws on processors beyond the 8086. + if(decoded.second.operation() == Operation::LEA && InstructionSet::x86::is_register(decoded.second.source().source())) { + failure_list = &permitted_failures; + } + + if(failure_list == &execution_failures) { + printf("Fail: %d\n", int(decoded.second.operation())); + } + // Record a failure. FailedExecution failure; failure.instruction = decoded.second;