[PATCH] D49234: [DebugInfo] Refactor DWARFDie::iterator to prevent UB

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D49234#1164550, @efriedma wrote:

> [forward.iterators]: "Two dereferenceable iterators a and b of type X offer the multi-pass guarantee if [...] the expression `(void)++X(a), *a` is equivalent to the expression `*a`."  [reverse.iter.requirements]: "The template parameter Iterator shall meet all the requirements of a Bidirectional Iterator".  So it's UB to use std::reverse_iterator with your iterator.
>
> There isn't any standard way to handle this situation; libcxx has an internal hack for standard library types (https://reviews.llvm.org/rL300164).


Hmm - thanks for the pointers/words, Eli! Though I'm not sure this applies in our case. I /think/ the DIE child iterator does meet this definition of multi-pass guarantee. Making a copy of the iterator and incrementing it does not affect the original iterator - which can still be dereferenced and yield the same value.

Ah, there it is: "If a and b are both dereferenceable, then a == b if and only if *a and *b are bound to the same object." - this is a feature of Forward Iterators that the DWARF iterator does not/cannot (practically) meet (because it returns a reference to an element stashed in the iterator - so though the iterators are ==, *a and *b are bound to different (though equivalent) objects).

Hrm. So technically it only meets the requirements of input iterator - which would cause it to fail to compile with llvm::reverse. That seems OK-ish. Yep, check-llvm still compiles if this is only an input iterator.

So we're still left with those general choices - coudl just say that llvm::reverse doesn't support these sort of iterators (like std::reverse_iterator doesn't) (& in this case, could have DWARFDie expose a "reverse children" accessor, "rchildren()" perhaps) or provide a trait that exposes a specific way to get a supported reverse iterator if an input_iterator can support it.  I'm open to either, really. If we go the simpler route (rchildren) then if at some point in the future we find more and more cases like this, we can always add the trait/generalize things.


https://reviews.llvm.org/D49234