Why the fault?

Project Infrastructure LLVM Dev List Archives
1

for (BasicBlock::reverse_iterator I = BB.rbegin(), E = BB.rend(); I != E; ) {

Instruction& inst = *I;

++I; ß iterator should be advanced to the previous instruction

// Happens to be an Instruction::SExt.

// Works fine if I iterate forwards

if (isInstructionTriviallyDead(&inst, TLI))

inst.eraseFromParent();

}

Sorry for the inexperienced question, but I’m confused why this works when using a forward iterator, but fails in reverse. The iterator is moved off the current instruction so why would erasing the current instruction cause an error?

2

ilist uses std::reverse_iterator.

reverse iterator erase is not that simple, because
std::reverse_iterator does not return element, but element-1, when it
is dereferenced.
(IE it lies).

In particular, the standard says the relationship between
std::reverse_iterator and the base iterator you hand it is:

&*(reverse_iterator(i)) == &*(base - 1)

If you wanted it to be unchanged, you'd need to erase std::next(I).base().

If you wanted it to be advanced by one, you'd have to do:

std::advance(I);
erase( I.base() );

The API doesn't give you enough control to make this happen right now.
One option is to make eraseFromParent return the iterators that erase
returns, and go from there.
Another is to just add the instructions you want to erase to a
vector, and erase them later.

3

Note:
This should now be fixable in trunk.

I committed a patch yesterday to make eraseFromParent return the
iterator for instructions (basic blocks coming next)

So
Instead of ++I, the loop should be:

  for (BasicBlock::reverse_iterator I = BB.rbegin(), E = BB.rend(); I
!= E; ++I) {

    Instruction& inst = *I;

    if (isInstructionTriviallyDead(&inst, TLI))

      I = inst.eraseFromParent();

  }

This should work, if everything isn't lying :slight_smile: