Duet3D / RepRapFirmware

OO C++ RepRap Firmware
GNU General Public License v3.0
945 stars 535 forks source link

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

Closed dc42 closed 5 months ago

dc42 commented 6 months ago

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 commented 6 months ago

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.

dc42 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

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