Skip to content
This repository has been archived by the owner on Jan 12, 2024. It is now read-only.

VM: Resetting pxVmSetGlobalSelf() after a pxVmCallFunction() call throws memory exceptin #7

Closed
JaXt0r opened this issue Apr 13, 2023 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@JaXt0r
Copy link
Collaborator

JaXt0r commented Apr 13, 2023

When calling the below VM method, I get a memory exception:

C#

 public static bool CallFunction(IntPtr vmPtr, uint index, IntPtr self, params object[] parameters)
        {
            var prevSelf = pxVmSetGlobalSelf(vmPtr, self); // returns 0x0

            StackPushParameters(vmPtr, parameters);
            var success = pxVmCallFunctionByIndex(vmPtr, index, IntPtr.Zero);

            pxVmSetGlobalSelf(vmPtr, prevSelf); // Memory Exception

            return success;
        }

C++

PxVmInstance* pxVmSetGlobalSelf(PxVm* vm, PxVmInstance* instance) {
    auto* old = pxVmGetGlobalSelf(vm);
    auto* instSym = vm->vm.find_symbol_by_index(instance->symbol_index());
    vm->vm.global_self()->set_instance(instSym->get_instance());
    return old; // returns 0x0 during first call of pxVmSetGlobalSelf()
}

OpenGothic is doing the reset of globalSelf() in a similar fashion, but there's a difference:
The original phoenix implementation uses std::shared_ptr which are never empty. But the C-API leverages the underlying .data() (c_npc*) which can be empty.

Could it be possible to have a null-check within C-API and reset to a default (empty shared_ptr<>()) value? (maybe create a new shared_ptr every time? But is the old one being deleted?)

@lmichaelis lmichaelis self-assigned this Apr 14, 2023
@lmichaelis lmichaelis added the bug Something isn't working label Apr 14, 2023
@lmichaelis
Copy link
Member

Ah, yes I can see the problem. I've added a preliminary fix in fix/vm-broken-global-instances. Could you try it out, please?

@JaXt0r
Copy link
Collaborator Author

JaXt0r commented Apr 14, 2023

Works as expected. Tests run smoothly:
i.e. 2x calling TA_MIN() will both times reveal the same npcPtr value. And removing the value afterwards from stack isn't throwing any error.
Can be merged imho.

@JaXt0r
Copy link
Collaborator Author

JaXt0r commented Apr 14, 2023

I'm happy with the solution.
Case closed. :-)

@JaXt0r JaXt0r closed this as completed Apr 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants