Stop calling `HeapSize()` on `malloc()`’d memory

[Jayman2000]

Pull Request Type

• [ ] GitHub Workflow changes
• [ ] Documentation or Wiki changes
• [ ] Build and Dependency changes
• [x] Runtime changes
  • [ ] Render changes
  • [ ] Audio changes
  • [ ] Input changes
  • [ ] Network changes
  • [x] Other changes

Description

This makes the Windows version of Descent 3 less dependent on Windows 95 compatibility mode. Users will still need to run the game in Windows 95 compatibility mode for the moment, but this change is required for us to eventually make Descent 3 not require Windows 95 compatibility mode. See the commit message for details.

Additionally, I’m working on another PR that uses std::filesystem::directory_iterator. Unfortunately, Microsoft’s implementation of std::filesystem::directory_iterator crashes when the program is run in Windows 95 compatibility mode.

Related Issues

This PR is related to #418, but it doesn’t actually fix that issue.

Checklist

• [x] I have tested my changes locally and verified that they work as intended.
• [x] I have documented any new or modified functionality.
• [x] I have reviewed the changes to ensure they do not introduce any unnecessary complexity or duplicate code.
• [x] I understand that by submitting this pull request, I am agreeing to license my contributions under the project's license.

Additional Comments

I’ve tested the linux preset on NixOS 23.11 x86_64, and I’ve tested the win32 preset on Windows 11 x86_64. I haven’t been able to test this PR on macOS, but it changes macOS-specific code.

[winterheart]

I think is better just simplify WIN32's variant of mem_size_sub() in mem.cpp just like you want it in macro:

int mem_size_sub(void *memblock) {
  return _msize(memblock);
}

[Jayman2000]

I think is better just simplify WIN32's variant of mem_size_sub() in mem.cpp just like you want it in macro:

int mem_size_sub(void *memblock) {
  return _msize(memblock);
}

I think that doing that would fix one bug but introduce another bug. According to the Microsoft C runtime library (CRT) reference:

The _msize function returns the size, in bytes, of the memory block allocated by a call to calloc, malloc, or realloc.

When MEM_USE_RTL is defined, stuff like this will happen:

void *pointer = mem_malloc(100);  // This ends up calling malloc().
size_t size = mem_size(pointer);  // This ends up calling _msize().
// Calling _msize() on a pointer returned by malloc() is OK.

When MEM_USE_RTL is not defined, stuff like this will happen:

void *pointer = mem_malloc(100);  // This ends up calling HeapAlloc().
size_t size = mem_size(pointer);  // This ends up calling _msize().
// Calling _msize() on a pointer returned by HeapAlloc() is bad.
// You’re supposed to call HeapSize() when dealing with pointers returned by HeapAlloc().

[winterheart]

Well, if main goal is get rid of Heap calling, it would be better get rid them in another mem_* functions too. There lot of code in mem.cpp for WIN32 can be cleaned up.

[Jayman2000]

Well, if main goal is get rid of Heap calling, it would be better get rid them in another mem_* functions too.

That’s not the main goal. As I wrote in the commit message,

The main motivation behind this change is to make it so that Windows users can run Descent 3 in the DWM8And16BitMitigation compatibility mode.

I don’t really know anything about Windows’s Heap* functions, so I don’t know if it would be a good idea to stop calling them. I hadn’t heard about them until I started trying to run Descent 3 without Windows 95 compatibility mode.

[JeodC]

I have been able to avoid using compatibility settings for Descent 3 ever since I made the default BPP 32 instead of 16. I am not sure if this is explicitly needed in the compatibility context.

[Jayman2000]

I have been able to avoid using compatibility settings for Descent 3 ever since I made the default BPP 32 instead of 16. I am not sure if this is explicitly needed in the compatibility context.

Did you set the default BPP to 32 before or after ENABLE_MEM_RTL was added? I didn’t start encountering this problem until after ENABLE_MEM_RTL was added.

[JeodC]

I have been able to avoid using compatibility settings for Descent 3 ever since I made the default BPP 32 instead of 16. I am not sure if this is explicitly needed in the compatibility context.

Did you set the default BPP to 32 before or after ENABLE_MEM_RTL was added? I didn’t start encountering this problem until after ENABLE_MEM_RTL was added.

Before. Like a month ago. I haven't recompiled for windows yet to test the recent merges.

[Lgt2x]

I have not taken the time to understand D3 memory management system yet as others did, but HeapSize call in mem_size_sub mem.cpp has been causing crashes/memory corruption in my windows branch, could this PR solve this issue?

[pzychotic]

I have not taken the time to understand D3 memory management system yet as others did, but HeapSize call in mem_size_sub mem.cpp has been causing crashes/memory corruption in my windows branch, could this PR solve this issue?

It does fix the memory corruption problems on Windows for me.

[Jayman2000]

I have not taken the time to understand D3 memory management system yet as others did, but HeapSize call in mem_size_sub mem.cpp has been causing crashes/memory corruption in my windows branch, could this PR solve this issue?

I dropped the “Hack to get the Heap memory size function working” commit from your win-sdl2 branch and then I cherry picked this PR on top of it. After doing that, I was able to load into a level without it crashing.

As a stopgap solution, you can compile the win-sdl2 branch with ENABLE_MEM_RTL=OFF.

[Lgt2x]

Thanks for taking the time to test, waiting on @winterheart opinion to merge this

[winterheart]

I'll recheck the code related to the problem and try to rework it taking into account the fact that Windows part can use different memory allocation methods.

[winterheart]

Please see #456 for proposed fix.