From edbc60a3fb9b0a22808b569a97a0439f0b21135c Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Tue, 17 Oct 2017 20:50:46 -0400 Subject: [PATCH 1/9] Various undefined behaviour fixes. Primarily around uninitialised variables, but also with an attempted use of a negative pointer. --- Machines/AmstradCPC/AmstradCPC.cpp | 30 ++++++++---------------- Outputs/CRT/Internals/ArrayBuilder.cpp | 4 +--- Outputs/CRT/Internals/ArrayBuilder.hpp | 12 +++++----- Outputs/CRT/Internals/TextureBuilder.cpp | 10 ++++---- 4 files changed, 22 insertions(+), 34 deletions(-) diff --git a/Machines/AmstradCPC/AmstradCPC.cpp b/Machines/AmstradCPC/AmstradCPC.cpp index 4c04db4c4..78486a1d8 100644 --- a/Machines/AmstradCPC/AmstradCPC.cpp +++ b/Machines/AmstradCPC/AmstradCPC.cpp @@ -155,18 +155,8 @@ class AYDeferrer { class CRTCBusHandler { public: CRTCBusHandler(uint8_t *ram, InterruptTimer &interrupt_timer) : - cycles_(0), - was_enabled_(false), - was_sync_(false), - pixel_data_(nullptr), - pixel_pointer_(nullptr), - was_hsync_(false), ram_(ram), - interrupt_timer_(interrupt_timer), - pixel_divider_(1), - mode_(2), - next_mode_(2), - cycles_into_hsync_(0) { + interrupt_timer_(interrupt_timer) { establish_palette_hits(); build_mode_table(); } @@ -500,19 +490,19 @@ class CRTCBusHandler { return mapping[colour]; } - unsigned int cycles_; + unsigned int cycles_ = 0; - bool was_enabled_, was_sync_, was_hsync_, was_vsync_; - int cycles_into_hsync_; + bool was_enabled_ = false, was_sync_ = false, was_hsync_ = false, was_vsync_ = false; + int cycles_into_hsync_ = 0; std::shared_ptr crt_; - uint8_t *pixel_data_, *pixel_pointer_; + uint8_t *pixel_data_ = nullptr, *pixel_pointer_ = nullptr; - uint8_t *ram_; + uint8_t *ram_ = nullptr; - int next_mode_, mode_; + int next_mode_ = 2, mode_ = 2; - unsigned int pixel_divider_; + unsigned int pixel_divider_ = 1; uint16_t mode0_output_[256]; uint32_t mode1_output_[256]; uint64_t mode2_output_[256]; @@ -522,9 +512,9 @@ class CRTCBusHandler { std::vector mode1_palette_hits_[4]; std::vector mode3_palette_hits_[4]; - int pen_; + int pen_ = 0; uint8_t palette_[16]; - uint8_t border_; + uint8_t border_ = 0; InterruptTimer &interrupt_timer_; }; diff --git a/Outputs/CRT/Internals/ArrayBuilder.cpp b/Outputs/CRT/Internals/ArrayBuilder.cpp index 26216834c..1f09602f5 100644 --- a/Outputs/CRT/Internals/ArrayBuilder.cpp +++ b/Outputs/CRT/Internals/ArrayBuilder.cpp @@ -67,9 +67,7 @@ ArrayBuilder::Submission ArrayBuilder::submit() { } ArrayBuilder::Buffer::Buffer(size_t size, std::function submission_function) : - is_full(false), - submission_function_(submission_function), - allocated_data(0), flushed_data(0), submitted_data(0) { + submission_function_(submission_function) { if(!submission_function_) { glGenBuffers(1, &buffer); glBindBuffer(GL_ARRAY_BUFFER, buffer); diff --git a/Outputs/CRT/Internals/ArrayBuilder.hpp b/Outputs/CRT/Internals/ArrayBuilder.hpp index 1c02e5e68..adaf384ae 100644 --- a/Outputs/CRT/Internals/ArrayBuilder.hpp +++ b/Outputs/CRT/Internals/ArrayBuilder.hpp @@ -82,17 +82,17 @@ class ArrayBuilder { void reset(); private: - bool is_full; - GLuint buffer; + bool is_full = false; + GLuint buffer = 0; std::function submission_function_; std::vector data; - size_t allocated_data; - size_t flushed_data; - size_t submitted_data; + size_t allocated_data = 0; + size_t flushed_data = 0; + size_t submitted_data = 0; } output_, input_; uint8_t *get_storage(size_t size, Buffer &buffer); - bool is_full_; + bool is_full_ = false; }; } diff --git a/Outputs/CRT/Internals/TextureBuilder.cpp b/Outputs/CRT/Internals/TextureBuilder.cpp index 7fa46d955..945b440b1 100644 --- a/Outputs/CRT/Internals/TextureBuilder.cpp +++ b/Outputs/CRT/Internals/TextureBuilder.cpp @@ -99,13 +99,13 @@ void TextureBuilder::reduce_previous_allocation_to(size_t actual_length) { // against rounding errors when this run is drawn. // TODO: allow somebody else to specify the rule for generating a left-padding value and // a right-padding value. - uint8_t *start_pointer = pointer_to_location(write_area_.x, write_area_.y); - memcpy( &start_pointer[-bytes_per_pixel_], - start_pointer, + uint8_t *start_pointer = pointer_to_location(write_area_.x, write_area_.y) - bytes_per_pixel_; + memcpy( start_pointer, + &start_pointer[bytes_per_pixel_], bytes_per_pixel_); - memcpy( &start_pointer[actual_length * bytes_per_pixel_], - &start_pointer[(actual_length - 1) * bytes_per_pixel_], + memcpy( &start_pointer[(actual_length + 1) * bytes_per_pixel_], + &start_pointer[actual_length * bytes_per_pixel_], bytes_per_pixel_); } From ce78d9d12c5daef408f8d7bbd9dc0a887fbefb11 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Tue, 17 Oct 2017 22:09:48 -0400 Subject: [PATCH 2/9] Introduces buffer alignment when writing to textures. To avoid cross-boundary writes and hopefully to eke out a little better performance. --- Machines/AmstradCPC/AmstradCPC.cpp | 2 +- Machines/Electron/Video.cpp | 2 +- Outputs/CRT/CRT.hpp | 4 ++-- Outputs/CRT/Internals/TextureBuilder.cpp | 8 +++++--- Outputs/CRT/Internals/TextureBuilder.hpp | 7 ++++--- 5 files changed, 13 insertions(+), 10 deletions(-) diff --git a/Machines/AmstradCPC/AmstradCPC.cpp b/Machines/AmstradCPC/AmstradCPC.cpp index 78486a1d8..6b8e7f2b9 100644 --- a/Machines/AmstradCPC/AmstradCPC.cpp +++ b/Machines/AmstradCPC/AmstradCPC.cpp @@ -207,7 +207,7 @@ class CRTCBusHandler { // collect some more pixels if output is ongoing if(!is_sync && state.display_enable) { if(!pixel_data_) { - pixel_pointer_ = pixel_data_ = crt_->allocate_write_area(320); + pixel_pointer_ = pixel_data_ = crt_->allocate_write_area(320, 8); } if(pixel_pointer_) { // the CPC shuffles output lines as: diff --git a/Machines/Electron/Video.cpp b/Machines/Electron/Video.cpp index dabac72cd..faa1d410e 100644 --- a/Machines/Electron/Video.cpp +++ b/Machines/Electron/Video.cpp @@ -106,7 +106,7 @@ void VideoOutput::output_pixels(unsigned int number_of_cycles) { if(!initial_output_target_ || divider != current_output_divider_) { if(current_output_target_) crt_->output_data((unsigned int)((current_output_target_ - initial_output_target_) * current_output_divider_), current_output_divider_); current_output_divider_ = divider; - initial_output_target_ = current_output_target_ = crt_->allocate_write_area(640 / current_output_divider_); + initial_output_target_ = current_output_target_ = crt_->allocate_write_area(640 / current_output_divider_, 4); } #define get_pixel() \ diff --git a/Outputs/CRT/CRT.hpp b/Outputs/CRT/CRT.hpp index 19d890ec3..ae26fbc8a 100644 --- a/Outputs/CRT/CRT.hpp +++ b/Outputs/CRT/CRT.hpp @@ -221,9 +221,9 @@ class CRT { @param required_length The number of samples to allocate. @returns A pointer to the allocated area if room is available; @c nullptr otherwise. */ - inline uint8_t *allocate_write_area(size_t required_length) { + inline uint8_t *allocate_write_area(size_t required_length, size_t required_alignment = 1) { std::unique_lock output_lock = openGL_output_builder_.get_output_lock(); - return openGL_output_builder_.texture_builder.allocate_write_area(required_length); + return openGL_output_builder_.texture_builder.allocate_write_area(required_length, required_alignment); } /*! Causes appropriate OpenGL or OpenGL ES calls to be issued in order to draw the current CRT state. diff --git a/Outputs/CRT/Internals/TextureBuilder.cpp b/Outputs/CRT/Internals/TextureBuilder.cpp index 945b440b1..9e5d5003f 100644 --- a/Outputs/CRT/Internals/TextureBuilder.cpp +++ b/Outputs/CRT/Internals/TextureBuilder.cpp @@ -60,7 +60,7 @@ inline uint8_t *TextureBuilder::pointer_to_location(uint16_t x, uint16_t y) { return &image_[((y * InputBufferBuilderWidth) + x) * bytes_per_pixel_]; } -uint8_t *TextureBuilder::allocate_write_area(size_t required_length) { +uint8_t *TextureBuilder::allocate_write_area(size_t required_length, size_t required_alignment) { // Keep a flag to indicate whether the buffer was full at allocate_write_area; if it was then // don't return anything now, and decline to act upon follow-up methods. is_full_ may be reset // by asynchronous calls to submit. was_full_ will not be touched by it. @@ -69,8 +69,10 @@ uint8_t *TextureBuilder::allocate_write_area(size_t required_length) { // If there's not enough space on this line, move to the next. If the next is where the current // submission group started, trigger is/was_full_ and return nothing. - if(write_areas_start_x_ + required_length + 2 > InputBufferBuilderWidth) { + size_t alignment_offset = (required_alignment - ((write_areas_start_x_ + 1) % required_alignment)) % required_alignment; + if(write_areas_start_x_ + required_length + 2 + alignment_offset > InputBufferBuilderWidth) { write_areas_start_x_ = 0; + alignment_offset = (required_alignment - 1) % required_alignment; write_areas_start_y_ = (write_areas_start_y_ + 1) % InputBufferBuilderHeight; if(write_areas_start_y_ == first_unsubmitted_y_) { @@ -80,7 +82,7 @@ uint8_t *TextureBuilder::allocate_write_area(size_t required_length) { } // Queue up the latest write area. - write_area_.x = write_areas_start_x_ + 1; + write_area_.x = write_areas_start_x_ + 1 + static_cast(alignment_offset); write_area_.y = write_areas_start_y_; write_area_.length = (uint16_t)required_length; diff --git a/Outputs/CRT/Internals/TextureBuilder.hpp b/Outputs/CRT/Internals/TextureBuilder.hpp index 476e1d04b..f0b211686 100644 --- a/Outputs/CRT/Internals/TextureBuilder.hpp +++ b/Outputs/CRT/Internals/TextureBuilder.hpp @@ -66,10 +66,11 @@ class TextureBuilder { TextureBuilder(size_t bytes_per_pixel, GLenum texture_unit); virtual ~TextureBuilder(); - /// Finds the first available space of at least @c required_length pixels in size. Calls must be paired off - /// with calls to @c reduce_previous_allocation_to. + /// Finds the first available space of at least @c required_length pixels in size which is suitably aligned + /// for writing of @c required_alignment number of pixels at a time. + /// Calls must be paired off with calls to @c reduce_previous_allocation_to. /// @returns a pointer to the allocated space if any was available; @c nullptr otherwise. - uint8_t *allocate_write_area(size_t required_length); + uint8_t *allocate_write_area(size_t required_length, size_t required_alignment = 1); /// Announces that the owner is finished with the region created by the most recent @c allocate_write_area /// and indicates that its actual final size was @c actual_length. From 3944e734d36ccf20ecabbe5d8c42e01739e72c12 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Tue, 17 Oct 2017 22:10:28 -0400 Subject: [PATCH 3/9] Ensures full 6845 instance state initialisation and uses an unsigned shifter. --- Components/6845/CRTC6845.hpp | 42 ++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/Components/6845/CRTC6845.hpp b/Components/6845/CRTC6845.hpp index 2544ada59..6370408f3 100644 --- a/Components/6845/CRTC6845.hpp +++ b/Components/6845/CRTC6845.hpp @@ -18,12 +18,12 @@ namespace Motorola { namespace CRTC { struct BusState { - bool display_enable; - bool hsync; - bool vsync; - bool cursor; - uint16_t refresh_address; - uint16_t row_address; + bool display_enable = false; + bool hsync = false; + bool vsync = false; + bool cursor = false; + uint16_t refresh_address = 0; + uint16_t row_address = 0; }; class BusHandler { @@ -167,8 +167,8 @@ template class CRTC6845 { private: inline void perform_bus_cycle_phase1() { // Skew theory of operation: keep a history of the last three states, and apply whichever is selected. - character_is_visible_shifter_ = (character_is_visible_shifter_ << 1) | static_cast(character_is_visible_); - bus_state_.display_enable = (character_is_visible_shifter_ & display_skew_mask_) && line_is_visible_; + character_is_visible_shifter_ = (character_is_visible_shifter_ << 1) | static_cast(character_is_visible_); + bus_state_.display_enable = (static_cast(character_is_visible_shifter_) & display_skew_mask_) && line_is_visible_; bus_handler_.perform_bus_cycle_phase1(bus_state_); } @@ -248,25 +248,25 @@ template class CRTC6845 { T &bus_handler_; BusState bus_state_; - uint8_t registers_[18]; - uint8_t dummy_register_; - int selected_register_; + uint8_t registers_[18] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}; + uint8_t dummy_register_ = 0; + int selected_register_ = 0; - uint8_t character_counter_; - uint8_t line_counter_; + uint8_t character_counter_ = 0; + uint8_t line_counter_ = 0; - bool character_is_visible_, line_is_visible_; + bool character_is_visible_ = false, line_is_visible_ = false; - int hsync_counter_; - int vsync_counter_; - bool is_in_adjustment_period_; + int hsync_counter_ = 0; + int vsync_counter_ = 0; + bool is_in_adjustment_period_ = false; - uint16_t line_address_; - uint16_t end_of_line_address_; - uint8_t status_; + uint16_t line_address_ = 0; + uint16_t end_of_line_address_ = 0; + uint8_t status_ = 0; int display_skew_mask_ = 1; - int character_is_visible_shifter_ = 0; + unsigned int character_is_visible_shifter_ = 0; }; } From 91b867a7b3091a363dc8ad0ec0d2991838dbad0e Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Tue, 17 Oct 2017 22:11:01 -0400 Subject: [PATCH 4/9] Ensures full 8272 instance state initialisation. --- Components/8272/i8272.cpp | 9 +----- Components/8272/i8272.hpp | 58 ++++++++++++++++++--------------------- 2 files changed, 28 insertions(+), 39 deletions(-) diff --git a/Components/8272/i8272.cpp b/Components/8272/i8272.cpp index f30add8e9..c1aa2a5d2 100644 --- a/Components/8272/i8272.cpp +++ b/Components/8272/i8272.cpp @@ -79,14 +79,7 @@ namespace { i8272::i8272(BusHandler &bus_handler, Cycles clock_rate) : Storage::Disk::MFMController(clock_rate), - bus_handler_(bus_handler), - main_status_(0), - interesting_event_mask_((int)Event8272::CommandByte), - resume_point_(0), - delay_time_(0), - head_timers_running_(0), - expects_input_(false), - drives_seeking_(0) { + bus_handler_(bus_handler) { posit_event((int)Event8272::CommandByte); } diff --git a/Components/8272/i8272.hpp b/Components/8272/i8272.hpp index 741972b4c..a11f81c00 100644 --- a/Components/8272/i8272.hpp +++ b/Components/8272/i8272.hpp @@ -50,15 +50,15 @@ class i8272: public Storage::Disk::MFMController { std::unique_ptr allocated_bus_handler_; // Status registers. - uint8_t main_status_; - uint8_t status_[3]; + uint8_t main_status_ = 0; + uint8_t status_[3] = {0, 0, 0}; // A buffer for accumulating the incoming command, and one for accumulating the result. std::vector command_; std::vector result_stack_; - uint8_t input_; - bool has_input_; - bool expects_input_; + uint8_t input_ = 0; + bool has_input_ = false; + bool expects_input_ = false; // Event stream: the 8272-specific events, plus the current event state. enum class Event8272: int { @@ -68,41 +68,37 @@ class i8272: public Storage::Disk::MFMController { NoLongerReady = (1 << 6) }; void posit_event(int type); - int interesting_event_mask_; - int resume_point_; - bool is_access_command_; + int interesting_event_mask_ = (int)Event8272::CommandByte; + int resume_point_ = 0; + bool is_access_command_ = false; // The counter used for ::Timer events. - int delay_time_; + int delay_time_ = 0; // The connected drives. struct Drive { - uint8_t head_position; + uint8_t head_position = 0; // Seeking: persistent state. enum Phase { NotSeeking, Seeking, CompletedSeeking - } phase; - bool did_seek; - bool seek_failed; + } phase = NotSeeking; + bool did_seek = false; + bool seek_failed = false; // Seeking: transient state. - int step_rate_counter; - int steps_taken; - int target_head_position; // either an actual number, or -1 to indicate to step until track zero + int step_rate_counter = 0; + int steps_taken = 0; + int target_head_position = 0; // either an actual number, or -1 to indicate to step until track zero // Head state. - int head_unload_delay[2]; - bool head_is_loaded[2]; + int head_unload_delay[2] = {0, 0}; + bool head_is_loaded[2] = {false, false}; - Drive() : - head_position(0), phase(NotSeeking), - head_is_loaded{false, false}, - head_unload_delay{0, 0} {}; } drives_[4]; - int drives_seeking_; + int drives_seeking_ = 0; /// @returns @c true if the selected drive, which is number @c drive, can stop seeking. bool seek_is_satisfied(int drive); @@ -115,22 +111,22 @@ class i8272: public Storage::Disk::MFMController { bool is_executing_ = false; // A count of head unload timers currently running. - int head_timers_running_; + int head_timers_running_ = 0; // Transient storage and counters used while reading the disk. - uint8_t header_[6]; - int distance_into_section_; - int index_hole_count_, index_hole_limit_; + uint8_t header_[6] = {0, 0, 0, 0, 0, 0}; + int distance_into_section_ = 0; + int index_hole_count_ = 0, index_hole_limit_ = 0; // Keeps track of the drive and head in use during commands. - int active_drive_; - int active_head_; + int active_drive_ = 0; + int active_head_ = 0; // Internal registers. - uint8_t cylinder_, head_, sector_, size_; + uint8_t cylinder_ = 0, head_ = 0, sector_ = 0, size_ = 0; // Master switch on not performing any work. - bool is_sleeping_; + bool is_sleeping_ = false; }; } From 2d7a4fe5f0366a965a651acfe416509e588ea635 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Tue, 17 Oct 2017 22:12:04 -0400 Subject: [PATCH 5/9] Switches the MFM shifter to unsigned accumulation. Since left shifting signed numbers is undefined behaviour. --- Storage/Disk/Encodings/MFM/Shifter.cpp | 2 +- Storage/Disk/Encodings/MFM/Shifter.hpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Storage/Disk/Encodings/MFM/Shifter.cpp b/Storage/Disk/Encodings/MFM/Shifter.cpp index 2c4437e3b..c0dc25803 100644 --- a/Storage/Disk/Encodings/MFM/Shifter.cpp +++ b/Storage/Disk/Encodings/MFM/Shifter.cpp @@ -24,7 +24,7 @@ void Shifter::set_should_obey_syncs(bool should_obey_syncs) { } void Shifter::add_input_bit(int value) { - shift_register_ = (shift_register_ << 1) | value; + shift_register_ = (shift_register_ << 1) | static_cast(value); bits_since_token_++; token_ = Token::None; diff --git a/Storage/Disk/Encodings/MFM/Shifter.hpp b/Storage/Disk/Encodings/MFM/Shifter.hpp index 54b315b3e..5ceca2edd 100644 --- a/Storage/Disk/Encodings/MFM/Shifter.hpp +++ b/Storage/Disk/Encodings/MFM/Shifter.hpp @@ -40,10 +40,10 @@ class Shifter { private: // Bit stream input state int bits_since_token_ = 0; - int shift_register_ = 0; + unsigned int shift_register_ = 0; bool is_awaiting_marker_value_ = false; - bool should_obey_syncs_; - Token token_; + bool should_obey_syncs_ = true; + Token token_ = None; // input configuration bool is_double_density_ = false; From 7f2febeec98480738f0ea7987e94f71a0ee1e7e1 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Tue, 17 Oct 2017 22:13:37 -0400 Subject: [PATCH 6/9] Ensures complete DPLL initial state assignment. --- Storage/Disk/DPLL/DigitalPhaseLockedLoop.cpp | 8 ++------ Storage/Disk/DPLL/DigitalPhaseLockedLoop.hpp | 16 ++++++++-------- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/Storage/Disk/DPLL/DigitalPhaseLockedLoop.cpp b/Storage/Disk/DPLL/DigitalPhaseLockedLoop.cpp index 8348049c9..ec3c5bbb1 100644 --- a/Storage/Disk/DPLL/DigitalPhaseLockedLoop.cpp +++ b/Storage/Disk/DPLL/DigitalPhaseLockedLoop.cpp @@ -13,13 +13,9 @@ using namespace Storage; DigitalPhaseLockedLoop::DigitalPhaseLockedLoop(int clocks_per_bit, size_t length_of_history) : - clocks_per_bit_(clocks_per_bit), - phase_(0), - window_length_(clocks_per_bit), - offset_history_pointer_(0), offset_history_(length_of_history, 0), - offset_(0), - delegate_(nullptr) {} + window_length_(clocks_per_bit), + clocks_per_bit_(clocks_per_bit) {} void DigitalPhaseLockedLoop::run_for(const Cycles cycles) { offset_ += cycles.as_int(); diff --git a/Storage/Disk/DPLL/DigitalPhaseLockedLoop.hpp b/Storage/Disk/DPLL/DigitalPhaseLockedLoop.hpp index 9cd081252..ecce97bbe 100644 --- a/Storage/Disk/DPLL/DigitalPhaseLockedLoop.hpp +++ b/Storage/Disk/DPLL/DigitalPhaseLockedLoop.hpp @@ -50,20 +50,20 @@ class DigitalPhaseLockedLoop { } private: - Delegate *delegate_; + Delegate *delegate_ = nullptr; void post_phase_offset(int phase, int offset); std::vector offset_history_; - size_t offset_history_pointer_; - int offset_; + size_t offset_history_pointer_ = 0; + int offset_ = 0; - int phase_; - int window_length_; - bool window_was_filled_; + int phase_ = 0; + int window_length_ = 0; + bool window_was_filled_ = false; - int clocks_per_bit_; - int tolerance_; + int clocks_per_bit_ = 0; + int tolerance_ = 0; }; } From 2c1e99858b51a4e24301e5abc37c4373a4e57916 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Tue, 17 Oct 2017 22:22:51 -0400 Subject: [PATCH 7/9] Fixed HalfCycles to allow conversion from Cycles without relying on undefined behaviour. Specifically: left shifting a negative number. --- ClockReceiver/ClockReceiver.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ClockReceiver/ClockReceiver.hpp b/ClockReceiver/ClockReceiver.hpp index 097b1c6db..a6f41b5f9 100644 --- a/ClockReceiver/ClockReceiver.hpp +++ b/ClockReceiver/ClockReceiver.hpp @@ -161,7 +161,7 @@ class HalfCycles: public WrappedInt { inline HalfCycles(int l) : WrappedInt(l) {} inline HalfCycles() : WrappedInt() {} - inline HalfCycles(const Cycles cycles) : WrappedInt(cycles.as_int() << 1) {} + inline HalfCycles(const Cycles cycles) : WrappedInt(cycles.as_int() * 2) {} inline HalfCycles(const HalfCycles &half_cycles) : WrappedInt(half_cycles.length_) {} /// @returns The number of whole cycles completely covered by this span of half cycles. From ba5f6683384654594d741f1a6f4570e5081f9d69 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Tue, 17 Oct 2017 22:34:10 -0400 Subject: [PATCH 8/9] Ensured full CRT instance initialisation. --- Outputs/CRT/CRT.cpp | 7 +------ Outputs/CRT/CRT.hpp | 34 +++++++++++++++++----------------- 2 files changed, 18 insertions(+), 23 deletions(-) diff --git a/Outputs/CRT/CRT.cpp b/Outputs/CRT/CRT.cpp index e75648445..6acf8f07b 100644 --- a/Outputs/CRT/CRT.cpp +++ b/Outputs/CRT/CRT.cpp @@ -90,13 +90,8 @@ void CRT::update_gamma() { } CRT::CRT(unsigned int common_output_divisor, unsigned int buffer_depth) : - is_receiving_sync_(false), common_output_divisor_(common_output_divisor), - is_writing_composite_run_(false), - delegate_(nullptr), - frames_since_last_delegate_call_(0), - openGL_output_builder_(buffer_depth), - is_alernate_line_(false) {} + openGL_output_builder_(buffer_depth) {} CRT::CRT( unsigned int cycles_per_line, unsigned int common_output_divisor, diff --git a/Outputs/CRT/CRT.hpp b/Outputs/CRT/CRT.hpp index ae26fbc8a..cc6c6b903 100644 --- a/Outputs/CRT/CRT.hpp +++ b/Outputs/CRT/CRT.hpp @@ -33,12 +33,12 @@ class CRT { // the incoming clock lengths will be multiplied by something to give at least 1000 // sample points per line - unsigned int time_multiplier_; - const unsigned int common_output_divisor_; + unsigned int time_multiplier_ = 1; + const unsigned int common_output_divisor_ = 1; // the two flywheels regulating scanning std::unique_ptr horizontal_flywheel_, vertical_flywheel_; - uint16_t vertical_flywheel_output_divider_; + uint16_t vertical_flywheel_output_divider_ = 1; struct Scan { enum Type { @@ -53,11 +53,11 @@ class CRT { }; void output_scan(const Scan *scan); - uint8_t colour_burst_phase_, colour_burst_amplitude_, colour_burst_phase_adjustment_; - bool is_writing_composite_run_; + uint8_t colour_burst_phase_ = 0, colour_burst_amplitude_ = 30, colour_burst_phase_adjustment_ = 0; + bool is_writing_composite_run_ = false; - unsigned int phase_denominator_, phase_numerator_, colour_cycle_numerator_; - bool is_alernate_line_, phase_alternates_; + unsigned int phase_denominator_ = 1, phase_numerator_ = 1, colour_cycle_numerator_ = 1; + bool is_alernate_line_ = false, phase_alternates_ = false; // the outer entry point for dispatching output_sync, output_blank, output_level and output_data void advance_cycles(unsigned int number_of_cycles, bool hsync_requested, bool vsync_requested, const Scan::Type type); @@ -76,8 +76,8 @@ class CRT { } output_run_; // the delegate - Delegate *delegate_; - unsigned int frames_since_last_delegate_call_; + Delegate *delegate_ = nullptr; + unsigned int frames_since_last_delegate_call_ = 0; // queued tasks for the OpenGL queue; performed before the next draw std::mutex function_mutex_; @@ -88,16 +88,16 @@ class CRT { } // sync counter, for determining vertical sync - bool is_receiving_sync_; // true if the CRT is currently receiving sync (i.e. this is for edge triggering of horizontal sync) - bool is_accumulating_sync_; // true if a sync level has triggered the suspicion that a vertical sync might be in progress - bool is_refusing_sync_; // true once a vertical sync has been detected, until a prolonged period of non-sync has ended suspicion of an ongoing vertical sync - unsigned int sync_capacitor_charge_threshold_; // this charges up during times of sync and depletes otherwise; needs to hit a required threshold to trigger a vertical sync - unsigned int cycles_of_sync_; // the number of cycles since the potential vertical sync began - unsigned int cycles_since_sync_; // the number of cycles since last in sync, for defeating the possibility of this being a vertical sync + bool is_receiving_sync_ = false; // true if the CRT is currently receiving sync (i.e. this is for edge triggering of horizontal sync) + bool is_accumulating_sync_ = false; // true if a sync level has triggered the suspicion that a vertical sync might be in progress + bool is_refusing_sync_ = false; // true once a vertical sync has been detected, until a prolonged period of non-sync has ended suspicion of an ongoing vertical sync + unsigned int sync_capacitor_charge_threshold_ = 0; // this charges up during times of sync and depletes otherwise; needs to hit a required threshold to trigger a vertical sync + unsigned int cycles_of_sync_ = 0; // the number of cycles since the potential vertical sync began + unsigned int cycles_since_sync_ = 0; // the number of cycles since last in sync, for defeating the possibility of this being a vertical sync - unsigned int cycles_per_line_; + unsigned int cycles_per_line_ = 1; - float input_gamma_, output_gamma_; + float input_gamma_ = 1.0f, output_gamma_ = 1.0f; void update_gamma(); public: From 7c8e830b90d4e23f10e0c7e30ca00c59dfd785d3 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Tue, 17 Oct 2017 22:34:49 -0400 Subject: [PATCH 9/9] Adjusted the Acorn tape parser to avoid signed left shifts. --- Storage/Tape/Parsers/Acorn.cpp | 10 +++++----- Storage/Tape/Parsers/Acorn.hpp | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Storage/Tape/Parsers/Acorn.cpp b/Storage/Tape/Parsers/Acorn.cpp index 80a7b14d0..3bcb2f496 100644 --- a/Storage/Tape/Parsers/Acorn.cpp +++ b/Storage/Tape/Parsers/Acorn.cpp @@ -41,14 +41,14 @@ int Parser::get_next_byte(const std::shared_ptr &tape) { return value; } -int Parser::get_next_short(const std::shared_ptr &tape) { - int result = get_next_byte(tape); - result |= get_next_byte(tape) << 8; +unsigned int Parser::get_next_short(const std::shared_ptr &tape) { + unsigned int result = static_cast(get_next_byte(tape)); + result |= static_cast(get_next_byte(tape)) << 8; return result; } -int Parser::get_next_word(const std::shared_ptr &tape) { - int result = get_next_short(tape); +unsigned int Parser::get_next_word(const std::shared_ptr &tape) { + unsigned int result = get_next_short(tape); result |= get_next_short(tape) << 8; return result; } diff --git a/Storage/Tape/Parsers/Acorn.hpp b/Storage/Tape/Parsers/Acorn.hpp index 5a749d23b..102c877b0 100644 --- a/Storage/Tape/Parsers/Acorn.hpp +++ b/Storage/Tape/Parsers/Acorn.hpp @@ -53,8 +53,8 @@ class Parser: public Storage::Tape::Parser, public Shifter::Delegate int get_next_bit(const std::shared_ptr &tape); int get_next_byte(const std::shared_ptr &tape); - int get_next_short(const std::shared_ptr &tape); - int get_next_word(const std::shared_ptr &tape); + unsigned int get_next_short(const std::shared_ptr &tape); + unsigned int get_next_word(const std::shared_ptr &tape); void reset_crc(); uint16_t get_crc();