Memory leak in Relooper Optimizer

[dcodeIO]

There is a memory leak at the end of Optimizer::SkipEmptyBlocks

binaryen/src/cfg/Relooper.cpp

Lines 656 to 657 in 9024697

	// FIXME do we leak old unused Branches?
	CurrBlock->BranchesOut.swap(NewBranchesOut);

where the answer to the question appears to be "yes" according to LSan.

Steps to reproduce:

• Compile with -fsanitize=address
• Run check.py wasm-opt --filter=*flatten_rereloop*

I tried to naively iterate through the swapped NewBranchesOut and find the branches not contained in CurrBlock->BranchesOut anymore, and that found some, but this didn't fix the problem entirely. Requesting expert support once more :)

[dcodeIO]

Other tests affected by the same leak:

• passes/interesting-pass-mix

Tests affected by other leaks (going to investigate):

• passes/legalize-js-interface_all-features Fix LegalizeJSInterface leaking duplicate stub Functions #3095
• spec/br (seen once, appears to be random) Fix Wasm2JSBuilder leaking temporary Function #3098

[dcodeIO]

One radical approach to fix this leak might be to keep vectors of all the CFG:Blocks and CFG:Branches created during the lifetime of the optimizer, and instead of trying to figure out what's not needed anymore half-way, delete all of them once the optimizer is done. Not sure if that'd add unnecessary memory footprint, though.

[dcodeIO]

Good news is, with the other fixes merged now, this one should be the last obvious leak check.py discovered. There's still the chance that spec/br is something super obscure, but I haven't been able to reproduce it so far so perhaps it was somehow connected to one of the others.

[tlively]

Keeping a vector of the temporary objects sounds very reasonable to me. I haven't looked very closely at the code, but I assume various things want to hold pointers to the CFG::Block and CFG::Branch objects. You probably can't just allocate a vector of blocks or branches then, because adding to the vector could move its contents and invalidate the pointers. I would probably investigate using vectors of unique_ptrs to blocks and branches instead. Then as long as those vectors live for the duration of the pass, it's ok for the rest of the code to continue using raw pointers to the blocks and branches.

[dcodeIO]

Alright, thanks! I have something that appears to work locally now. Going to clean that up and make a PR :)

[dcodeIO]

D'oh, found the spec/br one again, but this time I took notes. Unfortunately happened while testing a release build:

.. br.wast
executing:  /path/to/binaryen/lbuild/bin/wasm2js split.wast -all --disable-exception-handling
executing:  /path/to/binaryen/lbuild/bin/wasm2js split.wast -all --disable-exception-handling --allow-asserts

=================================================================
==21566==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 456 byte(s) in 1 object(s) allocated from:
    #0 0x7f9358fb9947 in operator new(unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10f947)
    #1 0x564d53c6a60f in wasm::Wasm2JSBuilder::processWasm(wasm::Module*, wasm::Name) (/path/to/binaryen/lbuild/bin/wasm2js+0x9260f)
    #2 0x564d53c6f509 in (anonymous namespace)::emitWasm(wasm::Module&, wasm::Output&, wasm::Wasm2JSBuilder::Flags, wasm::PassOptions, wasm::Name) (/path/to/binaryen/lbuild/bin/wasm2js+0x97509)
    #3 0x564d53c7ecd3 in (anonymous namespace)::AssertionEmitter::emit() (/path/to/binaryen/lbuild/bin/wasm2js+0xa6cd3)
    #4 0x564d53c16f0d in main (/path/to/binaryen/lbuild/bin/wasm2js+0x3ef0d)
    #5 0x7f93569fe0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

SUMMARY: AddressSanitizer: 456 byte(s) leaked in 1 allocation(s).

[dcodeIO]

Ah, no, is reproducible actually, just was looking at the wrong suite: check.py wasm2js --filter=*br*