<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Oct 20, 2015, at 7:39 PM, Justin Bogner via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" class="">llvm-dev@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">"Duncan P. N. Exon Smith via llvm-dev" <</span><a href="mailto:llvm-dev@lists.llvm.org" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">llvm-dev@lists.llvm.org</a><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">> writes:</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" class="">On 2015-Oct-20, at 11:23, Reid Kleckner <<a href="mailto:rnk@google.com" class="">rnk@google.com</a>> wrote:<br class=""><br class="">I think the implicit iterator conversions are much less important<br class="">now that we have range based for loops, but I still like having<br class="">them.<br class=""></blockquote><br class="">IMO, if a developer has an ilist iterator and wants a pointer, they<br class="">should explicitly use `&*I` to make the assumption that "`I` isn't the<br class="">end iterator" explicit in the code. Similarly, in the other direction,<br class="">`N->getIterator()` makes it clear that `N` is definitely not `nullptr`<br class="">and is therefore safe to compare to an iterator.<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">+1. The convenience of the implicit conversion isn't worthwhile here.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">While I'm not a huge fan of writing `&*I`, it's at least very obvious</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">that you need to check that I's valid. OTOH, N->getIterator() is very</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">clear (and is probably usually written as `auto I = N->getIterator()`,</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">which looks pretty nice).</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote><div><br class=""></div><div>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.</div><div><br class=""></div><div>Thanks for digging into this, everyone. Very cool.</div><div><br class=""></div><div>-Jim</div><br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">Note that after my ilist changes, this implicit conversion will look<br class="">basically like this:<br class="">--<br class="">struct ilist_node_base {<br class=""> ilist_node_base *Prev;<br class=""> ilist_node_base *Next;<br class="">};<br class="">struct ilist_iterator_base {<br class=""> ilist_node_base *N;<br class="">};<br class="">template <typename NodeTy><br class="">class ilist_iterator : private ilist_iterator_base {<br class=""> operator pointer() const { return static_cast<NodeTy *>(N); }<br class="">};<br class="">--<br class="">This kind of (implicit and potentially UB) downcast makes me uneasy.<br class=""><br class="">However, this will still be an improvement from having such implicit<br class="">(and totally wrong) downcasts on `operator++()`, so maybe it's not a<br class="">big deal. It's certainly more convenient to eschew type safety. I'm<br class="">willing to let these bitrot back if that's better.<br class=""><br class="">Now that I've rooted out the bugs I was looking for (confirmed LLVM<br class="">is clean as of r250852) I'll get back to fixing `getNextNode()` and<br class="">ilist itself.<br class=""><br class=""><blockquote type="cite" class=""><br class="">On Tue, Oct 20, 2015 at 11:13 AM, Duncan P. N. Exon Smith via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" class="">llvm-dev@lists.llvm.org</a>> wrote:<br class=""><br class=""><blockquote type="cite" class="">On 2015-Oct-07, at 17:57, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" class="">dexonsmith@apple.com</a>> wrote:<br class=""><br class="">I've been digging into some undefined behaviour stemming from how ilist<br class="">is typically configured. r247937, r247944, and r247978 caused a UBSan<br class="">failure to start firing on our Green Dragon bots, and after an IRC<br class="">conversation between David and Nick and Mehdi, we added a blacklist:<br class="">--<br class="">$echo "src:$WORKSPACE/llvm/include/llvm/CodeGen/MachineFunction.h" >> sanitize.blacklist<br class="">--<br class=""><br class="">ilist/iplist is a pretty strange list, and the more I dig into it (to<br class="">fix the UB) the more broken I think it is.<br class=""><br class="">I want to change a few things about it, but it'll be somewhat<br class="">intrusive (pun not really intended), so I want to get some buy-in before<br class="">really diving in. I've CC'ed the people in the IRC conversation and a<br class="">couple of others that seem to care about ADT and/or UB.<br class=""></blockquote><br class="">A quick update on this.<br class=""><br class="">The first problem I hit was that there are callers that *rely* on<br class="">`getNextNode()` returning the sentinel instead of `nullptr`. If you<br class="">look at the implementation of `getNextNode()`, that's kind of insane.<br class=""><br class="">The only way I could think to root out all the similar issues was to<br class="">look at every call to the implicit conversions and confirm they aren't<br class="">doing anything strange. Most easily done by applying the attached<br class="">patch, and getting this compiling again. I have some more commentary<br class="">in, e.g., r249767 and r249782.<br class=""><br class="">Some of the problems I've uncovered include r249758, r249763, r249764,<br class="">and more scary cases like r249925 and r250211.<br class=""><br class="">I've almost finished LLVM proper, but I haven't touched clang yet, or<br class="">other LLVM projects.<br class=""><br class="">Is there any contention about this? Do we eventually want to commit<br class="">this patch, or should we go back to our old implicit ways once I've<br class="">cleaned up ilist and iplist? (Basically, should we make clang clean<br class="">too and commit this patch, or should I just fix the bugs?)<br class=""><br class=""><br class=""><br class=""><br class=""><br class="">_______________________________________________<br class="">LLVM Developers mailing list<br class=""><a href="mailto:llvm-dev@lists.llvm.org" class="">llvm-dev@lists.llvm.org</a><br class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev<br class=""><br class=""><br class=""></blockquote><br class="">_______________________________________________<br class="">LLVM Developers mailing list<br class=""><a href="mailto:llvm-dev@lists.llvm.org" class="">llvm-dev@lists.llvm.org</a><br class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev<br class=""></blockquote><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">_______________________________________________</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">LLVM Developers mailing list</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="mailto:llvm-dev@lists.llvm.org" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">llvm-dev@lists.llvm.org</a><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a></div></blockquote></div><br class=""></body></html>