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.

Sign up for GitHub

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

Jump to bottom

Add Address methodmap #1841

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

Add Address methodmap #1841

wants to merge 9 commits into from

Conversation

Wend4r
Copy link
Contributor

@Wend4r Wend4r commented Oct 13, 2022

So far, draft.
Need tests

@Wend4r Wend4r marked this pull request as draft October 13, 2022 13:09
@Wend4r
Copy link
Contributor Author

Wend4r commented Oct 13, 2022

Everything works from the first tests

[SM] Plugin methodmap_address_speedtests.smx reloaded successfully.
sm plugins reload methodmap_address_speedtests
L 10/13/2022 - 18:35:45: [methodmap_address_speedtests.smx] Target memory access flags is 0b0011
L 10/13/2022 - 18:35:45: [methodmap_address_speedtests.smx] Start <methodmap Address> memory tests
L 10/13/2022 - 18:35:45: [methodmap_address_speedtests.smx] <methodmap Address> tests done in 0.148277 sec.
L 10/13/2022 - 18:35:45: [methodmap_address_speedtests.smx] 24640000 reads by each <int8>, 12320000 writes by each <int8>
L 10/13/2022 - 18:35:45: [methodmap_address_speedtests.smx] Start <LoadFromAddress() and StoreToAddress()> memory tests
L 10/13/2022 - 18:35:45: [methodmap_address_speedtests.smx] <LoadFromAddress() and StoreToAddress()> memory tests done in 0.156457 sec.
L 10/13/2022 - 18:35:45: [methodmap_address_speedtests.smx] 12320000 reads by each <int8>, 12320000 writes by each <int8>
[SM] Plugin methodmap_address_speedtests.smx reloaded successfully.

methodmap_address_speedtests.zip

@Wend4r Wend4r marked this pull request as ready for review October 13, 2022 15:49
@Wend4r
Copy link
Contributor Author

Wend4r commented Oct 13, 2022

Added int16 and int32 to tests.

sm plugins reload methodmap_address_speedtests
L 10/13/2022 - 19:05:51: [methodmap_address_speedtests.smx] Target memory access flags is 0b0011
L 10/13/2022 - 19:05:51: [methodmap_address_speedtests.smx] Start <methodmap Address> memory tests
L 10/13/2022 - 19:05:51: [methodmap_address_speedtests.smx] <methodmap Address> tests done in 0.259314 sec.
L 10/13/2022 - 19:05:51: [methodmap_address_speedtests.smx] 24640000 reads by each <int8>, 12320000 writes by each <int8>
L 10/13/2022 - 19:05:51: [methodmap_address_speedtests.smx] 12320000 reads by each <int16>, 6160000 writes by each <int16>
L 10/13/2022 - 19:05:51: [methodmap_address_speedtests.smx] 6160000 reads by each <int32>, 3080000 writes by each <int32>
L 10/13/2022 - 19:05:51: [methodmap_address_speedtests.smx] Start <LoadFromAddress() and StoreToAddress()> memory tests
L 10/13/2022 - 19:05:51: [methodmap_address_speedtests.smx] <LoadFromAddress() and StoreToAddress()> memory tests done in 0.262618 sec.
L 10/13/2022 - 19:05:51: [methodmap_address_speedtests.smx] 12320000 reads by each <int8>, 12320000 writes by each <int8>
L 10/13/2022 - 19:05:51: [methodmap_address_speedtests.smx] 6160000 reads by each <int16>, 6160000 writes by each <int16>
L 10/13/2022 - 19:05:51: [methodmap_address_speedtests.smx] 3080000 reads by each <int32>, 3080000 writes by each <int32>
[SM] Plugin methodmap_address_speedtests.smx reloaded successfully.

methodmap_address_speedtests.zip

If one of the methods worked incorrectly, the server would crash when RemoveEntity().

KyleSanderson
KyleSanderson requested changes Mar 30, 2023
View reviewed changes
Copy link
Member

@KyleSanderson KyleSanderson left a comment

Choose a reason for hiding this comment

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

This is desired, but the implementations should match underneath. If we want to change end-user aspects that's fine.

core/logic/smn_core.cpp

if (!IsAddressValidRange(addr))
{
#ifdef _DEBUG
Copy link
Member

Choose a reason for hiding this comment

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

This should be printed regardless. What's the harm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be printed regardless. What's the harm?

An ordinary server holder will be scared.
And from a SourceMod, there are usually not so many error written in message.

core/logic/smn_core.cpp
}

#ifdef _DEBUG
if (!HasAddressAccess<SH_MEM_READ>(addr))
Copy link
Member

Choose a reason for hiding this comment

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

Safety is preferred, but I'm not sure this is necessary in either context. Address should be "trusted", and this can be a very hot path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Safety is preferred

I put the debug build into safe operation, because direct memory take a long time with many calls. I can make a cache for quick ranges checks of allowed memory

core/logic/smn_core.cpp
{
void *addr = GetAddress(caddr, coffset);

if (!IsAddressValidRange(addr))
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this patching memory pages like in the old implementation?

core/logic/smn_core.cpp

if (!IsAddressValidRange(addr))
{
#ifdef _DEBUG
Copy link
Member

Choose a reason for hiding this comment

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

same printing comment

core/logic/smn_core.cpp
}

#ifdef _DEBUG
if (!HasAddressAccess<SH_MEM_READ /* Old value is being read */ | SH_MEM_WRITE>(addr))
Copy link
Member

Choose a reason for hiding this comment

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

write the page bits regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

write the page bits regardless.

From a viewpoint of dynamic memory hierarchy and a native call cost, it would be advantageous of speed and functionality to read a old value, returning it, instead of void (zero)

core/logic/smn_core.cpp
@@ -1114,6 +1251,15 @@ REGISTER_NATIVES(coreNatives)
{"IsNullVector", IsNullVector},
{"IsNullString", IsNullString},
{"LogStackTrace", LogStackTrace},

{"Address.ReadInt8", Address_ReadInt8},
Copy link
Member

Choose a reason for hiding this comment

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

the existing natives should be updated to use the new functions.

plugins/include/sourcemod.inc
@@ -697,6 +697,10 @@ native FeatureStatus GetFeatureStatus(FeatureType type, const char[] name);
native void RequireFeature(FeatureType type, const char[] name,
const char[] fmt="", any ...);

#define SH_MEM_READ (1 << 0) // Memory block can be read.
Copy link
Member

Choose a reason for hiding this comment

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

These are implementation specific, and likely shouldn't have the SourceHook prefix. Changing them to something on the address map would likely be better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KyleSanderson KyleSanderson KyleSanderson requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Wend4r @KyleSanderson