RedirectInstance raises error 5 when funcReturn is a class, part 2

[MarioAnthes]

Sorry to bother you again, but it seems I've found another issue in the new implementation of RedirectInstance:

Please see the following predeclared class module RedirectInstanceTestClass:

'@PredeclaredId

Option Explicit

Private Type TClassMembers
    ID As Long
    PrivateString As String
End Type

Private This As TClassMembers

Public Property Get ID() As Long
    ID = This.ID
End Property

' Factory with private Init function (using instance redirection):
Public Function Create(ByVal newID As Long) As RedirectInstanceTestClass
    Dim newClass As RedirectInstanceTestClass
    Set newClass = New RedirectInstanceTestClass
    Set Create = Init(newClass, newID)
End Function
Private Function Init(ByVal Instance As RedirectInstanceTestClass, _
                      ByVal newID As Long) As RedirectInstanceTestClass
    LibMemory.RedirectInstance Init, Me, Instance
    This.ID = newID
    Set Init = Me
End Function

Public Function Clone() As RedirectInstanceTestClass
    Dim newClass As RedirectInstanceTestClass
    Set newClass = New RedirectInstanceTestClass
    Set Clone = MemberCopyTo(newClass, This)
End Function
Private Function MemberCopyTo(newClass As RedirectInstanceTestClass, ByRef Members As TClassMembers) As RedirectInstanceTestClass
    LibMemory.RedirectInstance MemberCopyTo, Me, newClass
    This = Members
    Set MemberCopyTo = newClass
End Function

The instance creation with Create() and private Init() function works fine. But calling Clone() with private MemberCopyTo() function raises again error #5 in RedirectInstance, although MemberCopyTo() returns the same type as the Init() function. It works if I change the return type to Object.

Any ideas?

Here is a small function to test the behaviour:

    'Arrange:
    Dim testCls As RedirectInstanceTestClass
    Dim clonedCls As RedirectInstanceTestClass

    'Act:
    Set testCls = RedirectInstanceTestClass.Create(-1)  ' works fine
    Set clonedCls = testCls.Clone           ' raises runtime error #5

    'Assert:
    Debug.Assert testCls.ID = -1
    Debug.Assert clonedCls.ID = -1

[cristianbuse]

Hi @MarioAnthes ,

Please don't apologize! On the contrary, your feedback is super useful.

A quick fix is to replace this line:

Private Function MemberCopyTo(newClass As RedirectInstanceTestClass, ByRef Members As TClassMembers) As RedirectInstanceTestClass

with this:

Private Function MemberCopyTo(ByVal newClass As RedirectInstanceTestClass, ByRef Members As TClassMembers) As RedirectInstanceTestClass

i.e. pass the instance variable by value as opposed to by reference (default when not specified).

I suspect the same issue would occur with the previous version of RedirectInstance. It all has to do with the stack size and at what offset is the instance pointer behind the scenes when compared to the address of the function return. For example, I could even support function return of User Defined Type but that would require the user to pass the size in bytes for the specific UDT so that I can use that as an additional offset to the current logic. I suspect that passing ByRef affects the offset and I guees I could add extra logic for that. However, I won't have time to investigate until next week.

Many thanks!

[MarioAnthes]

Thanks for the quick fix! I didn't even notice that I've passed the newClass argument ByRef instead of ByVal. Now it seems to work, although I still don't understand why RedirectInstance hasn't raised the runtime error with the ByRef argument when the private function returned Object instead of RedirectInstanceTestClass.

By looking at the source code of LibMemory, there is a static MEMORY_ACCESSOR variable declared in each function. I think I've figured out how this MEMORY_ACCESSOR variable is being used and the logic behind all Mem... properties, but... is there a reason why each property/function has its own static variable, instead of using only one MEMORY_ACCESSOR struct declared on module level for all of them? I've tried that for the Mem... properties and it has passed all tests.

[cristianbuse]

Hi @MarioAnthes ,

I still don't understand why RedirectInstance hasn't raised the runtime error with the ByRef argument when the private function returned Object instead of RedirectInstanceTestClass.

That is because for Object there is no extra offset in the stack frame. That is why I had to push edb2c002494837617e03aa21635eeff76fba6cca when you raised #12. More specifically, these lines are only reached if the return type is a specific class e.g. DemoClass or IUnknown but for Object it works directly.

is there a reason why each property/function has its own static variable, instead of using only one MEMORY_ACCESSOR struct declared on module level for all of them?

Yes. And a good one. As you probably know, while debugging, if code stops on a Stop or on a breakpoint, then other calls coming from the Locals, Immediate and Watches windows are ran on a separate thread, and so the values in a module level accesor can change while the code is in break mode thus leading to potential crashes. By limiting an accessor for each method, this problem is minimized, although, of course, can still happen within the scope of the method where the code is stopped. A quick example:

Code in Class1 cls module:

Option Explicit

Private m_counter As Long

Public Property Get Counter() As Long
    Counter = m_counter
    m_counter = m_counter + 1
End Property

And code in a standard bas module:

Option Explicit

Sub Test()
    Dim c As Class1: Set c = New Class1

    Stop 'Now expand c in the Locals window before proceeding
    Debug.Assert c.Counter = 0
End Sub

Run Test and when the code stops on the Stop statement then go to the Locals window and expand c. You will see that the value of m_counter gets incremented inside the class while the code is stopped. If the pvData member of the safe array inside an accesor is changed because of a call from one of the windows, then either a crash or bad results might occur - so, I chose to reduce this problem as much as I could. Before implementing accessors, I had a static remote memory struct in each method for the same reasoning e.g. here

[cristianbuse]

Hi @MarioAnthes ,

Just pushed a new commit. Unfortunately, there is no way around this - there needs to be an extra parameter and the address of the function return must be passed as before I even updated the way this function works. So basically, there are 4 parameters now: the function return, the address of the function return, the current instance, the new instance.

The second parameter i.e. VarPtr(funcReturn) is needed/used only for objects of specific type and can be 0 (zero) for anything else including Object. The reason this is needed for specific object types (e.g. DemoClass, IUnknown ...) is because the first parameter is a ByRef Variant. While we can easily find the address of the function return by reading it from tthe 8th byte in the Variant for most data types, we cannot do that for specific objects types as you discovered in #12 and #13. That is because for for specific objects types the Variant is actually pushed in the stack frame before the arguments of the private class function and there is an arbitrary offset that we must take into account (depending on the number of parameters in the private class function where redirect is called) - this is not happening for none of the other data types including Object. I initially used the offset PTR_SIZE * 2 which happened to work but it was wrong because it was just a coincidence that you had 2 ByVal parameters in the private class function (same like I had in my demo). As soon as one of the 2 paramaters was ByRef as in this #13 issue, the offset was wrong and same issue would happen with a different number of parameters. And so, because it's arbitrary, I had to reinstate the return address as a mandatory parameter.

It's still useful to pass both functionReturn and VarPtr(functionReturn) because the first parameter gives us the exact data type (by reading the Var Type bytes in the Variant wrapping the func return) whereas the second would only give us the address. By knowng the data type, there is no need to check for different addresses, as the older solution was doing, thus making the new function way faster.

Hope this clarifies the issue and this should now be definitely solved.

[MarioAnthes]

Thanks a lot for your great work! RedirectInstance works now flawless in my tests and in all classes I'm using it. I have no problem in adding another parameter for VarPtr(funcReturn). Also many thanks for responding to my questions and the good explanations how your implementation works!

[cristianbuse]

Thank you for your kind words and all the feedback!