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.

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

Bug: setting elements of nested arrays sometimes goes wrong #1008

Closed
dc42 opened this issue May 17, 2024 · 5 comments
Closed

Bug: setting elements of nested arrays sometimes goes wrong #1008

dc42 opened this issue May 17, 2024 · 5 comments
Assignees
Labels
bug Bug that has been reproduced
Milestone

Comments

@dc42
Copy link
Collaborator

dc42 commented May 17, 2024

When the 'set' command is used to change the value of an element of an array within an array, the assignment can go wrong. See https://forum.duet3d.com/topic/35725/question-array-assignment.

@dc42 dc42 self-assigned this May 17, 2024
@dc42 dc42 added this to the 3.5.2 milestone May 17, 2024
@dc42
Copy link
Collaborator Author

dc42 commented May 17, 2024

I identified two issues. The minor one is incorrect calls to lock the heap. This is fixed in commit 702c47b. The major one is that at ArrayHandle.cpp(91) the recursive call to InternalAssignIndexed may trigger compacting garbage collection, which may cause the elements of the array (one of which is being assigned) to be moved.

@T3P3 T3P3 added the bug Bug that has been reproduced label May 20, 2024
gloomyandy added a commit to gloomyandy/RepRapFirmware that referenced this issue May 21, 2024
@dc42
Copy link
Collaborator Author

dc42 commented May 21, 2024

Code from user Nine Mile which is claimed to reproduce this:

Setup:

global mosET = { 0.0, null }
global mosTT = { vector(limits.tools, global.mosET) }

Some time later:

if { exists(param.X) }
    if { global.mosTT[param.P][1] == null }
        set global.mosTT[param.P][1] = { param.X, 0.0 }
    else
        set global.mosTT[param.P][1][0] = { param.X }

if { exists(param.Y) }
    if { global.mosTT[param.P][1] == null }
        set global.mosTT[param.P][1] = { 0.0, param.Y }
    else
        set global.mosTT[param.P][1][1] = { param.Y }

@dc42
Copy link
Collaborator Author

dc42 commented May 21, 2024

Here's a macro based on the code from Nine Mile that reproduces it. When invoked with a small Q parameter, it appears to work without error. When invoked with Q values greater than about 20 it either crashes or reports array index out of bounds.

var dummy = vector(50,0.0)
var mosET = { 0.0, null }
var mosTT = { vector(40, var.mosET) }
set var.dummy = 0.0

var X = 42
var Y = 24

while iterations < param.Q
    if { var.mosTT[iterations][1] == null }
        set var.mosTT[iterations][1] = { var.X, 0.0 }
    else
        set var.mosTT[iterations][1][0] = { var.X }

    if { var.mosTT[iterations][1] == null }
        set var.mosTT[iterations][1] = { 0.0, var.Y }
    else
        set var.mosTT[iterations][1][1] = { var.Y }

@dc42
Copy link
Collaborator Author

dc42 commented May 22, 2024

Believed fixed now in 3.5-dev. Andy S has confirmed that the fix works in his test setup. Now waiting for feedback from the user who posted the original thread.

@dc42
Copy link
Collaborator Author

dc42 commented May 23, 2024

User reported that it looks good, see https://forum.duet3d.com/post/339680.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug that has been reproduced
Projects
None yet
Development

No branches or pull requests

2 participants