Skip to content
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

Add new address natives #2112

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

Conversation

Kenzzer
Copy link
Member

@Kenzzer Kenzzer commented Feb 5, 2024

With TF2 64bits release looming on the horizon, its high time Sourcemod is updated with some very much needed natives for addresses. Otherwise we're going to have a lot of upset plugin authors !

Here's a list of a new natives I think could alleviate the issues :

Added offset parameter to LoadFromAddress and StoreToAddress

For instance plugins often retrieve C++ objects addresses and read member properties from them. So the need of offsetting address values is quite common and needs to be addressed, as it would be risky to perform within a plugin environment (overflowing the address value).

Added OffsetAddress

With loading and reading covered, manual offsetting still isn't (e.g setting up a manual SDKCall). A native like this one would be required.

Added LoadAddressFromAddress and StoreAddressToAddress

Addresses can be pointer variables. And while plugins could get away with it by treating Address type as NumberType_Int32 to store at a given address, they cannot anymore. An helper function for this scenario is required.

Added LoadSpanFromAddress and StoreSpanToAddress, LoadBytesFromAddress and StoreBytesToAddress

For every other types that aren't covered by our natives. Let's allow plugins to store and load abritrary chunk of bytes. Useful for big types like int64/int128. Or other obscure scenarios, like maybe reading a string in memory.

@nosoop
Copy link
Contributor

nosoop commented Feb 6, 2024

As someone that has needed to manipulate addresses in their plugins, this sort of thing is definitely needed as part of the architectural transition. I haven't had the time to confirm how the pseudo addresses work when it comes to heap allocations and the like so I've yet to confirm how this works in practice.

I gave my feedback in another channel, but I'll also comment on it here in case other people had their thoughts:

  • The PR initially only had the "byte" versions of the array accessors; I suggested adding a variant that takes any[] with optional sizes to allow for reading float / int types without having to do awkward char[] conversions.

@dvander
Copy link
Member

dvander commented Feb 6, 2024

Two short comments.

I don't have a strong opinion on unstructured memory access. However, it should be a last resort. It's inherently less maintainable and more susceptible to breakage. Having structured access is vastly better, even if 99% of the way there is structured, and the very last thing is the memory poke. That's why stuff like SDKCall and bintools and gamedata exist. But clearly the unstructured stuff is needed, and serves an important role, so it shouldn't be rejected or stuck in design hell.

As to int64 - my dance card got very full, very quickly and I had to take a step away from SP again. My bar for changes right now is "can I get it done in a night or two". int64 - probably not. Even if I could, I can't start on it right this moment.

Don't wait for int64. Use enumstructs:

enum struct Address {
    int high32
    int low32
}

It is MUCH easier to design syntactic sugar around these, so if you have an issue with them or have an idea that would make them easier to use, please file a bug.

@sapphonie
Copy link
Contributor

sapphonie commented Feb 6, 2024

Don't wait for int64. Use enumstructs:

enum struct Address {
    int high32
    int low32
}

It is MUCH easier to design syntactic sugar around these, so if you have an issue with them or have an idea that would make them easier to use, please file a bug.

IMO this should just be a new pawn type, something like Address64, that is maybe bcompat with 32 bit Address, where the low32 is populated and high32 is null?

@Kenzzer
Copy link
Member Author

Kenzzer commented Feb 6, 2024

Don't wait for int64. Use enumstructs:

enum struct Address {
    int high32
    int low32
}

It is MUCH easier to design syntactic sugar around these, so if you have an issue with them or have an idea that would make them easier to use, please file a bug.

Such a solution would definitively eliminate the need for OffsetAddress to exist, and the offset parameter to LoadFromAddress and StoreToAddress.

But LoadAddressFromAddress & StoreAddressToAddress would still be required. NumberType_Int32 is currently used to achieve load/storing an address in memory but it will never work for 64bits.
Sure we could use LoadSpanFromAddress & StoreSpanToAddress, but this would put higher importance on the order of struct properties for Address.

If we are to assume an enum struct is defined for Address. And it has the ""wrong"" order decided for high32 and low32 in the struct. Neither LoadSpanFromAddress or StoreSpanToAddress can then be used, and a special native like LoadAddressFromAddress and StoreAddressToAddress would be required still.

@bottiger1
Copy link
Contributor

Would a native like GetPseudoAddress(int hi, int low) be accepted? I have a use case where I can't put the address in gamedata as it doesn't have a symbol. Or at least a clone of LoadFromAddress that takes 2 Address values.

@Kenzzer
Copy link
Member Author

Kenzzer commented May 6, 2024

Would a native like GetPseudoAddress(int hi, int low) be accepted? I have a use case where I can't put the address in gamedata as it doesn't have a symbol. Or at least a clone of LoadFromAddress that takes 2 Address values.

This could be okay, however PeusdoAddresses are currently inherently broken, very high address numbers cannot be mapped to pseudoaddresses. We're currently investigating alternatives. One thing is sure, introducing new API that's gonna be replaced right after doesn't make much sense. That's why this PR has remained in limbo for so long.

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

Successfully merging this pull request may close these issues.

None yet

5 participants