MinGW: C++ の未定義動作によるクラッシュを修正する #1372
Conversation
|
✅ Build sakura 1.0.3000 completed (commit 2e0a05038b by @k-takata) |
|
✅ Build sakura 1.0.3001 completed (commit 8b1e2eb16c by @k-takata) |
|
安全な形でやるなら、関数内の 完全な形で修正するなら、 |
こちらの方針で修正完了。VC++ビルドへの影響は無くなったはず。 EditorConfigの影響で、行末空白の差分が大量に出ているので、差分を見るときは空白を無視するとよい。GitHub上だと以下のリンクが使える。 |
|
✅ Build sakura 1.0.3004 completed (commit 9beb48fe5e by @k-takata) |
k-takata
force-pushed
the
fix-mingw-build
branch
from
9754b8e to
2f9d69d
Compare
|
✅ Build sakura 1.0.3005 completed (commit 4c7d9e2a5c by @k-takata) |
|
このPRの中で行っていたMinGWビルドシステムの改善を別PRに切り出しました。#1375 |
Based on the EditorConfig setting.
Stop calling member functions with this==NULL. This causes undefined behavior on C++. This may work on VC++ but causes SEGV on MinGW. Related: sakura-editor#601
Restore "this" check for safety on VC++. Add TODO comments for the future work.
k-takata
force-pushed
the
fix-mingw-build
branch
from
2f9d69d to
1a3e380
Compare
|
✅ Build sakura 1.0.3010 completed (commit 6d6a6fd6c2 by @k-takata) |
| @@ -66,7 +66,12 @@ class CLayout | |||
| CLayoutInt GetIndent() const { return m_nIndent; } //!< このレイアウト行のインデントサイズを取得。単位は半角文字。 CMemoryIterator用 | |||
|
|
|||
| //取得インターフェース | |||
| CLogicInt GetLogicLineNo() const{ if(this)return m_ptLogicPos.GetY2(); else return CLogicInt(-1); } //$$$高速化 | |||
| CLogicInt GetLogicLineNo() const{ if(this)return m_ptLogicPos.GetY2(); else return CLogicInt(-1); } //$$$高速化 // TODO: Remove "this" check | |||
There was a problem hiding this comment.
レビューとは関係ない話です。
//$$$高速化 みたいなコメントがあちこちに入っているんです。
なんかの思惑があって入れたコメントなんだと思うんですが、意図が不明なのでヤダなって思ってます。
たとえばこのコメントの場合、「高速化できるよ！」なのか「高速化したよ！（だから実装ビミョーだけど勘弁してね）」なのか不明だと思います。高速化できるよ！だとして、「ここの高速化って必要？」というのは分からないし、高速化したよ！だとして、「なんで高速化したの？」というのが分からないです。
ソリューション全体を \${2,} で正規表現検索すると似たようなものが沢山出てきます。
コメントはおおむね、問題のありそうな箇所に付けられているので問題提起のつもりで残したのかな？と思っています。可能であるならこれらのコメントを一掃したいですが、いつになるやら・・・。
There was a problem hiding this comment.
あるいは、「高速化すべき」なのか…
一旦thisチェックを消した際にはコメントごと消したのですが、結局thisチェックを戻したので（しぶしぶ）一緒に復活させました。
Running the following command in ./sakura_core causes SEGV: ``` sakura.exe StdAfx.h -MTYPE=js -M="Down();Up();Right();Left();ShowFunckey();ShowMiniMap();ShowTab();SelectAll();GoFileEnd();GoFileTop();ShowFunckey();ShowMiniMap();ShowTab();ExitAll();" ``` This fixes it. Related: sakura-editor#1363 (comment), sakura-editor#601, sakura-editor#1372
PR の目的
C++ の未定義動作により、MinGWでビルドしたバイナリ(のRelease版)がクラッシュする問題を修正します。
（gccはNULLポインタに関する未定義動作を積極的に最適化に利用します。NULLポインタに対するメンバ関数の呼び出しは未定義動作であるため、gccは、メンバ関数が
this==NULLで呼ばれることはないと判断し、メンバ関数内のif(this)チェックを最適化で削除してしまいます。これにより、実際にthis==NULLで呼ばれてしまうと、NULLポインタアクセスが発生してクラッシュしてしまいます。）メンバ関数内での this が NULL かどうかのチェックをやめ、メンバ関数を呼ぶ元でクラスポインタがNULLでないことをチェックするようにします。必要に応じて、チェック用の static 関数を用意し、それに差し替えます。併せて、MinGW 用の Makefile が、msys2 の mintty 内から実行したときに正しく動かない問題があったので、修正しています。また、ヘッダファイルの依存関係の記述を修正し、githash.h, Funccode_enum.h, Funccode_define.h が自動で生成されるようにしました。→ #1375 に分離カテゴリ
PR のメリット
MinGWビルド(Release版)が動くようになる。
(起動するようにはなりましたが、特定の操作で落ちる可能性は依然として残っています。)
PR のデメリット (トレードオフとかあれば)
逆にVC++ビルドに影響出る可能性あり。テスト内容
MinGWビルド(Release版)が起動すること。
(いろいろ動かして落ちないこと。)
Ctrl+T を押してタグジャンプをしようとしたときに落ちないこと。
PR の影響範囲
CDocLine, CColorStrategy, EditNode, CMyWnd, CPrintPreview, CLayout, CEditWnd を使用している箇所に影響の可能性あり。
関連 issue, PR
Fix #601
参考資料