Possible bug when loading a data store with attributes

[kennyweiss]

A user (@GamefanA) reported a possible bug in sidre when loading a datastore with attributes.

@GamefanA provided the following reproducer:

void bug()
{
   uint8_t test[] = {1,2,3,4,5};

   axom::sidre::DataStore* ds = new axom::sidre::DataStore();

   ds->createAttributeScalar("attr", 10); // create the attribute

   // simple storing
   ds->getRoot()->createGroup("gr")
       ->createViewAndAllocate("foo", axom::sidre::UINT8_ID, sizeof(test));
   ds->getRoot()->getGroup("gr")
       ->createViewAndAllocate("bar", axom::sidre::UINT8_ID, sizeof(test));

   // attach views to attribute with different values
   ds->getRoot()->getGroup("gr")->getView("foo")->setAttributeScalar("attr", 1); 
   ds->getRoot()->getGroup("gr")->getView("bar")->setAttributeScalar("attr", 2);

   ds->getRoot()->getGroup("gr")->save("saveFile.hdf5");
   ds->getRoot()->getGroup("gr")->load("saveFile.hdf5"); // segfault
}

[kennyweiss]

It looks like the problem can be reduced as follows:

void bug()
{
   axom::sidre::DataStore* ds = new axom::sidre::DataStore();

   ds->createAttributeScalar("attr", 10); // create the attribute

   auto* gr = ds->getRoot()->createGroup("gr");

   gr->save("saveFile.json", "sidre_json");
   gr->load("saveFile.json", "sidre_json"); // segfault here
   
   delete ds;
}

@ltaylor16 -- do (or should?) we support loading attributes into a datastore that already has attributes? The existing tests only load datastores with attributes into empty datastores.

[ltaylor16]

We should probably support that, or at least not crash. The issue becomes what if the file you're loading has conflicting attributes? For example, loading an older version of the restart into a code.

[cyrush]

seems like the datastore's attribute info need to be saved and restored at the datastore level? Backward comap can be issue an issue, but you we can have clear semantics (loading will clear current atts and setup attrs from the loaded file)

[agcapps]

Revisiting this issue, I see that today the reproducer (@kennyweiss 's second comment of 23 July 2019) still crashes.

When a group in a datastore that contains attributes calls load() and the group that comes in happens to have attributes, we could do any number of things:

1. entirely replace the pre-existing attributes with the loaded attributes
2. entirely disregard (throw away) the loaded attributes
3. try to merge the pre-existing and loaded sets of attributes by name

My intuition is that 3. would be the least disruptive.

[rhornung67]

Do we have any clear use cases for the atttributes? That would help inform the basic behavior.

[kennyweiss]

Tag: @brunner6

[nselliott]

The segfault is entirely due to a name conflict--there's nothing wrong with calling load when Attributes already exist, but it crashes if the loaded Attribute has a name that is already being used. So we need to choose a better thing to do in that case, as summarized by @agcapps in the earlier post.