Usage of pointers to elements of a std::vector that might be reallocated

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

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

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

Oh yes you’re right, I missed that :slight_smile: But the point still hold.

Amaury Pouly

2010/8/8 Eugene Toder <eltoder@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

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@gmail.com>