View Issue Details

IDProjectCategoryView StatusLast Update
0006280The Dark ModCodingpublic07.04.2023 19:08
Reporterstgatilov Assigned Tostgatilov  
PrioritylowSeveritytweakReproducibilityN/A
Status resolvedResolutionfixed 
Product VersionTDM 2.11 
Target VersionTDM 2.12Fixed in VersionTDM 2.12 
Summary0006280: Default constructor and initialization
DescriptionThe long-time weird thing is that idVec3 is the only type which zeros itself in default constructor.
It is inconsistent, and it happened only because someone added zeroing in TDM team.
Maybe time to revert back and try to fix all uninitialized issues =(

Also, many custom types have empty default constructor.
With C++11, it's better to mark then as "= default".
TagsNo tags attached.

Activities

stgatilov

stgatilov

02.04.2023 09:51

administrator   ~0015987

As of now, I have not removed initialization from idVec3 on trunk, but I frequently remove it locally.

Note that marking constructors as "= default" allows to find more unused variables:
  r10335 Removed trivial default constructors for all vectors and matrices except idVec3.
  r10336 Fixed unused/uninitialized variable warnings.
  r10338 Added some more initialization.
  r10339 Deleted ton of unused variables.
  r10340 Replaced many empty constructors with "= default" constructors. Also replaced some empty destructors with "= default".
  r10341 Deleted more unused variables.

Some initialization fixes:
  r10342 Assert in idAASLocal::GetAreaBounds for file not loaded, and return zero box.
  r10343 Added more members initialization to game classes.

The plan for future is to try ASAN and static analyzer (maybe even make a new configuration for them) to find more usages of uninitialized idVec3.
stgatilov

stgatilov

07.04.2023 18:58

administrator   ~0015992

Static analysis is total trash.
Two options are available in MSVC: Microsoft Code Analysis and clang-tidy.

I enabled "static analyzer" ruleset in clang-tidy, and it simple hanged.
I waited for 1.5 hours, after which it did not finish, but printed many messages like "cannot process abcdef.cpp file".
I stopped it after that: it is horribly slow and won't give proper results anyway.

The Microsoft Code Analysis is somewhat better.
I tried it with "Native code Minimum" ruleset.

The first two runs ate all memory and sent my Windows into memory-related BSOD.
I even checked that I really have swap file allocated =)
Maybe that's some stability issue with my hardware... I guess I should check.

Anyway, on third time I decided to pause it when memory usage came close to RAM size, then continue.
After 3-5 chunks, I got the results =)
There was insane number of messages, and 99.99% of them are just trash.

One message relevant to initialization is "this member is not initialized in constructors".
Damn, is this trivial kind of message the best that the modern static analysis can provide?!
The second message is "possible uninitialized memory read".
I looked at some such cases, and all of them were false positives like this:
  idMat matrix;
  matrix = ...;
I suppose analyzer saw that operator= was called which accepts "matrix" by reference, and it assumes that the value is used inside (while it is not).

So static analysis is trash in our case, not worth rewriting all the code to please these stupid garbage rules.
stgatilov

stgatilov

07.04.2023 19:07

administrator   ~0015993

Last edited: 07.04.2023 19:08

ASAN is much better though!

However, it does not catch uninitialized memory usages.
In clang + LLVM, MSAN is used for this purpose, and it is only available on Linux =(
MSVC only has "Address Sanitizer", which does not catch uninitialized memory usage according to docs.
It catches read-after-poisoning, but happens only when we explicitly poison memory after deallocation (in custom allocators).

Anyway, I added Sanitize configuration, based on Debug Fast:
  r10352 Added "Sanitize" configuration with Address Sanitizer turned on.
It works pretty slowly: I have only about 10 FPS on Inn Business.
Applying it to Release build does not make any difference in performance...

ASAN detected a two real out-of-bounds reads and one false positive:
  r10350 Don't round up number of bytes when calling Alloc[Vertex|Index].
  r10353 Fixing issues found by ASAN.
The false positive was related to using const reference to prolong the lifetime of temporary...
I managed to complete Inn Business (first night) with ASAN.
stgatilov

stgatilov

07.04.2023 19:08

administrator   ~0015994

Anyway, I removed initialization:
  r10355 Removed initialization from idVec3.
  r10356 Deleted all hacky idRaw optimizations.

Let's hope it won't cause too much harm...

Issue History

Date Modified Username Field Change
30.03.2023 18:41 stgatilov New Issue
30.03.2023 18:41 stgatilov Status new => assigned
30.03.2023 18:41 stgatilov Assigned To => stgatilov
02.04.2023 09:51 stgatilov Note Added: 0015987
07.04.2023 18:58 stgatilov Note Added: 0015992
07.04.2023 19:07 stgatilov Note Added: 0015993
07.04.2023 19:08 stgatilov Note Edited: 0015993
07.04.2023 19:08 stgatilov Note Added: 0015994
07.04.2023 19:08 stgatilov Status assigned => resolved
07.04.2023 19:08 stgatilov Resolution open => fixed
07.04.2023 19:08 stgatilov Fixed in Version => TDM 2.12