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

[Justin Bogner via llvm-dev]

"Duncan P. N. Exon Smith via llvm-dev" <llvm-dev at lists.llvm.org> writes:
>> 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.

+1. The convenience of the implicit conversion isn't worthwhile here.
While I'm not a huge fan of writing `&*I`, it's at least very obvious
that you need to check that I's valid. OTOH, N->getIterator() is very
clear (and is probably usually written as `auto I = N->getIterator()`,
which looks pretty nice).

> 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
>> 
>> 
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev