WIP: Pass H5MM calls to stdlib counterparts by default

[jhendersonHDF]

Profiling of a synthetic benchmark for the Mathworks folks shows that the pass through from H5MM_malloc -> HDmalloc and similar adds a slight bit of overhead. Since the H5MM stuff generally doesn't add anything but overhead when the memory alloc sanity check option is off, let's just map those directly to their stdlib counterparts by default.

The only real functional change that should be here is that H5MM_realloc would be non-portable if you call it like H5MM_realloc(ptr, 0), but "let's not do that". I've left H5MM_xfree, xfree_const and xstrdup as they are, since they rely on different NULL pointer behavior from their stdlib counterparts.

[derobins]

I think these test failures are due to a library bug that's just being exposed here. clang emits warnings in the long double code in H5Tconv.c.

[qkoziol]

I understand the motivation and generally like this change. But, I'm concerned that the #ifdef'd code will languish and then rot (i.e. not work correctly) without a regular (if perhaps not terribly frequent) automated regression test that sets H5_MEMORY_ALLOC_SANITY_CHECK. Is that possible?

[jhendersonHDF]

I understand the motivation and generally like this change. But, I'm concerned that the #ifdef'd code will languish and then rot (i.e. not work correctly) without a regular (if perhaps not terribly frequent) automated regression test that sets H5_MEMORY_ALLOC_SANITY_CHECK. Is that possible?

I think this would certainly be a good candidate for a github action, either a compile-only or full workflow. I wish I could turn it on by default for my developer build mode PR, but unfortunately that will cause problems with filter plugins and probably confuse some future new developers.

[jhendersonHDF]

Marking this PR as a draft for now, since it looks like it's exposing an issue in the H5T code for long double -> float and long double -> double conversions.

[derobins]

I feel like the memory alloc sanity check code is unnecessary and we should just have a GitHub action that uses -fsanitize or valgrind or whatnot. We shouldn't be reinventing the wheel in HDF5.

[derobins]

The failing tests might be fixed by #1956

[qkoziol]

I feel like the memory alloc sanity check code is unnecessary and we should just have a GitHub action that uses -fsanitize or valgrind or whatnot. We shouldn't be reinventing the wheel in HDF5.

This is valuable when debugging on platforms without those tools.

[qkoziol]

I feel like the memory alloc sanity check code is unnecessary and we should just have a GitHub action that uses -fsanitize or valgrind or whatnot. We shouldn't be reinventing the wheel in HDF5.

This is valuable when debugging on platforms without those tools.

Which includes some cloud platforms I know about, e.g. AWS Graviton, etc...

[gnuoyd]

On Sun, Jul 31, 2022 at 06:49:46AM -0700, Quincey Koziol wrote: > > I feel like the memory alloc sanity check code is unnecessary and we should just have a GitHub action that uses -fsanitize or valgrind or whatnot. We shouldn't be reinventing the wheel in HDF5. > > This is valuable when debugging on platforms without those tools. Which includes some cloud platforms I know about, e.g. AWS Graviton, etc...

What makes you think that AWS Graviton processors cannot use sanitizers and such? Dave

[derobins]

I'll use my branch and PR for fixing the underlying MacOS type conversion bug