メモリバッファクラスをx64対応できるように書き替える #1618

berryzplus · 2021-04-08T15:09:41Z

PR の目的

CMemoryをリファクタリングして、x64対応作業を始められるようにすることが目的です。

カテゴリ

その他

PR の背景

CMemoryやCNativeWはサクラエディタの中核に位置するクラスです。

SonarCloudによる静的解析で、以下のBugsレベル警告が検出されています。

Bugs(Blocker)レベル
- sakura_core/mem/CMemory.cpp
  
  Out of bound memory access (accessed memory precedes memory block)
  https://sonarcloud.io/project/issues?id=sakura-editor_sakura&issues=AXiZfAqkWWGx2hJc4xkw&open=AXiZfAqkWWGx2hJc4xkw
Bugs(Critical)レベル
- sakura_core/mem/CNativeA.h
  
  Explicitly define or delete the missing destructor so that it will not be implicitly provided.
  https://sonarcloud.io/project/issues?id=sakura-editor_sakura&issues=AWnxcx-MO9B58BDk-CXO&open=AWnxcx-MO9B58BDk-CXO
- sakura_core/mem/CNativeW.h
  
  Explicitly define or delete the missing destructor so that it will not be implicitly provided.
  https://sonarcloud.io/project/issues?id=sakura-editor_sakura&issues=AWnxcx-EO9B58BDk-CXM&open=AWnxcx-EO9B58BDk-CXM

これらのクラスは、依存関係のルート付近にあるヘッダーで定義されているため、修正がしづらいです。
最近マージされた #1615 により、修正のやりづらさはさらに増したと思います。

ただ、ここを修正しないと x64対応は不可能なので、どこかでなんとかしたいと考えていました。

今回は、思い切ってCMemory/CNativeWの大幅改修をして、x64対応の作業ができる状態を作り出したいと思います。

PR のメリット

上記3件のBugsレベル警告を解消できます。
現在発生している2万件のCodeSmellsのうち21件を解消できます。
CMemoryの単体テスト網羅率が100%になります。
INT_MAX(＝約2GB) を超えるメモリの確保を試みた場合に確実に「失敗」するようになります。
※従来は異常を検出できず、処理が続行されていました。
x64 - Debugビルドの警告が 576件から 497件に減ります。

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

CMemoryのpublicメソッドをいくつかリネームしているため、修正量が多くなっています。
CodeSmellsが新たに18件増えます。
- うち2件は原因が別クラスの定義にあるので対応不要です。
- うち7件はCMemoryクラスの仕様的に対応不可です。
- 残り9件は対応が必要と考えますが、今回は対応しません。

仕様・動作説明

サクラエディタの仕様・機能に変更はありません。

CMemoryで定義されているメソッドの改廃を行っています。

クラス名	メソッド名	対応	代替	説明	対応理由
CMemory	Clean	削除	Reset	バッファをリセット(解放)する	同機能の別名メソッドがあり、存在自体が冗長であるため。
CMemory	Clear	削除	Reset	バッファをリセット(解放)する	CNativeの同名メソッドと混同する危険があるため。
CMemory	_Empty	削除	Reset	バッファをリセット(解放)する	メモリバッファのリセット(解放)はクラス外部から普通に利用する機能であり、非公開にしておく意味がないため。
CMemory	Reset	新規	-	バッファをリセット(解放)する	リセット(解放)を意味するメソッド名が存在していなかったため。
CMemory	_AddData	削除	AppendRawData	データを追加する	メモリ確保とコピーを統合することにより、メモリ確保失敗時にコピーが強行される事態を防ぐ必要があった。
CMemory	SwabHLByte	削除	SwapHLByte	Byteを交換する	`Windows SDK`の_swabを利用するためには、好ましくないキャストをする必要がある。加えて、_swabを利用するメリットはとくにないため。

PR の影響範囲

CMemoryを利用する機能（エディタ機能全般）に影響する修正です。

テスト内容

影響範囲が広すぎるので、単純な起動確認をしたほうが良さそうです。

サクラエディタを起動する。
任意のテキストファイルを開く。

参考資料

AppVeyorBot · 2021-04-08T15:51:28Z

❌ Build sakura 1.0.3646 failed (commit e869c6f2d4 by @berryzplus)

AppVeyorBot · 2021-04-08T16:37:54Z

❌ Build sakura 1.0.3647 failed (commit 0fdb081204 by @berryzplus)

サイズ変数をsize_t化した際の修正が不十分でVA発生いたため、追加修正

AppVeyorBot · 2021-04-09T01:20:30Z

✅ Build sakura 1.0.3648 completed (commit 49e448e076 by @berryzplus)

sakura_core/mem/CNativeA.cpp

sakura_core/mem/CNativeW.cpp

AppVeyorBot · 2021-04-09T04:35:42Z

✅ Build sakura 1.0.3649 completed (commit 0f678d8dac by @berryzplus)

berryzplus · 2021-04-09T07:46:58Z

こんなの出てたので追加修正しました。

Rename this member function so that it doesn't hide an inherited non-virtual function, or make it virtual in the base class "CMemory".
https://sonarcloud.io/project/issues?id=sakura-editor_sakura&issues=AXi037PsqNOaEhErIvC_&open=AXi037PsqNOaEhErIvC_&pullRequest=1618

AppVeyorBot · 2021-04-09T07:51:07Z

❌ Build sakura 1.0.3650 failed (commit f2d01f74b0 by @berryzplus)

AppVeyorBot · 2021-04-09T08:30:54Z

✅ Build sakura 1.0.3651 completed (commit 341a66af3a by @berryzplus)

berryzplus · 2021-04-09T10:00:25Z

#1618 (comment) を対応しようとしたらこんなの出たので諦めます。

'CMemory' has virtual functions but non-virtual destructor
https://sonarcloud.io/project/issues?id=sakura-editor_sakura&issues=AXi1t3KSmnSiw_EpU5Ks&open=AXi1t3KSmnSiw_EpU5Ks&pullRequest=1618

sonarcloud · 2021-04-09T10:33:51Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
10 Code Smells

87.3% Coverage
0.0% Duplication

AppVeyorBot · 2021-04-09T10:43:06Z

✅ Build sakura 1.0.3652 completed (commit 839aa7ff0e by @berryzplus)

sakura_core/mem/CNativeW.h

kengoide · 2021-04-10T13:20:40Z

sakura_core/mem/CMemory.cpp

+	CMemory cmem( pData, nDataLen );
+	cmem.SwapHLByte();
+	if( cmem.GetRawPtr() != nullptr ){
+		::memcpy( pData, cmem.GetRawPtr(), nDataLen );


元のコードが過去にパフォーマンス問題に対策した結果のものに見えるので、115行目のコンストラクタと memcpy での2回のコピーがパフォーマンスに与える影響が気になりました。ビット演算するのが書いた人の趣味だった可能性もありますけれど…。

bswapはCPU命令なので、パフォーマンスを重視するならこちらを使うべきです。

もともとはstaticバージョンに本体があり、インスタンスメソッド側が参照する構造になっていたのを逆転しています。staticバージョンを使わない方向に持っていき、最終的には廃止したいと考えているので、あえて不都合が起きる実装にしています。いきなり動かなくすると困りそうなのでいったん残してある状態です。

過渡的な実装ということで承知しました。

ちなみにstaticバージョンでメモリコピーを行っている理由は、以下です。

bswapの対象はuint16_t(=unsigned short)です。

uint16_t*の値は、必ず偶数アドレスになります。

static関数の引数はchar*です。

char*の値は、奇数アドレスになりえます。

　👇

bswapを使うには、char*をuint16_t*に reinterpret キャストする必要があるので、問題をふせぐために新たに確保したメモリにコピーしてから処理します。

細かいことをいうと、mallocが返すアドレス値がどんな値であるか（≒偶数or奇数、アラインメント単位は？）について言語仕様的な規定はないはずです。返却されるメモリアドレスになんらかの要求仕様があるときには _aligned_malloc を使って仕様を明示します。一応、mallocで奇数アドレスが返ってきたのを見たことありませんし、現状の通常mallocで問題はないと思っていますが。

usagisita · 2021-04-11T03:16:29Z

sakura_core/mem/CMemory.cpp

-		p[1]  = ctemp;
+	// 既に必要サイズを確保できている場合、直ちに抜ける
+	if( nAllocSize < m_nDataBufSize ){
+		return;


不具合ではないのですが、<よりも<=のほうがほんの少しだけ幸せになるかもしれないと思いました。

ここを less than にした理由は capacity() の実装に起因しています。
おっしゃる通りだと思うのでどこかで対応したいです。（今回は見送りたいです）

usagisita · 2021-04-11T03:26:54Z

sakura_core/mem/CMemory.h

+	void _SetRawLength( size_t nLength );
+	void swap( CMemory& left ) noexcept;
+	//! メモリ再確保を行わずに格納できる最大バイト数を求める
+	[[nodiscard]] int capacity() const noexcept { return 8 <= m_nDataBufSize ? m_nDataBufSize - 2: 0; }


この8というマジックナンバーが曲者ですね。
2を引くのはm_nDataBufSizeがNUL終端を含んでいるからというのが根拠ですが、
8のほうはAllocBufferの実装に強く依存している可能性があるというか。
マイナス値を避けたいので変更したんだと思いますが、
2以上であれば、8でなくてもよいというなら、2のほうがいいかもしれないとは思います。
(これは修正要求ではないので、変更しなくてもかまいません）

マジックナンバー「8」についてはクラスコメントに記載しました。
「内部バッファのサイズは8の倍数に切り上げて確保される。」
8を定数化するのがあるべき姿かもしれません。
（冷静に考えると、0より1つ大きい値が8なので「0 < m_nDataBufSize」にしとけばいいのかも。）

現在のcapacity()実装は標準ライブラリstd::wstring::capacity()と異なるので、メソッドをリネームするか、実装を標準ライブラリに寄せるかして取り違え防止を図るのがよいと思います。

2 = sizeof(wchar_t) だから実質的に「1つ違いエラー」の見直しになるので今回PR対象からは外しました。

berryzplus · 2021-04-11T05:05:09Z

レビューありがとうございます。とりあえずマージしちゃいます。

このPRは目的が「準備」であることもあり、至らぬ点が多数ある認識っす。

何か気付いた点があれば、このPRのコメントでも良いですし、新規issueでも良いので課題をあげてもらえれば幸いです。

berryzplus added 3 commits April 8, 2021 21:21

CMemoryのリファクタリング(x64対応準備)

82c0bcb

CNativeWのリファクタリング(x64対応準備)

f17f17e

CNativeAのリファクタリング(x64対応準備)

003157f

SonarCloud警告対応

875b687

CNativeW::Replaceを修正

96999fa

サイズ変数をsize_t化した際の修正が不十分でVA発生いたため、追加修正

berryzplus marked this pull request as ready for review April 9, 2021 01:53

sanomari requested changes Apr 9, 2021

View reviewed changes

berryzplus marked this pull request as draft April 9, 2021 03:33

レビュー指摘対応

5ddfecc

追加修正

2c8ee41

追加修正

c0e687c

CMemory::capacity()のvirtualをやめる

690659c

berryzplus marked this pull request as ready for review April 9, 2021 12:05

sanomari approved these changes Apr 9, 2021

View reviewed changes

kengoide reviewed Apr 10, 2021

View reviewed changes

sakura_core/mem/CNativeW.h Show resolved Hide resolved

kengoide approved these changes Apr 10, 2021

View reviewed changes

usagisita reviewed Apr 11, 2021

View reviewed changes

berryzplus merged commit f62f209 into sakura-editor:master Apr 11, 2021

berryzplus deleted the feature/refactoring_of_cmemory branch April 11, 2021 05:05

This was referenced Apr 15, 2021

「SAKURA　EDITORでGrep」が機能しません。 #1626

Closed

「SAKURA EDITORでGrep」が使えるようにnullチェックを入れる #1628

Closed

berryzplus mentioned this pull request Apr 17, 2021

CMemory変更のデグレ対策 #1629

Merged

usagisita mentioned this pull request Apr 22, 2021

CMemory::AppendRawData()でAllocBuffer条件ミスを修正 #1638

Merged

beru mentioned this pull request Apr 22, 2021

特定のテキストが読み込まれない #1641

Closed

berryzplus mentioned this pull request Jun 5, 2021

[WIP] x64での警告修正(CMemory::AllocBuffer の引数を size_t に変更) #524

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

メモリバッファクラスをx64対応できるように書き替える #1618

メモリバッファクラスをx64対応できるように書き替える #1618

berryzplus commented Apr 8, 2021 •

edited

AppVeyorBot commented Apr 8, 2021

AppVeyorBot commented Apr 8, 2021

AppVeyorBot commented Apr 9, 2021

AppVeyorBot commented Apr 9, 2021

berryzplus commented Apr 9, 2021

AppVeyorBot commented Apr 9, 2021

AppVeyorBot commented Apr 9, 2021

berryzplus commented Apr 9, 2021

sonarcloud bot commented Apr 9, 2021

AppVeyorBot commented Apr 9, 2021

kengoide Apr 10, 2021

berryzplus Apr 10, 2021

kengoide Apr 10, 2021

berryzplus Apr 11, 2021 •

edited

usagisita Apr 11, 2021

berryzplus Apr 11, 2021

usagisita Apr 11, 2021

berryzplus Apr 11, 2021

berryzplus commented Apr 11, 2021

メモリバッファクラスをx64対応できるように書き替える #1618

メモリバッファクラスをx64対応できるように書き替える #1618

Conversation

berryzplus commented Apr 8, 2021 • edited

PR の目的

カテゴリ

PR の背景

PR のメリット

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

仕様・動作説明

PR の影響範囲

テスト内容

関連 issue, PR

参考資料

AppVeyorBot commented Apr 8, 2021

AppVeyorBot commented Apr 8, 2021

AppVeyorBot commented Apr 9, 2021

AppVeyorBot commented Apr 9, 2021

berryzplus commented Apr 9, 2021

AppVeyorBot commented Apr 9, 2021

AppVeyorBot commented Apr 9, 2021

berryzplus commented Apr 9, 2021

sonarcloud bot commented Apr 9, 2021

AppVeyorBot commented Apr 9, 2021

kengoide Apr 10, 2021

Choose a reason for hiding this comment

berryzplus Apr 10, 2021

Choose a reason for hiding this comment

kengoide Apr 10, 2021

Choose a reason for hiding this comment

berryzplus Apr 11, 2021 • edited

Choose a reason for hiding this comment

usagisita Apr 11, 2021

Choose a reason for hiding this comment

berryzplus Apr 11, 2021

Choose a reason for hiding this comment

usagisita Apr 11, 2021

Choose a reason for hiding this comment

berryzplus Apr 11, 2021

Choose a reason for hiding this comment

berryzplus commented Apr 11, 2021

berryzplus commented Apr 8, 2021 •

edited

berryzplus Apr 11, 2021 •

edited