Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

デバッグにVC++のCランタイム機能を活用する #1051

Merged
merged 8 commits into from
Sep 22, 2019

Conversation

berryzplus
Copy link
Contributor

PR の目的

デバッグにVC++のCランタイム機能を活用することにより、
ビルドの問題を解消すると共にデバッグ実装をシンプルにします。

カテゴリ

  • 仕様変更

PR の背景

言葉で全貌を説明すると少しややこしいので、やってることを書きます。
#1045 のUSE_LEAK_CHECK_WITH_CRTDBGがビルドエラーになる件の対策PRとして作り始めました。


#1045 の解消方法自体は単純でした。( 293493c )

#1045 (comment) で書いている通り、ビルドエラーの原因はこの部分です。

#include <crtdbg.h>
#define new DEBUG_NEW
#define DEBUG_NEW new(_NORMAL_BLOCK, __FILE__, __LINE__)

ここで new ⇒ DEBUG_NEW とマクロ定義しているのを解除すれば、ビルドは通るようになります。
メモリリークを検出することだけを目的とするなら、この対応で十分です。


「十分です。」といっても、メモリリークチェックがどう動くものか知らん人が大半だと思います。

そこで、せっかくなので実際にメモリリークするコードを書いて、メモリリークチェックが正しく機能しているかどうか検証してみました。( 9ac48d7 )

この修正によるメモリリークの検出結果はこんな感じになりました。

Detected memory leaks!
Dumping objects ->
{245222} normal block at 0x00B6DAE0, 260 bytes long.
 Data: <N_E_W_!_N_E_W_!_> 4E 5F 45 5F 57 5F 21 5F 4E 5F 45 5F 57 5F 21 5F 
{245221} normal block at 0x01A0EE40, 8 bytes long.
 Data: < D      > E8 44 9F 01 00 00 00 00 
{245220} normal block at 0x019F44E8, 28 bytes long.
 Data: <@   t e s t   !_> 40 EE A0 01 74 00 65 00 73 00 74 00 00 00 21 5F 
{245219} normal block at 0x00B98F08, 16 bytes long.
 Data: <n_e_w_!_n_e_w_!_> 6E 5F 65 5F 77 5F 21 5F 6E 5F 65 5F 77 5F 21 5F 
Object dump complete.

この出力内容ではソースコードのどこに問題があるかわかりませんが、ちゃんと検出できてますね 😄

ソースコードの行番号が出てない理由は、DEBUG_NEW マクロを使ってないからなんですが、とりあえず、 マクロを使わなくてもメモリリークの検出自体はできる の検証材料にはなると思います。


ソースコード中の参考リンクがDeadLinkになってることから想像するに、
修正前のメモリリークの検出方式はだいぶ古い情報に基づくものです。

//crtdbg.hによるメモリーリークチェックを使うかどうか (デバッグ用)
#ifdef USE_LEAK_CHECK_WITH_CRTDBG
//new演算子をオーバーライドするヘッダはcrtdbg.hの前にincludeしないとコンパイルエラーとなる
//参考:http://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=99818

せっかくの機会なので、最新のドキュメントに基づいた検出方式にアップデートしてみました。( 6adb54d )

この修正によるメモリリークの検出結果はこんな感じになりました。

Detected memory leaks!
Dumping objects ->
C:\work\sakura\sakura_core\config\build_config.cpp(32) : {245222} normal block at 0x01AC6E18, 260 bytes long.
 Data: <N_E_W_!_N_E_W_!_> 4E 5F 45 5F 57 5F 21 5F 4E 5F 45 5F 57 5F 21 5F 
C:\work\sakura\sakura_core\config\build_config.cpp(21) : {245221} normal block at 0x03A8EF60, 8 bytes long.
 Data: <        > 18 D8 B9 01 00 00 00 00 
C:\work\sakura\sakura_core\config\build_config.cpp(21) : {245220} normal block at 0x01B9D818, 28 bytes long.
 Data: <`   t e s t   !_> 60 EF A8 03 74 00 65 00 73 00 74 00 00 00 21 5F 
C:\work\sakura\sakura_core\config\build_config.cpp(21) : {245219} normal block at 0x03A5E938, 16 bytes long.
 Data: <n_e_w_!_n_e_w_!_> 6E 5F 65 5F 77 5F 21 5F 6E 5F 65 5F 77 5F 21 5F 
Object dump complete.

デバッグ用マクロの名前が DEBUG_NEW ⇒ DBG_NEW になりました。
まだ DBG_NEW を使っていませんが、メモリ確保を行った行番号が出力されるようになっています。
これが最新の方法 _CRTDBG_MAP_ALLOC を使うメリットです。

ただ、この行番号って、mallocを記述した行番号だからあんまし意味ないですよね 😢


普通に考えて、せっかく行番号を出すなら正しい行番号が出てくれたほうがよいです。
検証コードの new を DBG_NEW に書き換えて、正しい行番号を出力できるかチェックしてみました。( c9dbcc7 )

この修正によるメモリリークの検出結果はこんな感じになりました。

Detected memory leaks!
Dumping objects ->
C:\work\sakura\sakura_core\_main\WinMain.cpp(116) : {245222} normal block at 0x009A6F68, 260 bytes long.
 Data: <                > CD CD CD CD CD CD CD CD CD CD CD CD CD CD CD CD 
C:\work\sakura\sakura_core\config\build_config.cpp(21) : {245221} normal block at 0x0325EDC8, 8 bytes long.
 Data: < q      > B0 71 90 00 00 00 00 00 
C:\work\sakura\sakura_core\_main\WinMain.cpp(115) : {245220} normal block at 0x009071B0, 28 bytes long.
 Data: <  % t e s t     > C8 ED 25 03 74 00 65 00 73 00 74 00 00 00 CD CD 
C:\work\sakura\sakura_core\_main\WinMain.cpp(114) : {245219} normal block at 0x0325D618, 16 bytes long.
 Data: <                > CD CD CD CD CD CD CD CD CD CD CD CD CD CD CD CD 
Object dump complete.

デバッグ用マクロ DBG_NEW を使ったことにより、実際にメモリ確保を行った検証コードの行番号が出力されるようになっています。

ただ、これも「問題アリ」です。
ダンプデータの内容が CDCDCDCD になっています。
@kobake さん 肝煎りn_e_w_!n_e_w_!n_e_w_! の仕様が吹っ飛んでいます。


メモリを汚せなくなった原因は、カスタムnewの実装がMSVCのデバッグ版newと異なっているからです。単純にシグニチャを合わせることで、この問題を解決できます。個人的には n_e_w_!n_e_w_!n_e_w_! の仕様の必要性には疑問を持っているんですが、対処を入れてみました。( 24641ef )

この修正によるメモリリークの検出結果はこんな感じになりました。

Detected memory leaks!
Dumping objects ->
C:\work\sakura\sakura_core\_main\WinMain.cpp(116) : {245222} normal block at 0x03531218, 260 bytes long.
 Data: <N_E_W_!_N_E_W_!_> 4E 5F 45 5F 57 5F 21 5F 4E 5F 45 5F 57 5F 21 5F 
{245221} normal block at 0x03520F40, 8 bytes long.
 Data: <  S     > 80 AF 53 03 00 00 00 00 
C:\work\sakura\sakura_core\_main\WinMain.cpp(115) : {245220} normal block at 0x0353AF80, 28 bytes long.
 Data: <@ R t e s t   !_> 40 0F 52 03 74 00 65 00 73 00 74 00 00 00 21 5F 
C:\work\sakura\sakura_core\_main\WinMain.cpp(114) : {245219} normal block at 0x0158CF58, 16 bytes long.
 Data: <n_e_w_!_n_e_w_!_> 6E 5F 65 5F 77 5F 21 5F 6E 5F 65 5F 77 5F 21 5F 
Object dump complete.

ここまでで当初の目的は果たせたと思っています。


あとのコミットは、リテラル文字列関連の微修正と検証コードの除去をしているだけです。

PR のメリット

  • デバッグ時のメモリリークチェックが機能するようになります。

PR のデメリット (トレードオフとかあれば)

  • とくにありません。

PR の影響範囲

  • デバッグ版ビルドでメモリリークチェックが使えるようになります。
  • アプリ(=サクラエディタ)の機能に影響はありません。

関連チケット

close #1046 DEBUGのUSE_LEAK_CHECK_WITH_CRTDBGを有効にしたい
#1045 DEBUGのUSE_LEAK_CHECK_WITH_CRTDBGを有効にするとコンパイルエラー
#381 new/deleteのオーバーロードがGCCに怒られる対応
#51 ビルド時警告の削減:operator new/delete 部分
#48 ビルド時警告の削減
#36 プロジェクトファイルをsakura配下に集めませんか?の実装案

参考資料

https://docs.microsoft.com/en-us/visualstudio/debugger/finding-memory-leaks-using-the-crt-library

new⇒DEBUG_NEWの読替マクロがビルドエラーになっているのを対処する。
同時に、デバッグ時にはUSE_LEAK_CHECK_WITH_CRTDBGが有効になるようにする。
コンストラクタなしの構造体、普通のクラス、配列データの3パターンで確認。
・参照ドキュメントがDeadLinkになっていたので有効なアドレスに差し替える。
・リークチェックの方法を差替後ドキュメントのやり方に合わせる。
ドキュメントの説明通り、DBG_NEWを使って検証する方式に変更。
デバッグ用に「メモリをわざと汚す」ためのoperatorを再定義する。
ファイル名と呼出元行番号を受け入れるデバッグ用バージョンを上書きする。
これは、MSVCRTDの機能なのでMSVCのデバッグ版でのみ有効になるように判定を変更する。
実質的に何もしてなかったoperator deleteのカスタム実装は削除する。
This reverts commit 19984f4920f0337c393c66d45eea91ee5e8eb902.
This reverts commit 0f1fa70028d9d49cac8f96b65b66ffe7538a2bff.
@AppVeyorBot
Copy link

Copy link
Contributor

@beru beru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

問題無いと思います。

テスト用に追加したメモリリークしているメモリ確保の位置がプログラム終了時に出力される事を確認しました。

@berryzplus
Copy link
Contributor Author

レビューありがとうございます。
なんかあったら別PRで対処させて頂きます。

@berryzplus berryzplus merged commit 08298ea into sakura-editor:master Sep 22, 2019
@berryzplus berryzplus deleted the feature/fix_leakcheck branch September 22, 2019 05:36
@m-tmatma m-tmatma added this to the v2.4.0 milestone Dec 29, 2019
@KENCHjp KENCHjp added enhancement ■機能追加 no-changelog 【ChangeLog除外】 labels Jan 7, 2020
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 16, 2020
…kcheck

デバッグにVC++のCランタイム機能を活用する
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ■機能追加 no-changelog 【ChangeLog除外】
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DEBUGのUSE_LEAK_CHECK_WITH_CRTDBGを有効にしたい
5 participants