imagedev/floppy: fix a bug with reading the first 1-bit on a track (#12439)

`floppy_image_device::find_index` uses binary search to find the index for
which `buf[spos] <= position < buf[spos + 1]`. However, the algorithm behaves
incorrectly when `position < buf[0]`. In this case, the algorithm returns 0,
as if `position` was between `buf[0]` and `buf[1]`.

The effect of this is that if `get_next_transition` is called with a timestamp
that is between the start of the revolution and the first transition, then
instead of returning the timestamp of that transition, it returns the timestamp
of the second transition instead. Essentially, the first 1-bit on the track gets
flipped to a 0.

I have encountered this in Apple II emulation, where this bug manifests as
sporadic I/O errors.

Fix it by doing two things:

1. Replace `find_index` with a call to `upper_bound` from the standard library,
   which behaves correctly in edge cases.

2. If `upper_bound` signals that `position < buf[0]`, then adjust `base` and
   `index` to point to the last transition of the previous revolution.
This commit is contained in:
Roman Donchenko 2024-06-04 11:39:50 +03:00 committed by GitHub
parent 1ad6369866
commit bb6e1adb06
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 12 additions and 28 deletions

View File

@ -33,6 +33,8 @@
#include "util/ioprocsfilter.h"
#include "util/zippath.h"
#include <algorithm>
/*
Debugging flags. Set to 0 or 1.
*/
@ -1081,25 +1083,6 @@ uint32_t floppy_image_device::hash32(uint32_t a) const
return a;
}
int floppy_image_device::find_index(uint32_t position, const std::vector<uint32_t> &buf)const
{
int spos = (buf.size() >> 1)-1;
int step;
for(step=1; step<buf.size()+1; step<<=1) { }
step >>= 1;
for(;;) {
if(spos >= int(buf.size()) || (spos > 0 && (buf[spos] & floppy_image::TIME_MASK) > position)) {
spos -= step;
step >>= 1;
} else if(spos < 0 || (spos < int(buf.size())-1 && (buf[spos+1] & floppy_image::TIME_MASK) <= position)) {
spos += step;
step >>= 1;
} else
return spos;
}
}
uint32_t floppy_image_device::find_position(attotime &base, const attotime &when)
{
base = m_revolution_start_time;
@ -1170,16 +1153,18 @@ void floppy_image_device::cache_fill(const attotime &when)
attotime base;
uint32_t position = find_position(base, when);
int index = find_index(position, buf);
auto it = std::upper_bound(
buf.begin(), buf.end(), position,
[](uint32_t a, uint32_t b) {
return a < (b & floppy_image::TIME_MASK);
}
);
int index = int(it - buf.begin()) - 1;
if(index == -1) {
// I suspect this should be an abort(), to check...
m_cache_start_time = attotime::zero;
m_cache_end_time = attotime::never;
m_cache_index = 0;
m_cache_entry = buf[0];
cache_weakness_setup();
return;
base -= m_rev_time;
index = buf.size() - 1;
}
for(;;) {

View File

@ -256,7 +256,6 @@ protected:
void check_led();
uint32_t find_position(attotime &base, const attotime &when);
int find_index(uint32_t position, const std::vector<uint32_t> &buf) const;
attotime position_to_time(const attotime &base, int position) const;
void commit_image();