Attempt to add 64 bit support to sdktools

[bottiger1]

No description provided.

[bottiger1]

@dvander @psychonic @asherkin

pinging the sourcemod core devs here so they can say if they are already working on this or that they don't like this solution before I spend any more time on this although I think it is pretty much done.

I'm introducing a new sdktools type called SDKType_Pointer which will takes 1 int on 32 bits and 2 int on 64 bits. Since sourcepawn can't return a 64 bit value as far as I know, this is why I introduce a new type instead of just fixing the POD type.

When you set it as a return value you need to put the return value as a parameter since it could be 64 bits.

Usage example with tf2attributes gamedata:

    Handle hGameConf = LoadGameConfigFile("tf2.attributes");
    
    Handle hSDKSchema;
    Handle hSDKGetAttributeDef;

    StartPrepSDKCall(SDKCall_Static);
    PrepSDKCall_SetFromConf(hGameConf, SDKConf_Signature, "GEconItemSchema");
    PrepSDKCall_SetReturnInfo(SDKType_Pointer, SDKPass_Plain); //Returns address of CEconItemSchema
    hSDKSchema = EndPrepSDKCall();
    if (!hSDKSchema) {
        SetFailState("Could not initialize call to GEconItemSchema");
    }

    StartPrepSDKCall(SDKCall_Raw);
    PrepSDKCall_SetFromConf(hGameConf, SDKConf_Signature, "CEconItemSchema::GetAttributeDefinition");
    PrepSDKCall_AddParameter(SDKType_PlainOldData, SDKPass_Plain);
    PrepSDKCall_SetReturnInfo(SDKType_Pointer, SDKPass_Plain); //Returns address of a CEconItemAttributeDefinition
    hSDKGetAttributeDef = EndPrepSDKCall();
    if (!hSDKGetAttributeDef) {
        SetFailState("Could not initialize call to CEconItemSchema::GetAttributeDefinition");
    }    

    int pSchema[2];
    SDKCall(hSDKSchema, pSchema);
    PrintToServer("pSchema %x %x", pSchema[0], pSchema[1]);

    int pAttr[2];
    SDKCall(hSDKGetAttributeDef, pSchema, pAttr, 1);
    PrintToServer("pAttr %x %x", pAttr[0], pAttr[1]);

[KyleSanderson]

Have you reviewed #705 ? It's nearly 7 years old but should help avoid what's being proposed with high and low bits.

[bottiger1]

Have you reviewed #705 ? It's nearly 7 years old but should help avoid what's being proposed with high and low bits.

I didn't review it. How will it help if it is already merged? Is there something specific you want to point out? I don't want to look through 90 files worth of changes.

[KyleSanderson]

Have you reviewed #705 ? It's nearly 7 years old but should help avoid what's being proposed with high and low bits.

I didn't review it. How will it help if it is already merged? Is there something specific you want to point out? I don't want to look through 90 files worth of changes.

hmmm...

The other notable change here is using what I call "pseudo addresses" on 64-bit platforms in any place that a native returns or is passed a memory address. A pseudo address consists of a 6-bit table index and a 26-bit offset. The table consists of base addresses to memory allocations. Using this kind of scheme allows some pointer arithmetic to be performed by a plugin. See core/logic/PseudoAddrManager.cpp for further details. I'll note that using dladdr was not an option here because GetEntityAddress for example returns addresses to things on the heap. The choice of 6 and 26 bits is kind of arbitrary, so we could adjust this as we see fit.

Is there something missing?

[bottiger1]

pseudoaddress is already being used and supposedly does not work on addresses far away from the code (ie heap)

[KyleSanderson]

pseudoaddress is already being used and supposedly does not work on addresses far away from the code (ie heap)

so ... do we need to allocate a new pseudoaddress type, based on your findings? Not trying to be cryptic here, but if we don't have to reinvent anything, and patch 16~ years of plugins that's the better solution, no?

[bottiger1]

It is not possible to invent another pseudoaddress that will fix the problem. You just can't fit 64 bits into 32 bits. If you just use the full 32 bits as the index to a hash table, you can't do pointer arithmetic on it anymore. No matter what combination of bits you use for the index or the offset, it's going to result in problems.

With my changes, you can port plugins to 64 bit and they will work on 32 bits, so it's fully backwards compatible. It just doesn't magically make unchanged plugins work on 64 bits.

Dvander himself said here not to wait for him to add 64 bit ints and to use arrays. So that is what I'm doing. If people want to wait until sourcemod fixes Address so it can be 64 bits transparently, they can wait. Or if they don't want their server to be down for a long time, they can use this. I don't think there are many plugins that use sdktools to get addresses.

Given that dvander said not to wait, I get the feeling that a stopgap measure like this is needed.

[bottiger1]

Can I get a decision on whether this will be approved or not or are we waiting for dvander to add 64 bit ints to sourcepawn? This is 1 of the last 3 things needed for full 64 bit support.

[Kenzzer]

Not here to comment on the decision. But I'd suggest you fix the diff change on the PR, as it's currently impossible to review what you've added or removed. And you should delete that duplicate change in sdktools, as you already have another PR #2152 adding that.

[bottiger1]

But I'd suggest you fix the diff change on the PR

I'm not sure what you mean. It looks pretty clean to me.

https://github.com/alliedmodders/sourcemod/pull/2159/files

Don't click on the force-push I did, because that was just me rebasing the branch to the recently merged changes.

I just want to hear a simple yes or no whether this solution is acceptable or we are waiting until dvander to make major changes to sourcepawn can return 64 bit values (even though he said not to). If it isn't, there's no point in doing more work on this like making the commits perfect.

I need the fixes I made from the other pull request so my server can run without crashing.

[Kenzzer]

I'm not sure what you mean. It looks pretty clean to me.

It's really not though
Edit: Yeah it's fine if you hide whitespace change, but still I think it's best if it's fixed. But anyways that just a suggestion to make things cleaner looking

[bottiger1]

yeah as you found out you didn't have whitespace changes hidden. I am not sure how that happened, but there are quite a few files in the project already with different line ending styles.

[KaelaSavia]

any ETA on merging? as int64 type is not coming to sourcepawn anytime soon and pseudoaddresses are gonna cause more trouble than not with heap, it seems like there's no better solution at the hand

[KaelaSavia]

Apologies for double post but we decided to compile SourceMod branch with this PR merged and run on our servers. So far it works great (tf2attributes, tf2 taunt'em, etc.) although was kinda confusing when to use SDKType_Pointer and when not.

But that's more of skill issue on our side

[dedimark]

any plans to merging?

[Hex4dec]

So what's blocking this? Measly formatting? If so, have you seen the state of the Sourcemod codebase? It is an absolute mess and that is not a valid reason to block this.

It seems like there are PRs superceding this, but the contributors of those PRs have stopped maintaining their patches and seems like these maintainers are too lazy to do any work themselves. Please just resolve the conflicts yoruself and merge this

[Kenzzer]

So what's blocking this? Measly formatting? If so, have you seen the state of the Sourcemod codebase? It is an absolute mess and that is not a valid reason to block this.

It seems like there are PRs superceding this, but the contributors of those PRs have stopped maintaining their patches and seems like these maintainers are too lazy to do any work themselves. Please just resolve the conflicts yoruself and merge this

Can we please not devolve the conversation into a heated debate, everyone understands and knows there's frustration around the lack of x64 fixes. But this is not a one PR merge job. Adding 64bits capability to plugins can't be done just like that.

This PR alone does not solve anything, yes you're able to pass 64 bits wide addresses to SDKCalls through the usage of 2-D array. But plugins are still unable of obtaining those 2-D arrays (other than through another SDKCall). The Address type and its various natives are still not fixed, and we neither account for DHooks' callbacks.

If we rush this in any capacity, we could end up with a lot of deprecated natives which we absolutely want to avoid. This PR as well as its other alternative has remained in limbo, because nobody has provided any testing feedback on the PRs, rightfully so because a lot of bricks (primarily dhooks) need to be fixed first so server operators feel encouraged to try and test their plugins on 64 bits.

And if you truly believe anybody is being lazy, you're welcome to provide your own contributions to the various 64 bits PR on all the AM repositories.

[ramirez-tf2]

seems like these maintainers are too lazy to do any work themselves. Please just resolve the conflicts yoruself and merge this

True entitlement to call the maintainers lazy when they volunteer to work on this on their spare time outside of their day jobs and other life obligations. If you’re in such a hurry, compile the PR yourself into a custom sourcemod build and run it in your server.

[bottiger1]

This PR alone does not solve anything, yes you're able to pass 64 bits wide addresses to SDKCalls through the usage of 2-D array. But plugins are still unable of obtaining those 2-D arrays (other than through another SDKCall). The Address type and its various natives are still not fixed, and we neither account for DHooks' callbacks.

I am not sure why you say this doesn't solve anything by itself. I have already fixed plugins that don't use dhooks. Dhooks is a completely seperate thing.

I've long given up on merging stuff like this anyhow and I'm just moving all logic to C++ extensions now.

[Kenzzer]

I am not sure why you say this doesn't solve anything by itself. I have already fixed plugins that don't use dhooks. Dhooks is a completely seperate thing.

Because it doesn't, and I'm not saying this to undermine this PR. It is as I've stated, other than using an SDKCall there's no way for plugins to obtain array addresses. The Address type is left unused, and none of the various natives are fixed to work with this PR new system that you introduced. Such as GameData.GetMemSig or GameData.GetAddress or GetEntityAddress or LoadFromAddress or StoreToAddress or DHooks callbacks. If you don't use any of those to retrieve pointers/addresses for your SDKCalls doesn't mean other server operators don't and I hope this clears the confusion, as I've stated here :

If we rush this in any capacity, we could end up with a lot of deprecated natives which we absolutely want to avoid.

And to be really clear, I'm not against this PR. But we need a comprehensive fix, or at the very least experimental features.

I've long given up on merging stuff like this anyhow and I'm just moving all logic to C++ extensions now.

Just to be clear, do you still want to pursue the PR and explore solutions for the stated issues or should we close it ?

[bottiger1]

Because it doesn't, and I'm not saying this to undermine this PR. It is as I've stated, other than using an SDKCall there's no way for plugins to obtain array addresses

What's wrong with using SDKCall to obtain a 64 bit address? You can also get them with this extension I made

https://github.com/skial-com/port64/blob/main/sourcepawn/port64.inc

I already used this PR to port tf2attributes so to say that this PR doesn't do anything is simply not true.

I think you personally use a lot of dhooks, while we don't. Dhooks is a relatively new thing and a lot of plugins don't use it.

I don't care if this gets accepted now as I've accepted it's never going to get merged long ago so I moved all the logic to C++. I'm already prepared for 64 bits except for ff2. I'm ok with just that being down in case valve ever decides to delete 32 bits.

The only way to fix this better is to implement 64 bit variables in sourcepawn. As I said in another pull request, dvander should say whether or not he's going to implement it, and he said to not wait for him to do it.

The Address type is left unused, and none of the various natives are fixed to work with this PR new system that you introduced.

I don't see why this is an issue. The other natives are still broken right now on 64 bits without this pull request. This pull request does not break anything.

Some broken stuff is still broken, sure. But when was there a requirement that a pull request fix everything? If other people want the other stuff fixed, they can either use my port64 extension (which you could merge into sourcemod), or they can make their own pull request fixing the other stuff.

[Kenzzer]

Again can we please not devolve this into a heated argument on hypotheticals over my personal situation when I'm not even running tf2 servers and it's irrelevant to this PR's scope ?

You bring back each time the topic to dhooks. You seem to misunderstand me. I primarily cite DHooks here because it's the primary source of Address but there are several other sources which I've cited above. I have never said your PR doesn't do anything, yes it does solve issues of passing and returning pointers in SDKCalls. However I did say it doesn't solve anything for the greater 64 bits issue. If we don't pause to think about this, we're going to deprecate a lot of natives, a lot of callbacks, and perhaps even introduce competing natives, which is not going to help any plugin developpers or server operators who we should be worrying about here rather than bringing all back to what you or I are doing, which is irrelevant.

I've surveyed several times roughly 30 different tf2 server operators or various sizes, and it always comes back that if we don't solve the Address situation, which pertains to ALL of sourcemod nobody is even going to attempt to run 64 bits plugins. I'm not attacking you or anyone when I say adding this feature does not solve anything, because you still need that bridge to convert Address to your array. Those sourcemod natives I quoted do exist and they're used a lot in a lot of plugins. And yes now you've mentioned you've port64, that's great but we would have needed a complementary PR or an issue or it mentioned in original post so we could talk about it rather than a blank OP, because merging that extension is going to deprecate a lot of natives and we (you, I, everyone) need to discuss to know if it's worth it. Because if it's worth it, then yes we merge it, then we merge your PR. And if it's not worth it, then we don't merge it, and we can't merge this PR neither. This issue spans beyond this PR's scope. At the end of the day, how we chose to approach this will affect a lot of plugin developpers, and it is in everyone's best interests to limit damage to plugins by having this conversation.

If you still refuse to acknowledge that this problem exists then this will be my last message on the PR. I'm afraid no other maintainer or contributor is going to comment here. If the solution was satisfactory the PR would have been merged a while ago or at the very least somebody else but me would have spoken up. I appreciate your work, I really do because that's what kicked the chains of thoughts and made me make several PRs to help add x64 support, but I don't want that sentiment soured because you think I'm pushing some dhooks agenda or being stubborn when it's irrelevant for the PR. I'm trying to make you understand the bigger picture here. You seem to think there's no harm in merging this as-is, and let everybody figure out how to make it work, and here I think there's troubles we should be discussing that, exploring hypothesis and solutions...

TLDR: Good PR, but nobody can use the fix in a straight forward manner (requiring third-party extensions) and/or full plugin rewrite and that problem must be addressed if we want to sanely merge this.