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

Introduce MemoryPointer #2196

Closed
wants to merge 5 commits into from
Closed

Conversation

Kenzzer
Copy link
Member

@Kenzzer Kenzzer commented Jul 27, 2024

This whole PR is still an heavy draft, nothing has been field tested.

Introduction

As of today no immediate solution to PseudoAddress has been explored. Only third-party solutions like NotnHeavy/SM-Address64 & skial-com/port64 have been made, but no major adoption so far. And while those solutions can work, I believe the friction for plugin authors is quite high. Now having to worry about the server architecture, and the confusing concept of treating ptrs as array of int/any.

Explanation

Given that there's no timeline as for when enum struct natives will ever be a thing. I propose instead to expose a new handle type that shall act as an opaque pointer towards abitrary memory regions. The underlying C++ class IMemoryPointer contained by the handle type is exposed as a public header. Allowing third-party extensions to wrap their pointers into it, no longer having to rely and depend on cell_t capacity to store the pointer's value on 32 bits.

IMemoryPointer shall become the defacto standard to pass abitrary pointers around for plugins & extensions.

List of natives

MemoryPointer.MemoryPointer(int size)
Allocates an abitrary amount of memory.

MemoryPointer.Load
Similar to LoadFromAddress native.

MemoryPointer.Store
Similar to StoreToAddress native.

MemoryPointer.Offset
Offsets the base pointer, and creates a new MemoryPointer handle.

MemoryPointer.LoadMemoryPointer
Interprets the loaded data as an address and wraps it into a new MemoryPointer handle.

MemoryPointer.StoreMemoryPointer
Retrieves the pointer's value wrapped inside the MemoryPointer handle, and stores it.

MemoryPointer.LoadEntityFromHandle
Similar to LoadEntityFromHandleAddress native.

MemoryPointer.StoreEntityToHandle
Similar to StoreEntityToHandleAddress native.

GameData.GetMemSigEx
Similar to GameData.GetMemSig but wraps the pointer into a MemoryPointer handle.

GameData.GetAddressEx
Similar to GameData.GetAddress but wraps the pointer into a MemoryPointer handle.

GetEntityMemoryPointer
Similar to GetEntityAddress but wraps the entity pointer into a MemoryPointer handle.

SDKCall_MemoryPointer
New sdkcall type to allow member function calls from the ptr contained by a MemoryPointer handle.

SDKType_MemoryPointer
New sdkparam type, to retrieve the abitrary ptr contained inside a MemoryPointer handle. INVALID_HANDLE / null are allowed values to represent nullptr, if allow null flag was passed.

PrepSDKCall_SetAddressFromMemoryPointer
Similar to PrepSDKCall_SetAddress native.

Ultimately this PR aims to supersede #2159 & #2112

@NotnHeavy
Copy link
Contributor

I think this is a decent work-around, considering the current cell_t limitations of SourcePawn and the numerous extensions that currently exist as a result of 64-bit address workarounds - including mine (which I hoped would've been the standard until an official SM native of some kind came around).

However, I feel like it's still missing out on some features, such as being able to wrap around existing addresses rather than only being able to use the handle to allocate a new block of memory.

Perhaps natives for converting between pseudo addresses and MemoryPointer would also be ideal?

In the end as well this obviously creates a new issue of having to delete your MemoryPointer objects every time you use them as well, but I suppose active use of it wouldn't necessarily be encouraged.

@KaelaSavia
Copy link
Contributor

KaelaSavia commented Jul 28, 2024

In the end as well this obviously creates a new issue of having to delete your MemoryPointer objects every time you use them as well, but I suppose active use of it wouldn't necessarily be encouraged.

Perhaps it could be possible to have argument in MemoryPointer constructor that would decide whether pointer is freed on plugin end, or requires manual freeing.

Also, would it be possible for "reasons" to have those two natives implemented?

  • Convert two 32-bit ints (array) into MemoryPointer
  • Convert MemoryPointer into array of two 32-bits

Other than that, current solution is pretty decent, people should be able to easily adapt. Although in future also having wiki page just like we had for transistional syntax will make things even smoother in that regard!

@Kenzzer
Copy link
Member Author

Kenzzer commented Jul 29, 2024

Perhaps it could be possible to have argument in MemoryPointer constructor that would decide whether pointer is freed on plugin end, or requires manual freeing.

Purposefully leaking memory isn't something I'd advise. The standard in Sourcemod is that all plugin-instanced ressources are freed on unloading, this shouldn't be an exception. If a plugin or extension wants to keep a memorypointer around for longer, they're free to create their own and copy the data over.

//
// @param name Name of the property to find.
// @return New MemoryPointer handle containing the address calculated on success, or null on failure.
public native Address GetAddressEx(const char[] name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed here and GetMemSigEx having return field as MemoryPointer?

Copy link
Member

@Headline Headline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most is nit picky or have already been pointed out

core/logic/MemoryPointer.cpp Show resolved Hide resolved
core/logic/smn_core.cpp Show resolved Hide resolved
core/logic/smn_gameconfigs.cpp Outdated Show resolved Hide resolved
core/logic/smn_gameconfigs.cpp Outdated Show resolved Hide resolved
plugins/include/sourcemod.inc Outdated Show resolved Hide resolved
plugins/include/sourcemod.inc Outdated Show resolved Hide resolved
@Kenzzer Kenzzer requested a review from Headline August 3, 2024 12:39
@Kenzzer Kenzzer marked this pull request as ready for review August 3, 2024 12:40
@NotnHeavy
Copy link
Contributor

There's issues with using SDKType_MemoryPointer. These are addressed in a PR I made to Kenzzer's PR.

@Kenzzer
Copy link
Member Author

Kenzzer commented Aug 19, 2024

We should be good to merge this time. Though maybe we should wait for @psychonic to split branches, as mentioned on discord. Stabilise 1.12, start 1.13 and merge this over there.

@NotnHeavy
Copy link
Contributor

The thing with 1.12 at the moment is that dvander is currently refactoring the SourcePawn compiler completely, to the point that int64 hypothetically may be introduced. I'm not sure if it will make sense to start 1.13 now.

@dvander
Copy link
Member

dvander commented Aug 22, 2024 via email

@Kenzzer
Copy link
Member Author

Kenzzer commented Aug 22, 2024

The thing with 1.12 at the moment is that dvander is currently refactoring the SourcePawn compiler completely, to the point that int64 hypothetically may be introduced. I'm not sure if it will make sense to start 1.13 now.

Branching makes the most sense, because we can make breaking changes on the dev branch. This was suggested because int64 on SP is a possibility. And when it is, we can destroy MemoryPointer without really breaking bcompat, because its dev branch.

@NotnHeavy
Copy link
Contributor

NotnHeavy commented Aug 24, 2024

Ah, I suppose I haven't really thought about that. That makes more sense then.

Copy link
Member Author

@Kenzzer Kenzzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving a self-review; before I forget again what I was missing on this PR.
I plan on fixing all of this later this week.

}

HandleError err = HandleError_None;
Handle_t hndl = handlesys->CreateHandle(g_MemPtrHandle, new ForeignMemoryPointer(ptr), pContext->GetIdentity(), myself->GetIdentity(), &err);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't leak memory.

*(void **)buffer = nullptr;
return Data_Okay;
}
pContext->ThrowNativeError("Null/Invalid Handle MemoryPointer isn't allowed");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove "invalid", we can never branch here with an invalid handle. And it'd be weird to imply invalid handles can be used as "nullptr".

// @param size Offset from base pointer in bytes.
// @return New Handle to a memory pointer.
public native MemoryPointer Offset(int offset);
};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing equal native; i.e if two MemoryPointer handles hold the same pointer value.

@NotnHeavy
Copy link
Contributor

I decided to work on these for you. My code should be fine, although I can't test on my laptop because both my laptop and internet are horrible in the place I am currently staying at.

@Kenzzer
Copy link
Member Author

Kenzzer commented Dec 15, 2024

Closing in favor of #2226 . The consensus internally seems to be that we avoid introducing a new handle type.

Will re-open if cited PR fails.

@Kenzzer Kenzzer closed this Dec 15, 2024
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.

7 participants