buzz-lang / BittyBuzz

BittyBuzz is an implementation of Buzz for microcrontrollers.
MIT License
8 stars 7 forks source link

Garbage collection on neighbors cause an invalid fields #12

Closed xgroleau closed 3 years ago

xgroleau commented 3 years ago

When the garbage collection is called on the neighbors, data is added for the neighbor count. Though on the foreach, the function is called with the internal string as key and count as arg.

https://github.com/MISTLab/BittyBuzz/blob/cb791538d5c199409efef790f7439d2bedfc313f/src/bittybuzz/bbzneighbors.c#L479

To reproduce just let the garbage collection run on the neighbors, (so about 10 steps), no need to push neighbors.

function init() {
}

function step() {
    neighbors.foreach(
      function(rid, data) {
        if(data != nil){
           // After neighbor garbage collection, rid is a string of __BBZSTRID___INTERNAL_2_DO_NOT_USE__
           log("robot ", rid, "data ", data.distance); // We get an error since data is not a table here.  
        }
      });
}

Or modify the foreach unit test in the testneighbors.c, this will cause the test to fail.

TEST(foreach) {
    bbzvm_construct(0);
    bbzneighbors_data_gc(); // Line to add to reproduce

#ifndef BBZ_NEIGHBORS_USE_FLOATS
    bbzneighbors_elem_t elem = {.robot=1,.distance=127,.azimuth=0,.elevation=0};
    bbzneighbors_elem_t elem2 = {.robot=2,.distance=64,.azimuth=0,.elevation=0};
#else // !BBZ_NEIGHBORS_USE_FLOATS
    bbzneighbors_elem_t elem = {.robot=1,.distance=bbzfloat_fromint(127),.azimuth=bbzfloat_fromint(0),.elevation=bbzfloat_fromint(0)};
    bbzneighbors_elem_t elem2 = {.robot=2,.distance=bbzfloat_fromint(64),.azimuth=bbzfloat_fromint(0),.elevation=bbzfloat_fromint(0)};
#endif // !BBZ_NEIGHBORS_USE_FLOATS
    bbzneighbors_add(&elem);
    bbzneighbors_add(&elem2);

    bbzvm_push(vm->neighbors.hpos);
    bbzvm_dup(); // Push self table
    bbzvm_pushs(__BBZSTRID_foreach);
    bbzvm_tget();
    bbzvm_pushcc(foreach_fun);
    bbzvm_closure_call(1);
    REQUIRE(vm->state != BBZVM_STATE_ERROR);

    bbzvm_gc();
    bbzvm_destruct();
}

We have a temporary fix for now, we just check if if data is a table instead of checking it's not nil.

xgroleau commented 3 years ago

This bug seems to affect the count and reduce tests too if the bbzneighbors_data_gc is called at the start of those tests.

beltrame commented 3 years ago

I suspect the problem is that the cleanup of the neighbors structure is not done at the right time (or in the right way). We are investigating, and we'll try to fix this soon.

beltrame commented 3 years ago

I have identified the issue: the bbzneighbors_data_gc() is not a full gc() that is called at an odd position (during the processing of in messages). I don't have a fix yet, but I think the options are moving the data_gc() or expanding it to clear the dictionary.

xgroleau commented 3 years ago

Hi @beltrame, any update on this issue? I'm having the same issue trying to use neighbors.filter() (since swarmlist are not available, i'm filtering to get the kin and nonkin) when trying to create a square formation.

In the meantime i'm checking if the type an int for the robot_id and a table for the data.

EDIT: Actually I can't use filter even if I'm checking the type, I always get an empty table

xgroleau commented 3 years ago

It seems my neighbors.count() is wrong too. I seem to have one neighbors more than I'm supposed to have (probably the internal string key).

EDIT: The neighbors.count() that is wrong is caused by "BBZSTRID_INTERNAL_2_DO_NOT_USE" which get inserted. So the count is actually one less than what is returned.

xgroleau commented 3 years ago

It seems the BBZSTRID_INTERNAL_2_DO_NOT_USE is only used for BBZ_XTREME_MEMORY but I am not totally sure, so it could probably wrap it between an #ifdef/removing it seems to remove the problem, but I am not sure if there are other implications of this. You can check what I mean by that on this commit.

With my quick testing it did fix the wrong count.