Version: 1.2.0_devel (using KDE 3.4.3, Gentoo) Compiler: gcc version 3.3.6 (Gentoo 3.3.6, ssp-3.3.6-1.0, pie-8.7.8) OS: Linux (i686) release 2.6.14-gentoo-r5 Load a dirfile (eg, from dirfile_maker). In the wizard, load fields. Put the starting frame past the end of the file. When you apply, kst crashes with the following assertion failure: kst: /home/cbn/programs/KDE/graphics/kst/kst/kstrvector.cpp:579: KstObject::UpdateType KstRVector::doUpdate(bool): Assertion `new_nf - NF - 1 > 0 || new_nf - NF - 1 == -1' failed.
> kst: /home/cbn/programs/KDE/graphics/kst/kst/kstrvector.cpp:579: > KstObject::UpdateType KstRVector::doUpdate(bool): Assertion `new_nf - NF - > 1 > 0 || new_nf - NF - 1 == -1' failed. Do you know what the values of new_nf and NF are? (can you find out?)
new_nf 1 NF 0 new_f0 0
It would appear that the asserts are invalid, and that the subsequent _file->readField(...) calls should be protected by some if statements checking for the values of new_nf, NF, and new_f0 (as is the case for the samplesPerFrame = 1).
On Thursday 26 January 2006 13:39, Andrew Walker wrote: > ------- It would appear that the asserts are invalid, and that the > subsequent _file->readField(...) calls should be protected by some if > statements checking for the values of new_nf, NF, and new_f0 (as is the > case for the samplesPerFrame = 1). Why?
The ascii data source doesn't crash under the same conditions and the only difference is the samplesPerFrame value. It seems clear that the asserts are unnecessarily causing a crash.
On Thursday 26 January 2006 14:10, Andrew Walker wrote: > ------- The ascii data source doesn't crash under the same conditions and > the only difference is the samplesPerFrame value. It seems clear that the > asserts are unnecessarily causing a crash. a) asserts don't cause crashes, they cause signals which are not typically caught, and are not emitted in release builds b) those asserts were added because they were catching improper behavior by datasources and/or Kst c) "because it crashes the app" is not a reason why the assert is wrong.
George, did you want to fix this or shall I?
On Thursday 26 January 2006 14:48, Andrew Walker wrote: > ------- George, did you want to fix this or shall I? I can look at it once the view object painting rework is done, reviewed, and committed. Anyone else is free to submit a patch. However I won't approve an unjustified patch that just removes the asserts and converts them to runtime checks.
The situation is that there is no valid data in the sample range requested by the user. What is the correct behavior? -read the requested number of points, but count from the end? -read nothing and leave the vector empty (can vectors/curves handle 0 length vectors?)
On Thursday 26 January 2006 18:39, netterfield@astro.utoronto.ca wrote: > 00:39 ------- The situation is that there is no valid data in the sample > range requested by the user. What is the correct behavior? -read the > requested number of points, but count from the end? I don't think we should read something that the user didn't ask for. > -read nothing and leave the vector empty (can vectors/curves handle 0 > length vectors?) Exactly, this is [one of]the problem[s]. We cannot have 0 length vectors. Historical assumption is that length of vectors is always >= 1. (Used to even be >= 2.)
Around line 461 there is this: if (new_nf <= 0) { // Tried to read starting past the end. new_f0 = 0; new_nf = 1; } Is it being executed? If so, maybe it needs to be more robust. I think if we have a read-from-past end case, we need to, at the top, bail out and set the size to 0, filled with NAN. It's a nonsense case. (and we should leave the code down lower as-is so we can catch errors)
OK. I think Andrew is right. As the code was designed, the first assert is invalid: assert(new_nf - NF - 1 > 0 || new_nf - NF - 1 == -1); should be assert(new_nf - NF - 1 >=0 || new_nf - NF - 1 == -1); or not be there at all. What the code was suppose to do, on a start past eof, is to read only the first sample in the file. Of course George is also right - this is not the most clever of all possible solutions. In an attempt to be more clever, the patch below detects the start past eof condition, and makes a 1 sample long vector which contains only NaN. I have not fully tested what happens in all cases when you try to make curves, equations, psds, etc, of vectors with only 1 sample, which is a NaN, but at first try, it seems ok (but so does deleting the assert). Index: kstrvector.cpp =================================================================== --- kstrvector.cpp (revision 502941) +++ kstrvector.cpp (working copy) @@ -430,7 +430,8 @@ int i, k, shift, n_read=0; int ave_nread; int new_f0, new_nf; - + bool start_past_eof = false; + checkIntegrity(); if (DoSkip && Skip < 2 && SPF == 1) { @@ -462,6 +463,7 @@ // Tried to read starting past the end. new_f0 = 0; new_nf = 1; + start_past_eof = true; } } @@ -575,7 +577,10 @@ } // read the new data from file - if (_file->samplesPerFrame(_field) > 1) { + if (start_past_eof) { + _v[0] = NAN; + n_read = 1; + } else if (_file->samplesPerFrame(_field) > 1) { assert(new_nf - NF - 1 > 0 || new_nf - NF - 1 == -1); assert(new_f0 + NF >= 0); assert(new_f0 + new_nf - 1 >= 0);
This patch looks correct. Reviewed=me. Leaving the asserts there will help us catch regressions later. On Friday 27 January 2006 11:55, netterfield@astro.utoronto.ca wrote: > Index: kstrvector.cpp > =================================================================== > --- kstrvector.cpp (revision 502941) > +++ kstrvector.cpp (working copy) > @ -430,7 +430,8 @ > int i, k, shift, n_read=0; > int ave_nread; > int new_f0, new_nf; > - > + bool start_past_eof = false; > + > checkIntegrity(); > > if (DoSkip && Skip < 2 && SPF == 1) { > @ -462,6 +463,7 @ > // Tried to read starting past the end. > new_f0 = 0; > new_nf = 1; > + start_past_eof = true; > } > } > > @ -575,7 +577,10 @ > } > > // read the new data from file > - if (_file->samplesPerFrame(_field) > 1) { > + if (start_past_eof) { > + _v[0] = NAN; > + n_read = 1; > + } else if (_file->samplesPerFrame(_field) > 1) { > assert(new_nf - NF - 1 > 0 || new_nf - NF - 1 == -1); > assert(new_f0 + NF >= 0); > assert(new_f0 + new_nf - 1 >= 0); > _______________________________________________ > Kst mailing list > Kst@kde.org > https://mail.kde.org/mailman/listinfo/kst
SVN commit 503057 by netterfield: BUG: 120823 Read past EOF results in a vector of 1 sample = NaN. M +7 -2 kstrvector.cpp --- trunk/extragear/graphics/kst/kst/kstrvector.cpp #503056:503057 @@ -430,7 +430,8 @@ int i, k, shift, n_read=0; int ave_nread; int new_f0, new_nf; - + bool start_past_eof = false; + checkIntegrity(); if (DoSkip && Skip < 2 && SPF == 1) { @@ -462,6 +463,7 @@ // Tried to read starting past the end. new_f0 = 0; new_nf = 1; + start_past_eof = true; } } @@ -575,7 +577,10 @@ } // read the new data from file - if (_file->samplesPerFrame(_field) > 1) { + if (start_past_eof) { + _v[0] = NAN; + n_read = 1; + } else if (_file->samplesPerFrame(_field) > 1) { assert(new_nf - NF - 1 > 0 || new_nf - NF - 1 == -1); assert(new_f0 + NF >= 0); assert(new_f0 + new_nf - 1 >= 0);