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

Fri Nov 6 16:07:10 PST 2015

> On 2015-Oct-21, at 10:38, Jim Grosbach via llvm-dev <llvm-dev at lists.llvm.org> wrote:
> 
>> 
>> On Oct 20, 2015, at 7:39 PM, Justin Bogner via llvm-dev <llvm-dev at lists.llvm.org> wrote:
>> 
>> "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).
> 
> I completely agree. In a pre-C++11 world, the implicit conversions made more sense, as the alternative was more verbose. Now with “auto” and range based loops, the issues are much more tractable.
> 

Okay, this part is done as of r252372.

> Thanks for digging into this, everyone. Very cool.
> 
> -Jim
> 
>> 
>>> 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
>> _______________________________________________
>> 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