Skip to content

SourceMod SDK: introduction into Valve memory system #1689

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Wend4r
Copy link
Contributor

@Wend4r Wend4r commented Jan 7, 2022

These changes to the overload of memory operators are necessary for components that include HL2SDK and subsequently static linking with the tier1! It was originally written to use the Valve memory system from tier0, so that through it there was an abstract opportunity to profile working with memory (g_pMemAlloc) to find leaks.

AMTL is not overloaded - it has a separate working environment with memory.

Wend4r added 2 commits January 7, 2022 03:36
by components related to HL2SDK.
Can be disabled with `META_NO_HL2SDK` definition
@asherkin
Copy link
Member

asherkin commented Jan 7, 2022

Thank you for the PR, but this is going to need a lot more description and rationale to consider merging.

@Wend4r
Copy link
Contributor Author

Wend4r commented Jan 7, 2022

I really don't like that in my extension with the tier1 used, memory can first be allocated from operator new -> malloc, and then closed in IMemAlloc::Free

image

And operator delete is used for allocated memory from tier1 which is used IMemAlloc inside

@Wend4r
Copy link
Contributor Author

Wend4r commented Jan 7, 2022

If I include separately tier0/memoverride.cpp , then there will be a conflict of operator overloads at the linking stage with

void *operator new(size_t size)
{
return malloc(size);
}
void *operator new[](size_t size)
{
return malloc(size);
}
void operator delete(void *ptr)
{
free(ptr);
}
void operator delete[](void * ptr)
{
free(ptr);
}

so I decided to embed g_pMemAlloc overload in the SourceMod SDK for components using the HL2SDK with tier1

@asherkin
Copy link
Member

I do think this might end up a little painful across games, there was a bunch of Source games that had memoveride disabled on Linux builds, so a lot of testing will be required (and CI isn't going to validate anything here).

If your root issue is just cross-crt allocation around CUtlVector, you might be interested in a solution similar to #1165.

Other than seeming like a large-ish linkage change here (which we've done worse of recently, and paid the support price), I'm not really for or against it, but would like the thoughts of @psychonic who knows the engine much better than I.

@Wend4r
Copy link
Contributor Author

Wend4r commented Jan 10, 2022

I do think this might end up a little painful across games, there was a bunch of Source games that had memoveride disabled on Linux builds, so a lot of testing will be required (and CI isn't going to validate anything here).

Games that do not support MemAlloc overloading/override will be ignored

sourcemod/AMBuildScript

Lines 623 to 625 in 4e3df76

if sdk.name in ['css', 'hl2dm', 'dods', 'sdk2013', 'bms', 'tf2', 'l4d', 'nucleardawn', 'l4d2']:
if compiler.target.platform in ['linux', 'mac']:
compiler.defines += ['NO_HOOK_MALLOC', 'NO_MALLOC_OVERRIDE']

if sdk.name in ['css', 'hl2dm', 'dods', 'sdk2013', 'bms', 'tf2', 'l4d', 'nucleardawn', 'l4d2']:
if builder.target_platform in ['linux', 'mac']:
compiler.defines += ['NO_HOOK_MALLOC', 'NO_MALLOC_OVERRIDE']

compiler.defines += ['META_NO_HL2SDK']

I built and looked, they use malloc/free

@Wend4r
Copy link
Contributor Author

Wend4r commented Jan 10, 2022

Another motivation to do this PR was what I saw in Valve binaries that they use MemAlloc overloading/override for binaries using tier1 (in server.so, engine.so and etc.)

@psychonic
Copy link
Member

but would like the thoughts of @psychonic who knows the engine much better than I.

I think this would probably be a net positive change, but the blast radius is huge. We need better ways to test these kinds of changes across all or most supported engines (or someone needs to test this one with brute force) before we can take this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Testing untested by author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants