sourcemod icon indicating copy to clipboard operation
sourcemod copied to clipboard

SourceMod SDK: introduction into Valve memory system

Open Wend4r opened this issue 3 years ago • 7 comments

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 avatar Jan 07 '22 15:01 Wend4r

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

asherkin avatar Jan 07 '22 15:01 asherkin

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 avatar Jan 07 '22 16:01 Wend4r

If I include separately tier0/memoverride.cpp , then there will be a conflict of operator overloads at the linking stage with https://github.com/alliedmodders/sourcemod/blob/5611ec54a21c3045cc1680b954631c6ca049c768/public/smsdk_ext.cpp#L483-L501

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

Wend4r avatar Jan 07 '22 16:01 Wend4r

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.

asherkin avatar Jan 10 '22 15:01 asherkin

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 https://github.com/alliedmodders/sourcemod/blob/4e3df763582e06e6308c1c88305764f14f8311cf/AMBuildScript#L623-L625 https://github.com/alliedmodders/sourcemod/blob/62cb6a0458184c2c03500cc934e5eb125413f2f9/public/sample_ext/AMBuildScript#L348-L350 https://github.com/alliedmodders/sourcemod/blob/62cb6a0458184c2c03500cc934e5eb125413f2f9/public/sample_ext_nosdk/AMBuildScript#L244

I built and looked, they use malloc/free

Wend4r avatar Jan 10 '22 15:01 Wend4r

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.)

Wend4r avatar Jan 10 '22 15:01 Wend4r

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.

psychonic avatar Dec 28 '22 21:12 psychonic