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

I think the implicit iterator conversions are much less important now that
we have range based for loops, but I still like having them.

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20151020/f9a42202/attachment.html>

> 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
> 
>

"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