[llvm-dev] ilist/iplist are broken (maybe I'll fix them?)

[Duncan P. N. Exon Smith via llvm-dev]

> On 2015-Oct-20, at 11:23, Reid Kleckner <rnk at google.com> wrote:
> 
> I think the implicit iterator conversions are much less important now that we have range based for loops, but I still like having them.

IMO, if a developer has an ilist iterator and wants a pointer, they
should explicitly use `&*I` to make the assumption that "`I` isn't the
end iterator" explicit in the code.  Similarly, in the other direction,
`N->getIterator()` makes it clear that `N` is definitely not `nullptr`
and is therefore safe to compare to an iterator.

Note that after my ilist changes, this implicit conversion will look
basically like this:
--
struct ilist_node_base {
  ilist_node_base *Prev;
  ilist_node_base *Next;
};
struct ilist_iterator_base {
  ilist_node_base *N;
};
template <typename NodeTy>
class ilist_iterator : private ilist_iterator_base {
  operator pointer() const { return static_cast<NodeTy *>(N); }
};
--
This kind of (implicit and potentially UB) downcast makes me uneasy.

However, this will still be an improvement from having such implicit
(and totally wrong) downcasts on `operator++()`, so maybe it's not a
big deal.  It's certainly more convenient to eschew type safety.  I'm
willing to let these bitrot back if that's better.

Now that I've rooted out the bugs I was looking for (confirmed LLVM
is clean as of r250852) I'll get back to fixing `getNextNode()` and
ilist itself.

> 
> On Tue, Oct 20, 2015 at 11:13 AM, Duncan P. N. Exon Smith via llvm-dev <llvm-dev at lists.llvm.org> wrote:
> 
> > On 2015-Oct-07, at 17:57, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> >
> > I've been digging into some undefined behaviour stemming from how ilist
> > is typically configured.  r247937, r247944, and r247978 caused a UBSan
> > failure to start firing on our Green Dragon bots, and after an IRC
> > conversation between David and Nick and Mehdi, we added a blacklist:
> > --
> > $echo "src:$WORKSPACE/llvm/include/llvm/CodeGen/MachineFunction.h" >> sanitize.blacklist
> > --
> >
> > ilist/iplist is a pretty strange list, and the more I dig into it (to
> > fix the UB) the more broken I think it is.
> >
> > I want to change a few things about it, but it'll be somewhat
> > intrusive (pun not really intended), so I want to get some buy-in before
> > really diving in.  I've CC'ed the people in the IRC conversation and a
> > couple of others that seem to care about ADT and/or UB.
> 
> A quick update on this.
> 
> The first problem I hit was that there are callers that *rely* on
> `getNextNode()` returning the sentinel instead of `nullptr`.  If you
> look at the implementation of `getNextNode()`, that's kind of insane.
> 
> The only way I could think to root out all the similar issues was to
> look at every call to the implicit conversions and confirm they aren't
> doing anything strange.  Most easily done by applying the attached
> patch, and getting this compiling again.  I have some more commentary
> in, e.g., r249767 and r249782.
> 
> Some of the problems I've uncovered include r249758, r249763, r249764,
> and more scary cases like r249925 and r250211.
> 
> I've almost finished LLVM proper, but I haven't touched clang yet, or
> other LLVM projects.
> 
> Is there any contention about this?  Do we eventually want to commit
> this patch, or should we go back to our old implicit ways once I've
> cleaned up ilist and iplist?  (Basically, should we make clang clean
> too and commit this patch, or should I just fix the bugs?)
> 
> 
> 
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> 
>