Skip to content

Fix GArray warning due to unsigned int underflow

Issue

In the helpfully named "fix various compiler warnings" commit, I replaced some ints with unsigned ints, as there's no good reason to compare the two.
We were comparing flush_len (the number of elements to remove from the start of an array)
with flag_segments->len, the number of elements in the array.

The latter is clearly unsigned, and the former should be too.

Except this was the original:

...
int flush_len = 0;
for (int i = 0; i < flag_segments->len; i++) {
    if (should_flush_element) flush_len = i - 1;
}
// g_array_remove(array, index, number_of_elements
if (flush_len > 0) g_array_remove(flush_segments, 0, flush_len)

And this is what I did in !65 (merged):

...
unsigned int flush_len = 0;
for (unsigned int i = 0; i < flag_segments->len; i++) {
    if (should_flush_element) flush_len = i - 1;
}
// g_array_remove(array, index, number_of_elements
if (flush_len > 0) g_array_remove(flush_segments, 0, flush_len)

For i = 0, that causes an unsigned int underflow, makes the 'flush_len > 0' evaluate to true, and GArray reports a runtime warning saying "why are you trying to remove more elements than I have, I'm going to do nothing instead".

I've had a brief look through my other compiler warning fixes and don't see anything else risky. Reviewers have asked for a table categorizing each fix in terms of risk, that'll be a good chance to be sure we've thoroughly considered everything.

Fix

I've investigated, and i - 1 is meant to be i + 1.

The old code effectively says "Remove any segments 2 before the last one that stops before our buffer starts.".
The latter says "remove all segments that stop before our buffer starts".

The only way I could imagine the former being correct is if we're doing something like adding a segment on initialization, and assuming that the array is nonempty elsewhere.

Except push_with_flag itself includes an assertion that the final segment 'stop's after our buffer 'stop's. So following this function, it's impossible that clearing every element that stops before the buffer starts will leave flag_segments empty.

There also aren't too many uses of flag_segments for an exhaustive search. It's used:

  • In multiratespiir push_with_flag, which I've edited.
  • In multiratespiir add_flag_segment, which checks the array is nonempty.
    • If it's non-empty, it looks at the final segment. As above, we can't remove the final segment with the modified code anyway. But for completeness:
      • If the final segment is length 0, it will overwrite it.
        • A comment notes that such a segment is a "heartbeat", indicating how far the flag stream has progressed.
        • A comment also notes this was "inspired by control segments in gstlal_gate.c", so the heartbeat could be a holdover anyway.
      • If it's non-empty, and the final segment's stop is >= the new segment's start, it combines them.
  • Other uses in multirate spiir just create/destroy the array.
  • postcoh need_flag_gap, which I've edited.
  • postcoh add_flag_segment, duplicate of multiratespiir add_flag_segment
  • postcoh init/teardown as in multirate spiir

If the bold bit above weren't true, we might need to check further, but I'm pretty sure that the cases above wouldn't break anything anyway.

I've made that change and refactored the functions in the process.

Tests

I've run tests here (before the latest 2 commits): /fred/oz016/tdavies/projects/testing/run_tdavies__fix_flag_flush_length
And analysed here: test_zerolags_22_11_MR105_MR49.txt
(just looking at node 13 for convenience, all fields are identical)

An old run /fred/oz016/tdavies/projects/testing/run_tdavies__fix_compiler_warnings/logs_test/pipe_30683583_13.err produced this message:
image

And the new one does not:
image

Edited by Timothy Davies

Merge request reports