Justin Bogner via llvm-dev
2015-Oct-21 02:39 UTC
[llvm-dev] ilist/iplist are broken (maybe I'll fix them?)
"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
Chris Lattner via llvm-dev
2015-Oct-21 04:24 UTC
[llvm-dev] ilist/iplist are broken (maybe I'll fix them?)
> 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).+1 for explicitness. -Chris
Jim Grosbach via llvm-dev
2015-Oct-21 17:38 UTC
[llvm-dev] ilist/iplist are broken (maybe I'll fix them?)
> 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 <mailto:llvm-dev at lists.llvm.org>> writes: >>> On 2015-Oct-20, at 11:23, Reid Kleckner <rnk at google.com <mailto: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. 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 <mailto:llvm-dev at lists.llvm.org> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev <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/20151021/ffcd9392/attachment-0001.html>
Duncan P. N. Exon Smith via llvm-dev
2015-Nov-07 00:07 UTC
[llvm-dev] ilist/iplist are broken (maybe I'll fix them?)
> 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