#1473: Carpet segfaults in Shutdown ---------------------+------------------------------------------------------ Reporter: knarf | Owner: eschnett Type: defect | Status: new Priority: minor | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------

Comment (by rhaas):

I just tried this on bethe (9 cores, ht, intel13). Using 9 OpenMP threads and 1 process I get an assertion failure from within hwloc: {{{ MPI process 0 on host 0 (process 0 of 1 on this host) OpenMP thread 0: PU set L#{0-15} P#{0-15} OpenMP thread 1: PU set L#{0-15} P#{0-15} OpenMP thread 2: PU set L#{0-15} P#{0-15} OpenMP thread 3: PU set L#{0-15} P#{0-15} OpenMP thread 4: PU set L#{0-15} P#{0-15} OpenMP thread 5: PU set L#{0-15} P#{0-15} OpenMP thread 6: PU set L#{0-15} P#{0-15} OpenMP thread 7: PU set L#{0-15} P#{0-15} OpenMP thread 8: PU set L#{0-15} P#{0-15} INFO (hwloc): Setting thread CPU bindings: cactus_sim: /home/rhaas/ET_trunk/arrangements/ExternalLibraries/hwloc/src/system_topology.cc:525: void <unnamed>::set_bindings(hwloc_topology *, const <unnamed>::mpi_host_mapping_t &): Assertion `core_num < num_cores' failed. }}} To run, do: {{{ export CCTK_TESTSUITE_RUN_COMMAND="/usr/local/apps/openmpi-1.6-intel-2013.2.146/bin/mpirun -np $nprocs $NICE $exe $parfile" OMP_NUM_THREADS=9 make sim-testsuite }}} then select 1 process, and the CarpetWaveToyNewRecover_test_1proc test of CarpetIOHDF5. The test passes with 8 threads and 4 threads (and also 16). It fails also with 9, 11 threads.

#1473: Carpet segfaults in Shutdown ---------------------+------------------------------------------------------ Reporter: knarf | Owner: eschnett Type: defect | Status: new Priority: minor | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------

Comment (by rhaas):

ok. This is non trivial.

The reason for the segfault is actually due to the way Cacuts/Carpet interact when initially creating the group Carpet::timing_levels which is defined as {{{ CCTK_REAL timing_levels TYPE=array DIM=1 SIZE=max_refinement_levels DISTRIB=constant TAGS='checkpoint="no"' { time_level time_level_count } "Per-level timing information" }}} Notice that its SIZE depends on the Carpet parameter max_refinement_levels. This is valid and often used.

The problem occurs when we recover from a checkpoint using a very minimal recover parfile (such as the test does) that does not set Carpet::max_refinement_levels in the recovery parfile. In that case the group is created so early (basically when Carpet is activated) inside of configs/bns_all/bindings/Variables/Carpet.c that max_refinement_levels has not yet been recovered from the checkpoint (since the thorns are just being activated) so still has its default value of 1.

Carpet then contains a loop {{{ for (int rl=0; rl<max_refinement_levels; ++rl) { time_level [rl] = 0.0; time_level_count[rl] = 0.0; } }}} in Carpet/src/Timing.cc line 202. This loop writes past the end of the allocated data for time_level (since only 1 entry was allocated due to max_refinement_level having been 1 when the group was created but being recovered to its original value of 4 from the checkpoint). Since this is just a short array, we don't get a segfault but just overwrite malloc's control data which leads to a double free() error message and eventually a segfault when we free the data in the array during SHUTDOWN.

So: this is actually dangerous and also quite hard to fix since it basically requires that Carpet restores the grid structure of not just grid functions but also of grid arrays.

A simple fix for the test is to add max_refinement_levels = 4 to the recovery parfile.

Thankfully I doubt that this bug is commonly triggered in the wild since we usually use the same parfile for checkpointing and recovery.

I was actually hoping that this would give me some insight into a nasty checkpoint recovery bug on bluewaters, but from the looks of it, this seems unlikely ;-).

#1473: Carpet segfaults in Shutdown ---------------------+------------------------------------------------------ Reporter: knarf | Owner: eschnett Type: defect | Status: new Priority: minor | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------

Comment (by rhaas):

I just ran a quick test with PUGH with a parfile like this: {{{ ActiveThorns = "PUGH IOUtil IOHDF5 Debug IOASCII PUGHSlab"

Cactus::cctk_itlast = 1

IO::checkpoint_id = yes IOHDF5::checkpoint = yes

IO::checkpoint_dir = "cppugh" IO::out_dir = $parfile

debug::gasize = 12

IOASCII::out1d_every = 1 IOASCII::out1d_vars = "Debug::array" }}} where the Debug thorn provides a 1d array whose size is controlled by gasize. I then comment out {{{ debug::gasize = 12 }}} for recovery (the default is 3). PUGH behaves the same way that Carpet does. After recover the grid array has size 3 while the parameter gasize is recovered as size 12.

If we want to allow the possibility to change an array size during recovery, then we need to make sure that CP_RECOVER_PARAMETERS happens before the groups are created (which is very early I think). In particular this means that the recovery code (and any thorns that it depends on) may not assume that their grid functions exist (or at least they are not yet fully created) until after CP_RECOVER_PARAMETERS.

Carpet cannot just resize the grid arrays after recovery since it no longer knows which sizes are affected by parameter changes.

The alternative is what Erik mentioned: to checkpoint the size of (all, not just the checkpoint=yes) arrays and re-instantiate them with this size in which case Carpet can resize them (since the information is part of GroupDynamicData which the driver controls) but which does not allow them to change at recovery time.

#1473: Carpet segfaults in Shutdown ---------------------+------------------------------------------------------ Reporter: knarf | Owner: eschnett Type: defect | Status: new Priority: minor | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------

Comment (by rhaas):

I agree PUGH's and Carpet's current behaviour if perfectly well defined. Since these error are very hard to track down (for example it seems that Philipp's recovery issue on bluewaters is due to this problem after all) I would also suggest that CarpetIOHDF5/IOHDF5 warn with level 1 if the array size in the checkpoint does not agree with the current one.

The typical case (similar thing can happen I think with vectors of SCALARS) where this fails is something like this: {{{ CCTK_REAL myscalar TYPE=ARRAY DIM=1 SIZE=numberofthingummies DISTRIB=constant "some number of things"

for(int i = 0 ; i < numberofthingummies ; i++) { do_something(); } }}} which is very common (eg Carpet does it, the PITTNullCode does it, ZelmaniLeak does it). Right now CarpetIOHDF5 will abort the array grows (I think) since then it can be read only partially from file, while shrinking the array (which is what happened in the testsuite) is not caught and can cause memory corruption.

#1473: Carpet segfaults in Shutdown ---------------------+------------------------------------------------------ Reporter: knarf | Owner: eschnett Type: defect | Status: new Priority: minor | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------

Comment (by eschnett):

I am confused

What is the size of the grid array that is recovered? Is it too small, since the parameters are recovered too late? If so, this should be an error that needs to be corrected.

In the mean time, there should be an error (not just a warning) if Carpet's recovery detects an inconsistency.

#1473: Carpet segfaults in Shutdown ---------------------+------------------------------------------------------ Reporter: knarf | Owner: eschnett Type: defect | Status: reviewed_ok Priority: minor | Milestone: Component: Carpet | Version: development version Resolution: | Keywords: ---------------------+------------------------------------------------------ Changes (by eschnett):

* status: review => reviewed_ok

Comment:

Let's try this.

#1473: Carpet segfaults in Shutdown ---------------------+------------------------------------------------------ Reporter: knarf | Owner: eschnett Type: defect | Status: closed Priority: minor | Milestone: Component: Carpet | Version: development version Resolution: fixed | Keywords: ---------------------+------------------------------------------------------ Changes (by rhaas):

* status: reviewed_ok => closed * resolution: => fixed