Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing: Set MALLOC_PERTURB_ to 1 #1492

Merged
merged 2 commits into from
Sep 13, 2021

Conversation

marshallward
Copy link
Collaborator

The MALLOC_PERTURB_ parameter was used to initialize allocated arrays to
unphysical values. Previously, we had set this to 256 which would
initialize bytes to 0xff, and indirectly set all floats to NaN.

However, MALLOC_PERTURB_ was undocumnted for values greater than 255,
and we were relying on undefined behavior. In newer versions of glibc,
a value of 256 does nothing and is equivalent to a value of 0.

This patch changes the MALLOC_PERTURB_ value to 1, which sets bytes to
0xfe and tends to initialize them to an unphysically large value.

Unfortunately we have temporarily lost the ability to initialize to NaN,
but for now this is probably the best we can do.

(Note that other compilers like Intel can explicitly initialize
allocatables to NaN, so this update is more specific to the GCC
configuration of our GitHub Actions setup than the test suite.)

The MALLOC_PERTURB_ parameter was used to initialize allocated arrays to
unphysical values.  Previously, we had set this to 256 which would
initialize bytes to 0xff, and indirectly set all floats to NaN.

However, MALLOC_PERTURB_ was undocumnted for values greater than 255,
and we were relying on undefined behavior.  In newer versions of glibc,
a value of 256 does nothing and is equivalent to a value of 0.

This patch changes the MALLOC_PERTURB_ value to 1, which sets bytes to
0xfe and tends to initialize them to an unphysically large value.

Unfortunately we have temporarily lost the ability to initialize to NaN,
but for now this is probably the best we can do.

(Note that other compilers like Intel can explicitly initialize
allocatables to NaN, so this update is more specific to the GCC
configuration of our GitHub Actions setup than the test suite.)
@codecov
Copy link

codecov bot commented Aug 31, 2021

Codecov Report

Merging #1492 (4749308) into dev/gfdl (87aad26) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           dev/gfdl    #1492   +/-   ##
=========================================
  Coverage     29.03%   29.03%           
=========================================
  Files           236      236           
  Lines         71491    71491           
=========================================
  Hits          20756    20756           
  Misses        50735    50735           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87aad26...4749308. Read the comment docs.

@marshallward
Copy link
Collaborator Author

I just noticed these are still called "NaN" tests, even though we are no longer doing NaN initialization. Also it's not a great idea to inject glibc-specific flags into a supposedly platform-agnostic Makefile.

I will think on this for a bit. If I can't find a better solution then I'll close the PR.

Copy link
Collaborator

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a simple enough change, and it is well motivated by the descriptive comments. Given that this impacts only the TC testing, which has passed, I am going to approve this particular PR without requiring the redundant pipeline testing.

@Hallberg-NOAA Hallberg-NOAA merged commit cabe0d1 into mom-ocean:dev/gfdl Sep 13, 2021
@marshallward marshallward deleted the malloc_perturb_update branch February 3, 2022 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants