Amaury Pouly
2010-Aug-08 14:11 UTC
[LLVMdev] Usage of pointers to elements of a std::vector that might be reallocated
Oh yes you're right, I missed that :) But the point still hold. Amaury Pouly 2010/8/8 Eugene Toder <eltoder at gmail.com>> > Not only this code does not compile with NDEBUG set > > I may be missing something, but why does it not compile with -DNDEBUG? > assert() macro expands to noop when NDEBUG is set. > > Eugene > > On Sun, Aug 8, 2010 at 2:19 PM, Amaury Pouly <amaury.pouly at gmail.com> > wrote: > > Hello, > > I was trying to interface a custom backend instruction scheduler with > llvm > > code when I realize something terrible. The scheduling code builds a > graph > > made up of SUnit * nodes (see ScheduleDAG*.{cpp,h}). These SUnits nodes > are > > allocated via a std::vector< SUnit >. > > This isn't a problem as long as the pointers are taken after the vector > is > > fully filled and the vector never changes its size. But the problem is > that > > is can happen ! > > Indeed, in some rare cases, the scheduler needs to duplicate a SUnit and > > thus allocate a new one. This gives code this: > > > > ScheduleDAGSNodes.cpp: > > > > #ifndef NDEBUG > > const SUnit *Addr = 0; > > if (!SUnits.empty()) > > Addr = &SUnits[0]; > > #endif > > SUnits.push_back(SUnit(N, (unsigned)SUnits.size())); > > assert((Addr == 0 || Addr == &SUnits[0]) && > > > > Not only this code does not compile with NDEBUG set but it could trigger > an > > extermely reliable assertion failure in some cases. One could think that > > this is too rare to happen but it does happen with my scheduler and I'm > not > > quite embarassed because I can't do anything apart from hacking llvm > source > > code. I feel that triggering an assertion failure just because the user > made > > the mistake of having not luck is not great for such a big framework as > LLVM > > :) > > > > Shouldn't LLVM use a custom vector implementation for such cases, an > > implementation that does not invalidate pointers when growing ? > > I have such an implementation at hand that I'm willing to provide if > needed. > > > > Regards > > > > > > Amaury Pouly > > > > _______________________________________________ > > LLVM Developers mailing list > > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100808/95fc8e35/attachment.html>
Eugene Toder
2010-Aug-08 15:04 UTC
[LLVMdev] Usage of pointers to elements of a std::vector that might be reallocated
Right, later in the same file we have: // Reserve entries in the vector for each of the SUnits we are creating. This // ensure that reallocation of the vector won't happen, so SUnit*'s won't get // invalidated. // FIXME: Multiply by 2 because we may clone nodes during scheduling. // This is a temporary workaround. SUnits.reserve(NumNodes * 2); So for some reason *2 is not enough in your case. I guess the right thing here is to either have a way to reliably estimate the number of SUnits (if it's at all possible) or use a container that doesn't reallocate on insert. Eugene On Sun, Aug 8, 2010 at 3:11 PM, Amaury Pouly <amaury.pouly at gmail.com> wrote:> Oh yes you're right, I missed that :) But the point still hold. > > Amaury Pouly > > > 2010/8/8 Eugene Toder <eltoder at gmail.com> >> >> > Not only this code does not compile with NDEBUG set >> >> I may be missing something, but why does it not compile with -DNDEBUG? >> assert() macro expands to noop when NDEBUG is set. >> >> Eugene >> >> On Sun, Aug 8, 2010 at 2:19 PM, Amaury Pouly <amaury.pouly at gmail.com> >> wrote: >> > Hello, >> > I was trying to interface a custom backend instruction scheduler with >> > llvm >> > code when I realize something terrible. The scheduling code builds a >> > graph >> > made up of SUnit * nodes (see ScheduleDAG*.{cpp,h}). These SUnits nodes >> > are >> > allocated via a std::vector< SUnit >. >> > This isn't a problem as long as the pointers are taken after the vector >> > is >> > fully filled and the vector never changes its size. But the problem is >> > that >> > is can happen ! >> > Indeed, in some rare cases, the scheduler needs to duplicate a SUnit and >> > thus allocate a new one. This gives code this: >> > >> > ScheduleDAGSNodes.cpp: >> > >> > #ifndef NDEBUG >> > const SUnit *Addr = 0; >> > if (!SUnits.empty()) >> > Addr = &SUnits[0]; >> > #endif >> > SUnits.push_back(SUnit(N, (unsigned)SUnits.size())); >> > assert((Addr == 0 || Addr == &SUnits[0]) && >> > >> > Not only this code does not compile with NDEBUG set but it could trigger >> > an >> > extermely reliable assertion failure in some cases. One could think that >> > this is too rare to happen but it does happen with my scheduler and I'm >> > not >> > quite embarassed because I can't do anything apart from hacking llvm >> > source >> > code. I feel that triggering an assertion failure just because the user >> > made >> > the mistake of having not luck is not great for such a big framework as >> > LLVM >> > :) >> > >> > Shouldn't LLVM use a custom vector implementation for such cases, an >> > implementation that does not invalidate pointers when growing ? >> > I have such an implementation at hand that I'm willing to provide if >> > needed. >> > >> > Regards >> > >> > >> > Amaury Pouly >> > >> > _______________________________________________ >> > LLVM Developers mailing list >> > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> > >> > > >
Amaury Pouly
2010-Aug-08 15:13 UTC
[LLVMdev] Usage of pointers to elements of a std::vector that might be reallocated
It might well be a bug in my code, I'm pretty sure it clones unecessary units so part of the fault is mine for sure. My point is more a general thought about such workarounds. Amaury Pouly 2010/8/8 Eugene Toder <eltoder at gmail.com>> Right, later in the same file we have: > > // Reserve entries in the vector for each of the SUnits we are creating. > This > // ensure that reallocation of the vector won't happen, so SUnit*'s won't > get > // invalidated. > // FIXME: Multiply by 2 because we may clone nodes during scheduling. > // This is a temporary workaround. > SUnits.reserve(NumNodes * 2); > > So for some reason *2 is not enough in your case. I guess the right > thing here is to either have a way to reliably estimate the number of > SUnits (if it's at all possible) or use a container that doesn't > reallocate on insert. > > Eugene > > On Sun, Aug 8, 2010 at 3:11 PM, Amaury Pouly <amaury.pouly at gmail.com> > wrote: > > Oh yes you're right, I missed that :) But the point still hold. > > > > Amaury Pouly > > > > > > 2010/8/8 Eugene Toder <eltoder at gmail.com> > >> > >> > Not only this code does not compile with NDEBUG set > >> > >> I may be missing something, but why does it not compile with -DNDEBUG? > >> assert() macro expands to noop when NDEBUG is set. > >> > >> Eugene > >> > >> On Sun, Aug 8, 2010 at 2:19 PM, Amaury Pouly <amaury.pouly at gmail.com> > >> wrote: > >> > Hello, > >> > I was trying to interface a custom backend instruction scheduler with > >> > llvm > >> > code when I realize something terrible. The scheduling code builds a > >> > graph > >> > made up of SUnit * nodes (see ScheduleDAG*.{cpp,h}). These SUnits > nodes > >> > are > >> > allocated via a std::vector< SUnit >. > >> > This isn't a problem as long as the pointers are taken after the > vector > >> > is > >> > fully filled and the vector never changes its size. But the problem is > >> > that > >> > is can happen ! > >> > Indeed, in some rare cases, the scheduler needs to duplicate a SUnit > and > >> > thus allocate a new one. This gives code this: > >> > > >> > ScheduleDAGSNodes.cpp: > >> > > >> > #ifndef NDEBUG > >> > const SUnit *Addr = 0; > >> > if (!SUnits.empty()) > >> > Addr = &SUnits[0]; > >> > #endif > >> > SUnits.push_back(SUnit(N, (unsigned)SUnits.size())); > >> > assert((Addr == 0 || Addr == &SUnits[0]) && > >> > > >> > Not only this code does not compile with NDEBUG set but it could > trigger > >> > an > >> > extermely reliable assertion failure in some cases. One could think > that > >> > this is too rare to happen but it does happen with my scheduler and > I'm > >> > not > >> > quite embarassed because I can't do anything apart from hacking llvm > >> > source > >> > code. I feel that triggering an assertion failure just because the > user > >> > made > >> > the mistake of having not luck is not great for such a big framework > as > >> > LLVM > >> > :) > >> > > >> > Shouldn't LLVM use a custom vector implementation for such cases, an > >> > implementation that does not invalidate pointers when growing ? > >> > I have such an implementation at hand that I'm willing to provide if > >> > needed. > >> > > >> > Regards > >> > > >> > > >> > Amaury Pouly > >> > > >> > _______________________________________________ > >> > LLVM Developers mailing list > >> > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >> > > >> > > > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100808/65df31c5/attachment.html>
Apparently Analagous Threads
- [LLVMdev] Usage of pointers to elements of a std::vector that might be reallocated
- [LLVMdev] Usage of pointers to elements of a std::vector that might be reallocated
- [LLVMdev] Usage of pointers to elements of a std::vector that might be reallocated
- [LLVMdev] Suspicious code in backend scheduler
- [LLVMdev] Win32 JIT issue + bug in ScheduleDAGSNodes.h?