#2426: g++-10 possibly miscompiles parts of Carpet
| Reporter: | Roland Haas |
| Status: | new |
| Milestone: | |
| Version: | |
| Type: | bug |
| Priority: | major |
| Component: | Other |
Comment (by Roland Haas):
Changing all users was too much for me, so to test this suspicion I added an almost-iterator class to bboxsset2 (with an ugly hack for end()) that is in here: https://bitbucket.org/eschnett/carpet/branch/rhaas/pseudoiterator it makes the failing test (and the other testsuites) pass so seems to confirm the suspicion that the memoization is causing issues.
An alternative to changing callers might be to have all non-const members of bboxset2 explicitly clear the memoized state (instead of doing in in begin()) and only re-serialize in begin() if the state is not already serialized. I.e. change tracking in bboxset2. This would however, in contrast to changing the callers, keep the serialised state around even after it is no longer needed (just like the current implementation).
Finally one could (possibly) keep track of how many iterators exist (essentially using a counter in bboxset2 objects that count how many iterators exist) by having the iterators keep a pointer to their “parent” and only re-serialise once all iterators have been destroyed. This also means that in becomes possible to track if the parent bboxset2 is modified while there are const-iterators in existence (which is technically allowed I assume, the only non-allowed action is likely doing anything with the iterators after their parent has been modified).
I have never really had to worry about (correctly) implementing iterators so cannot say what the best approach would be.
I do not know if this bug has the chance to fail non-catastrophically (ie without users noticing). However it seems serious enough to me to backport a fix to the current release branch, or at the very least document using CARPET_DISABLE_BBOXSET2 as workaround, which however then requires that