Problems with eraseFromParent()

Hi LLVMers,

I am working on my llvm school project optimization pass. It’s relatively simple, and it tries to remove redundant bitcast instructions followed by load instructions. At first, I’m using as input a bytecode generated by a simple Java test program.
I’ve already found the reduntant instructions by looking at it’s CFG, and I also could implement the code to substitute their references, in order to eliminate all dependencies on these instructions (the number of their uses are changed to zero).
The point is that when I finally attempt to remove the redundant unused instructions from the generated code, there is a seg fault error.
To remove them, I’m using this function:

void EraseInst(Instruction &I) {
assert(I.use_empty() && “Cannot erase used instructions!”);
I.eraseFromParent();
}

So please, if anyone could help me on this, i would be very grateful!

PS: It’s also following my pass and the bytecode used for tests.

Thanks for the attention,
Alysson

k2.java (136 Bytes)

k2.bc (7.85 KB)

my_pass.cpp (3.37 KB)

Alysson wrote:

Hi LLVMers,

     I am working on my llvm school project optimization pass. It's
relatively simple, and it tries to remove redundant bitcast instructions
followed by load instructions. At first, I'm using as input a bytecode
generated by a simple Java test program.
     I've already found the reduntant instructions by looking at it's
CFG, and I also could implement the code to substitute their references,
in order to eliminate all dependencies on these instructions (the number
of their uses are changed to zero).
    The point is that when I finally attempt to remove the redundant
unused instructions from the generated code, there is a seg fault error.
    To remove them, I'm using this function:

void EraseInst(Instruction &I) {
             assert(I.use_empty() && "Cannot erase used instructions!");
             I.eraseFromParent();
}

The problem is in how you use it:

   for (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E; I++) {
     if (I->use_empty() && dyn_cast<LoadInst>(I)) {
       errs() << "*****Instruction unused*****" << "\n";
       errs() << *I << "\n";
       EraseInst(*I);
     }
   }

Once you call EraseInst, you can't use it again in the I++ part of your for loop. You'll have to restructure your loop so that you increment before erasing your instruction, and erase the one you haven't incremented yet.

Nick

Dear Alysson,

You may be invalidating the BasicBlock::iterator by removing instructions while iterating through the instructions in the BasicBlock. One way to fix this would be to iterate through the BasicBlock and record in a std::vector<> all the instructions that should be removed. Then you can write a loop that removes the last element of the std::vector, erases it, and then repeats until the std::vector is empty.

Also, just to note, it looks like your code is not checking whether the LoadInst is volatile before deleting it. Your transform should not remove volatile LoadInst or StoreInst instructions, even if they appear dead.

-- John T.

Alysson wrote:

Thank you guys for the tips, now it’s working!
The problem was really concerned on the BasicBlock::iterator invalidation.

Best regards,
Alysson