1. The Enzo Project
  2. Untitled project
  3. enzo-dev
  4. Pull requests

Pull requests

#180 Merged
Repository
samskillman
Branch
week-of-code
Repository
enzo
Branch
week-of-code

Uninitialized memory bugfixes

Author
  1. Sam Skillman
Reviewers
Description

The motivation for these changes is that when running a cosmological simulation that was relatively deep, and possibly many SUBling fluxes interfaces (siblings children fluxes), I was encountering crashes that were not easily explained by the usual suspects. I've narrowed this down to an uninitialized (Left/Right)Flux(Start/End)GlobalIndex in SUBling fluxes. I believe this only appears when running with CorrectParentBoundaryFlux = 1 (which is not the default, but in my mind should be). I've set up initial values for these to be 0 in a new InitializeFluxes function, but I'd like dcollins4096 to chime in on whether this is a dangerous thing to set.

In the process of finding this bug by running valgrind on >= 2 mpi processes with --track-origins=yes, I found found a bunch of other warnings/errors that crop up on various problem types. This does not completely make enzo "valgrind clean" but it is quite close now.

One of the deeper changes that I'd like Greg Bryan or John Wise to take a look at is the change in intvar.F. I believe this qr value was causing a few of the warnings, and Nathan Goldbaum actually pointed this out a long time ago on the mailing list.

Finally, in order to initialize the values of some of the arrays, I used the syntax float[size](), which is a way to initialize the values of an array to the default (which I think always means 0 for float/int) value. Any warnings to not do this and manually loop through size are welcome.

  • Learn about pull requests

Comments (7)

  1. Nathan Goldbaum

    Thank you for fixing the valgrind issues, this gets rid of all of the valgrind noise I commonly run into. I've been running this patch on the AgoraRestart problem on my fork with CorrectParentBoundaryFlux=1 and have noticed no issues.

  2. Greg Bryan

    Thanks Sam -- this looks very nice! I haven't tried them out, but I'm happy with your changes. Regarding the intvar.F changes, I agree that there is a problem with the previous code. In fact, I'm pretty sure it is confined to the CHECK_LR loop, which should (I think) have an end index of i2+1 instead of i2+2. I suspect that change would solve the issue, however, initializing the arrays to zero also solves it (extremely nitpicking note: technically you use F90 notation in this .F routine).

    1. John Wise

      Hi Sam, your changes look good to me, but I haven't tested them either. I remember I had the bounds of the CHECK_LR loop to be i1-1 -> i2+2 so that it has the same bounds as the original ql/qr loop (line 94).

  3. dcollins4096

    I looked at the changes and they seem good to me. I don't completely understand why it works, because it seems like the variable that's getting initialized is then again initialized in GetProjectedBoundaryFluxes. I have a small fear that there's something else going on in addition, but this clearly makes the code run where it otherwise fails, so it's an improvement.

  4. Sam Skillman author

    Heads up -- I think Dave may have hit the "update pr" button (i could kick bitbucket for making it seem like a tiny action when it can actually do harm), which unfortunately grabbed my attempt at a ngp_deposit_c.c. I think it is okay, but I haven't tested it other than just running. I can spend some time verifying that it is correct, and will update the PR to include that as part of the description.

      1. Sam Skillman author

        No worries -- I am pretty sure everyone hits that button because it is so similar to the "this PR has been updated, click to refresh" button. It's happened several times over on yt.