Merge pull request #511 from galacticusorg/fixOutputRank2ExtendSegFault

Fix a segfault associated with output of `rank?VarLen` datasets
galacticusorg · Dec 15, 2023 · 59f25e7 · 59f25e7
2 parents 075e118 + 73004ce
commit 59f25e7
Show file tree

Hide file tree

Showing 3 changed files with 546 additions and 13 deletions.
diff --git a/.github/workflows/cicd.yml b/.github/workflows/cicd.yml
@@ -8815,6 +8815,42 @@ jobs:
           ./Galacticus.exe testSuite/regressions/treeWithNoPrimaryProgenitor.xml 2>&1 | tee test.log
           ! grep -q FAIL test.log
       - run: echo "This job's status is ${{ job.status }}."
+  Regressions-Output-Rank2VarLen-SegFault:
+    runs-on: ubuntu-latest
+    container: ghcr.io/galacticusorg/buildenv:latest
+    needs: Build-Executable-Linux
+    steps:
+      - run: echo "The job was automatically triggered by a ${{ github.event_name }} event."
+      - run: echo "This job is now running on a ${{ runner.os }} server."
+      - run: echo "The name of the branch is ${{ github.ref }} and the repository is ${{ github.repository }}."
+      - name: Check out repository code
+        uses: actions/checkout@v4
+      - name: Check out repository datasets
+        uses: actions/checkout@v4      
+        with:
+          repository: galacticusorg/datasets
+          path: datasets
+      - run: echo "The ${{ github.repository }} repository has been cloned to the runner."
+      - name: "Set environmental variables"
+        run: |
+          echo "GALACTICUS_EXEC_PATH=$GITHUB_WORKSPACE" >> $GITHUB_ENV
+          echo "GALACTICUS_DATA_PATH=$GITHUB_WORKSPACE/datasets" >> $GITHUB_ENV
+      - name: Download executables
+        uses: actions/download-artifact@v3
+        with:
+          name: galacticus-exec
+      - name: Create test suite output directory
+        run: mkdir -p $GALACTICUS_EXEC_PATH/testSuite/outputs
+      - name: Run test
+        run: |
+          cd $GALACTICUS_EXEC_PATH
+          git config --global --add safe.directory $GALACTICUS_EXEC_PATH
+          chmod u=wrx ./Galacticus.exe
+          mkdir -p testSuite/outputs/regressions
+          set -o pipefail
+          ./Galacticus.exe testSuite/regressions/outputRank2ExtendSegFault.xml 2>&1 | tee test.log
+          ! grep -q FAIL test.log
+      - run: echo "This job's status is ${{ job.status }}."
   Regressions-Bar-Instability-FPE:
     runs-on: ubuntu-latest
     container: ghcr.io/galacticusorg/buildenv:latest
@@ -10525,6 +10561,7 @@ jobs:
              Regressions-Tree-With-Initial-Satellite-In-Progenitorless-Host,
              Regressions-Tree-With-No-Primary-Progenitor,
              Regressions-Bar-Instability-FPE,
+             Regressions-Output-Rank2VarLen-SegFault,
              Test-Methods-01,
              Test-Methods-02,
              Test-Methods-03,

diff --git a/source/merger_trees.outputter.standard.F90 b/source/merger_trees.outputter.standard.F90
@@ -477,7 +477,7 @@ subroutine standardOutput(self,node,time)
           doubleArray =extractor_%extract       (node,time,instance)
           do i=1,+extractor_%elementCount(                  time)
              if (     allocated(self%doubleProperty (doubleProperty +i)%scalar))                          deallocate(self%doubleProperty (doubleProperty +i)%scalar)
-             if (     allocated(self%doubleProperty (doubleProperty +i)%rank1 )) then
+             if (     allocated(self%doubleProperty (doubleProperty +i)%rank1 )) then 
                 if (size(self%doubleProperty (doubleProperty +i)%rank1,dim=1) /= size(doubleArray,dim=1)) deallocate(self%doubleProperty (doubleProperty +i)%rank1 )
              end if
              if (.not.allocated(self%doubleProperty (doubleProperty +i)%rank1)) allocate(self%doubleProperty (doubleProperty +i)%rank1(size(doubleArray,dim=1),self%doubleBufferSize))
@@ -513,7 +513,11 @@ subroutine standardOutput(self,node,time)
              if (     allocated(self%doubleProperty (doubleProperty +i)%rank1VarLen))                          deallocate(self%doubleProperty (doubleProperty +i)%rank1VarLen)
              if (.not.allocated(self%doubleProperty (doubleProperty +i)%rank2VarLen)) allocate(self%doubleProperty (doubleProperty +i)%rank2VarLen(self%doubleBufferSize))
              if (associated(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row)) then
-                if (size(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row) /= size(doubleArray2D,dim=1)) deallocate(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row)
+                if     (                                                                                                                            &
+                     &   size(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row,dim=1) /= size(doubleArray2D,dim=1) &
+                     &  .or.                                                                                                                        &
+                     &   size(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row,dim=2) /= size(doubleArray2D,dim=2) &
+                     & ) deallocate(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row)
              end if
              if (.not.associated(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row)) then
                 allocate(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row(size(doubleArray2D,dim=1),size(doubleArray2D,dim=2)))
@@ -573,7 +577,11 @@ subroutine standardOutput(self,node,time)
                 if (.not.allocated(self%doubleProperty(doubleProperty +i)%rank2VarLen)) allocate(self%doubleProperty (doubleProperty +i)%rank2VarLen(self%doubleBufferSize))
                 if (associated(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row)) then
                    shape_=doubleProperties(i)%shape()
-                   if (size(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row) /= shape_(1)) deallocate(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row)
+                   if     (                                                                                                            &
+                        &   size(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row,dim=1) /= shape_(1) &
+                        &  .or.                                                                                                        &
+                        &   size(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row,dim=2) /= shape_(2) &
+                        & ) deallocate(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row)
                    deallocate(shape_)
                 end if
                 if (.not.associated(self%doubleProperty (doubleProperty +i)%rank2VarLen (self%doubleBufferCount )%row)) then
@@ -828,28 +836,52 @@ subroutine standardExtendDoubleBuffer(self)
     !!{
     Extend the size of the double buffer.
     !!}
+    use :: IO_HDF5, only : hdf5VarDouble , hdf5VarDouble2D
     implicit none
     class           (mergerTreeOutputterStandard), intent(inout)                 :: self
     double precision                             , allocatable  , dimension(:  ) :: scalarTemporary
     double precision                             , allocatable  , dimension(:,:) :: rank1Temporary
-    integer                                                                      :: i
+    type            (hdf5VarDouble              ), allocatable  , dimension(:  ) :: rank1VarLenTemporary
+    type            (hdf5VarDouble2D            ), allocatable  , dimension(:  ) :: rank2VarLenTemporary
+    integer                                                                      :: i                   , j
 
     do i=1,size(self%doubleProperty)
-       if (allocated(self%doubleProperty(i)%scalar)) then
-          call move_alloc(self%doubleProperty(i)%scalar,scalarTemporary)
-          allocate(self%doubleProperty(i)%scalar(                           self%doubleBufferSize+standardBufferSizeIncrement))
-          self%doubleProperty(i)%scalar(  1:self%doubleBufferSize)=scalarTemporary
+       if (allocated(self%doubleProperty(i)%scalar    )) then
+          call move_alloc(self%doubleProperty(i)%scalar    ,scalarTemporary)
+          allocate(self%doubleProperty(i)%scalar    (                           self%doubleBufferSize+standardBufferSizeIncrement))
+          self%doubleProperty(i)%scalar    (  1:self%doubleBufferSize)=scalarTemporary
           deallocate(scalarTemporary)
        end if
-       if (allocated(self%doubleProperty(i)%rank1 )) then
-          call move_alloc(self%doubleProperty(i)%rank1 ,rank1Temporary )
-          allocate(self%doubleProperty(i)%rank1 (size(rank1Temporary,dim=1),self%doubleBufferSize+standardBufferSizeIncrement))
-          self%doubleProperty(i)%rank1 (:,1:self%doubleBufferSize)=rank1Temporary
+       if (allocated(self%doubleProperty(i)%rank1     )) then
+          call move_alloc(self%doubleProperty(i)%rank1     ,rank1Temporary )
+          allocate(self%doubleProperty(i)%rank1     (size(rank1Temporary,dim=1),self%doubleBufferSize+standardBufferSizeIncrement))
+          self%doubleProperty(i)%rank1     (:,1:self%doubleBufferSize)=rank1Temporary
           deallocate(rank1Temporary )
        end if
+       if (allocated(self%doubleProperty(i)%rank1VarLen)) then
+          call move_alloc(self%doubleProperty(i)%rank1VarLen,rank1VarLenTemporary)
+          allocate(self%doubleProperty(i)%rank1VarLen(                           self%doubleBufferSize+standardBufferSizeIncrement))
+          self%doubleProperty(i)%rank1VarLen(  1:self%doubleBufferSize)=rank1VarLenTemporary
+          ! Nullify row pointers in the temporary array to avoid the associated data being destroyed when our temporary array is
+          ! deallocated.
+          do j=1,size(rank1VarLenTemporary)
+             rank1VarLenTemporary(j)%row => null()
+          end do
+          deallocate(rank1VarLenTemporary)
+       end if
+       if (allocated(self%doubleProperty(i)%rank2VarLen)) then
+          call move_alloc(self%doubleProperty(i)%rank2VarLen,rank2VarLenTemporary)
+          allocate(self%doubleProperty(i)%rank2VarLen(                           self%doubleBufferSize+standardBufferSizeIncrement))
+          self%doubleProperty(i)%rank2VarLen(  1:self%doubleBufferSize)=rank2VarLenTemporary
+          ! Nullify row pointers in the temporary array to avoid the associated data being destroyed when our temporary array is
+          ! deallocated.
+          do j=1,size(rank2VarLenTemporary)
+             rank2VarLenTemporary(j)%row => null()
+          end do
+          deallocate(rank2VarLenTemporary)
+       end if
     end do
     self%doubleBufferSize=self%doubleBufferSize+standardBufferSizeIncrement
-
     return
   end subroutine standardExtendDoubleBuffer