twinbasic / twinbasic

274 stars 24 forks source link

Bug when using `MemCopy` from `VBA-MemoryTools`'s `LibMemory` #1701

Open Theadd opened 11 months ago

Theadd commented 11 months ago

Describe the bug MemCopy in LibMemory.bas from @cristianbuse's VBA-MemoryTools crashes (restarts the compiler when used within tB, crashes in VBE when loaded as a dll built with tB, or just doesn't work as expected based on input). Other methods in LibMemory.bas seem to work as expected.

To Reproduce

  1. In a new twinBASIC project, add LibMemory.bas
  2. Add a new twinBASIC module and paste the following code in it:
    Public Sub RunMAIN()
    Dim t(0 To 4) As Byte, t2(0 To 4) As Byte, i As Byte
    For i = 0 To 4: t(i) = i: Next i
    MemCopy VarPtr(t2(0)), VarPtr(t(0)), 5
    For i = 0 To 4
        Debug.Assert t(i) = t2(i)
    Next i
    End Sub
  3. Execute RunMAIN in the debug console.

Expected behavior To work, as it does in VBE.

Screenshots image

Desktop (please complete the following information):

Additional context I've tested with latest version of LibMemory.bas and previous one, tB compiler in win32 and win64, using arrays of byte, integer, long and variant with different sizes, everything fails. In win64 it directly restarts the compiler. I've attached the *.twinproj file used in the screenshot. MemCopyBug.zip Thank you! I'd have preferred to report the problem without third party dependencies but the magic behind LibMemory currently beats me, sorry.

cristianbuse commented 11 months ago

Hi @Theadd ,

Just a note - I intended the MemoryTools library for VBA7 because of the overhead of API calls as seen here and here.

However, in TwinBasic you most certainly want to use the RtlMoveMemory API because TB should not have the overhead issue.

I am not sure sure if there's a compiler directive in TB (something like #If TwinBasic Then) but if there is then please let me know and I will happily update my repo to use the API directly.

fafalone commented 11 months ago

There is a #If TWINBASIC Then compiler directive, but I'm not sure there's much difference between how APIs are called between the two. They both call it dynamically but cache the function pointer afaik (although tB has an option to put it into the IAT instead- but that only provides a non-negligible benefit on the first call).

ETA: Sorry, didn't read your test results thoroughly, looks like VBA64 might be doing something funky (again, a workaround for a major bug with AddressOf was found recently). But regardless, needed or not, since there's no internals hacking it's a bug needing to be fixed that it doesn't work properly in tB.

cristianbuse commented 11 months ago

Thanks @fafalone !

@Theadd I've updated the library here. Regardless if this issue gets fixed in TB, the wrapping should work fine now as the calls default to RtlMoveMemory.

cristianbuse commented 11 months ago

Hi @WaynePhillipsEA ,

This bug can be summarized with the following:

Module MyModule
    Public Sub TestLSet()
        Dim s As String

        s = "Test"
        LSetByRef s, s
    End Sub

    Private Sub LSetByRef(ByRef s As String, ByRef v As Variant)
        Dim t As String

        t = "Pass test"
        LSet s = t 'Works fine. Updates both 's' and 'v' as they both point to the same String By Ref
        Debug.Assert s = v

        t = "Fail test"
        LSet v = t 'Does not update the correct String By Ref but instead creates a new String
        Debug. Assert s = v
    End Sub
End Module

Additional bug:

Module MyModule
    Public Sub TestLSet2()
        Dim s As String

        s = "Test"
        LSet s = "Pa"
        Debug.Assert s = "Pa  "
    End Sub
End Module

Another one in #1704

cristianbuse commented 11 months ago

Hi @fafalone ,

Could you please test the above 2 bugs in VB6 as well? Thanks!

fafalone commented 11 months ago

I'm a little confused on how you mean the first one... I tested by adding MsgBox s after LSetByRef. VB6 and tB definitely behave different, but I'm getting 'Fail' from VB6 and 'Pass' from tB (sic-- neither provide the " Test")

cristianbuse commented 11 months ago

Apologies @fafalone , added 3 Asserts

fafalone commented 11 months ago

For the first bug both pass in VB6, in tB the second one fails.

The second bug yes that works in VB6 but not tB as well.