Enable address sanitizer

[ikanago]

address sanitizer は malloc と free を置き換えることで double free や memory leak などの挙動を検出するツールです．

使い方

gcc にオプションを渡すだけで有効化できます．

g++ -fsanitize=address -fno-omit-frame-pointer -g test.cpp && ./a.out

int main() {
    int *a = new int[10];
}

導入手順

CI で動かすことで main branch にはメモリリークなどがないままに保ちたいところですが，いきなり CI で動かすには既存の不具合を解消する必要があります．
しかし，実際に上記のオプションをつけてテストコードを実行してみたところ，かなり大量のエラーが出力されました．
具体的にはこのようなサマリが得られました: SUMMARY: AddressSanitizer: 2235751 byte(s) leaked in 18090 allocation(s).
これがソースコードの行数にしてどの程度のものかは不明なのですが，おそらくそれなりの量であり，すぐに address sanitizer を CI で動かすことはできなさそうです．

なので，まずは手元で動かしてエラーをつぶしつつ，いくつかの PR にわたって修正します．
そしてエラーが検出されなくなったら CI で address sanitizer を動かすという流れをとるというのがいま考えている案です．

[ikanago]

UB(undefined behavior) に対する sanitizer もあるようで，これも -fsanitize=undefined とするだけで使えるようです．
https://stackoverflow.com/questions/31803705/using-gcc-undefined-behavior-sanitizer

[ikanago]

gate_factory にある関数が以下のように new を返すので全部怒られます．
これは普通に new せずにコンストラクタを呼んで return すればいいと思うんですが、修正範囲が広すぎてきびしいです

qulacs-osaka/src/cppsim/gate_factory.cpp

Lines 31 to 33 in 9ec1044

	QuantumGateBase* Identity(UINT qubit_index) {
	return new ClsIGate(qubit_index);
	}

[forest1040]

修正範囲が広すぎてきびしいです

ですよね。。
どっかで対応を皆で話し合いたいですね。。

[ikanago]

address sanitizer を有効化したプログラムは，標準エラー出力にメモリリークのある呼び出しのスタックトレースを吐いてくれるんですが、スタックトレースの一番浅い呼び出しをまとめるスクリプトを書きました: https://github.com/Qulacs-Osaka/qulacs-osaka/blob/fix-memory-leak-trial/extract_memory_leak.py

重複除去をしたうえで144ヶ所あり，ほとんどは return new によるものでした．
以下のスニペットの init_circuit->copy() のようにすぐ修正できそうなコードもあるので，そういうところから手をつけていくとよさそうです．

qulacs-osaka/src/cppsim/noisesimulator.cpp

Lines 28 to 33 in 0db8913

	circuit = init_circuit->copy();
	for (UINT i = 0; i < circuit->gate_list.size(); ++i) {
	auto gate = circuit->gate_list[i];
	if (!gate->is_noise()) continue;
	gate->optimize_ProbablisticGate();
	}

[KowerKoint]

これは普通に new せずにコンストラクタを呼んで return すればいい

これ、関数内でローカルに新しいオブジェクトを作ってそのローカル変数へのポインタを返しても意味がないからnewで確保する必要があって大事なのは使い終わったゲートを確実にdelすることだと思っていますがどうですか…？

[forest1040]

まだこの問題（address sanitizer）を深く考えていないのですが。。

関数内でローカルに新しいオブジェクトを作ってそのローカル変数へのポインタを返しても意味がない

RVO/NRVO とか考えないといけない気がしてきました。。

[ikanago]

関数内でローカルに新しいオブジェクトを作ってそのローカル変数へのポインタを返しても意味がない

一般にはそうなんですが，RVO(return value optimization) があるので実際にはコピーなくオブジェクトを生成できて， new したポインタを必ずしも返さなくてもよい場合があるんですよね
ただちゃんと RVO でムーヴされてる確信が Rust ほど(個人的には)持てなくて，難しいです

ゲートは時間経過にしたがって個数が増えるとかはあまりなく，また計算を終えたらすぐ exit する用途がほとんどだと思うので，delete のことは考えずこのままにしてもいいかもしれません．

[ikanago]

cppreference によると，以下のようなコードでは T の copy/move constructor の有無によらず，オブジェクトは呼び出し元で生成される必要がある(required)らしいです．

T f() {
    return T();
}
f();

実際に小さいクラスで試してみたんですが、アセンブリを見ると main で直接オブジェクトが生成されていました: https://godbolt.org/z/Waoo34xGM

[forest1040]

ありがとうございます！
compiler explorer こんなサービスがあったとは。
すごくわかりやすいですね！！

[ysuzuki-qc]

こちら、お手数おかけしていてすいません。２点コメントがあって

・私もKowerKointさんと同じ考えで、RVOが働くのは静的なオブジェクト生成に対してであって、今回のように生成したゲートのポインタを要求する際には、

T* f(){
  return &T();
}

などとしても無効なポインタが返る（かtemporal dataのポインタを取るなというエラーが出る）と認識しています。いずれにせよ、上記のIdentity関数など、ポインタの形でインスタンスを生成する関数全般は実行したらユーザが自身の責任でポインタを破棄することが前提となっています。

・コア部分はメモリリークが無いよう細心の注意を払っているつもりですが、googletestのコードはメモリリークがあっても事実上問題が生じないと思っていたので、あまり確認せず作っていると思います。例えば張っていただいたexampleでがDensityMatrixGeneralGateTest::InstrumentGateでリークが生じているようですが、該当箇所を見ると

TEST(DensityMatrixGeneralGateTest, InstrumentGate) {
	auto gate1 = gate::merge(gate::P0(0), gate::P0(1));
	auto gate2 = gate::merge(gate::P0(0), gate::P1(1));
	auto gate3 = gate::merge(gate::P1(0), gate::P0(1));
	auto gate4 = gate::merge(gate::P1(0), gate::P1(1));

	auto Inst = gate::Instrument({ gate3, gate2, gate1, gate4 }, 1);
	DensityMatrix s(3);
	s.set_computational_basis(0);
	Inst->update_quantum_state(&s);
	UINT res1 = s.get_classical_value(1);
	ASSERT_EQ(res1, 2);
	s.set_Haar_random_state();
	Inst->update_quantum_state(&s);
	UINT res2 = s.get_classical_value(1);
	delete Inst;
}

ですが、これはgate::mergeとgate::Instrumentは引数のポインタをコピーして使うので、gate1,2,3,4を作るための16個のポインタと、instrumentの4個引数の4個のポインタは全てリークしていると思います。
（逆に言えば責任をもってポインタを解放している限りは、ライブラリ内部で何かがリークしていることは現状はほぼない…と期待したいです。とはいえ、testを雑に書いているせいでそれが確認できる形になっていないのはマズいのですが…）

・個人的には、sanitizerのテスト用に基本的な関数を全て読んで正常に開放するコードを一つ作り、そのコードにリークが無ければ問題ない、という形にする方が楽かなとは思います。

[WATLE]

この件に関して、どのくらいリーク修正の実作業が進んでいますか？

[forest1040]

この件に関して、どのくらいリーク修正の実作業が進んでいますか？

まだ実作業は、何も手つかずです。。