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.

Sign up for GitHub

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

Remove memory sanity checks #2164

Closed
wants to merge 5 commits into from
Closed

Remove memory sanity checks #2164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c5e0561
Removes the "memory allocation sanity checks feature"
derobins Oct 15, 2022
4250e00
Added a release note
derobins Oct 15, 2022
f4b47b2
Removes most H5MM wrappers and replaces with C stdlib defines
derobins Oct 15, 2022
e25ed9f
Committing clang-format changes
github-actions[bot] Oct 15, 2022
807bc81
Updates H5allocate_memory and H5resize_memory
derobins Oct 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,14 +560,6 @@ if (HDF5_ENABLE_USING_MEMCHECKER)
set (H5_USING_MEMCHECKER 1)
endif ()

#-----------------------------------------------------------------------------
# Option to indicate internal memory allocation sanity checks are enabled
#-----------------------------------------------------------------------------
option (HDF5_MEMORY_ALLOC_SANITY_CHECK "Indicate that internal memory allocation sanity checks are enabled" OFF)
if (HDF5_MEMORY_ALLOC_SANITY_CHECK)
set (H5_MEMORY_ALLOC_SANITY_CHECK 1)
endif ()

#-----------------------------------------------------------------------------
# Option to enable/disable using pread/pwrite for VFDs
#-----------------------------------------------------------------------------
Expand Down
3 changes: 0 additions & 3 deletions config/cmake/H5pubconf.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,9 +439,6 @@
/* Define to the sub-directory where libtool stores uninstalled libraries. */
#cmakedefine H5_LT_OBJDIR @H5_LT_OBJDIR@

/* Define to enable internal memory allocation sanity checking. */
#cmakedefine H5_MEMORY_ALLOC_SANITY_CHECK @H5_MEMORY_ALLOC_SANITY_CHECK@

/* Define if deprecated public API symbols are disabled */
#cmakedefine H5_NO_DEPRECATED_SYMBOLS @H5_NO_DEPRECATED_SYMBOLS@

Expand Down
1 change: 0 additions & 1 deletion config/cmake/libhdf5.settings.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ Dimension scales w/ new references: @DIMENSION_SCALES_WITH_NEW_REF@
Packages w/ extra debug output: @INTERNAL_DEBUG_OUTPUT@
API Tracing: @HDF5_ENABLE_TRACE@
Using memory checker: @HDF5_ENABLE_USING_MEMCHECKER@
Memory allocation sanity checks: @HDF5_MEMORY_ALLOC_SANITY_CHECK@
Function Stack Tracing: @HDF5_ENABLE_CODESTACK@
Use file locking: @HDF5_FILE_LOCKING_SETTING@
Strict File Format Checks: @HDF5_STRICT_FORMAT_CHECKS@
Expand Down
44 changes: 0 additions & 44 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2689,50 +2689,6 @@ case "X-$USINGMEMCHECKER" in
;;
esac

## ----------------------------------------------------------------------
## Check if they would like to enable the internal memory allocation sanity
## checking code.
##
AC_MSG_CHECKING([whether internal memory allocation sanity checking is used])
AC_ARG_ENABLE([memory-alloc-sanity-check],
[AS_HELP_STRING([--enable-memory-alloc-sanity-check],
[Enable this option to turn on internal memory
allocation sanity checking. This could cause
more memory use and somewhat slower allocation.
This option may also cause issues with HDF5
filter plugins, so should not be enabled if
filters are to be used. This option is orthogonal
to the --enable-using-memchecker option.
[default=no]
])],
[MEMORYALLOCSANITYCHECK=$enableval])

## Allow this variable to be substituted in
## other files (src/libhdf5.settings.in, etc.)
AC_SUBST([MEMORYALLOCSANITYCHECK])

## Set default
if test "X-$MEMORYALLOCSANITYCHECK" = X- ; then
# Should consider enabling this option by default for
# 'developer' builds if that build mode is added in
# the future
MEMORYALLOCSANITYCHECK=no
fi

case "X-$MEMORYALLOCSANITYCHECK" in
X-yes)
AC_DEFINE([MEMORY_ALLOC_SANITY_CHECK], [1],
[Define to enable internal memory allocation sanity checking.])
AC_MSG_RESULT([yes])
;;
X-no)
AC_MSG_RESULT([no])
;;
*)
AC_MSG_ERROR([Unrecognized value: $MEMORYALLOCSANITYCHECK])
;;
esac

## Checkpoint the cache
AC_CACHE_SAVE

Expand Down
1 change: 0 additions & 1 deletion release_docs/INSTALL_CMake.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -773,7 +773,6 @@ HDF5_ENABLE_USING_MEMCHECKER "Indicate that a memory checker is used"
HDF5_GENERATE_HEADERS "Rebuild Generated Files" ON
HDF5_BUILD_GENERATORS "Build Test Generators" OFF
HDF5_JAVA_PACK_JRE "Package a JRE installer directory" OFF
HDF5_MEMORY_ALLOC_SANITY_CHECK "Indicate that internal memory allocation sanity checks are enabled" OFF
HDF5_NO_PACKAGES "Do not include CPack Packaging" OFF
HDF5_PACK_EXAMPLES "Package the HDF5 Library Examples Compressed File" OFF
HDF5_PACK_MACOSX_FRAMEWORK "Package the HDF5 Library in a Frameworks" OFF
Expand Down
22 changes: 19 additions & 3 deletions release_docs/RELEASE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,29 @@ New Features

(ADB - 2022/08/29, HDFFV-11329)

-
- Removed the --enable-memory-alloc-sanity-check Autotools configure option
Copy link
Contributor

Choose a reason for hiding this comment

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

newer notes at top of section, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what I thought...

and the HDF5_MEMORY_ALLOC_SANITY_CHECK CMake option

The memory allocation sanity checks have been removed from the library.

(DER - 2022/10/15)


Library:
--------
-
- Removed the memory allocation sanity checks from the library

This feature added heap canaries to memory allocated inside the library
that were checked on library close. Although potentially helpful, other
methods for investigating memory leaks exist (valgrind, -fsanitize, etc.)
and the heap canaries caused problems with filters and API calls that
return library-allocated memory.

As a part of this change, the H5get_alloc_stats() API call has been
removed from the library. The Autotools and CMake features that
enabled this feature have also been removed (see above).

(DER - 2022/10/15)

Parallel Library:
-----------------
Expand All @@ -75,7 +91,7 @@ New Features
Added Fortran H5Dfill_f, which is fully equivalent to the C API. It accepts pointers,
fill value datatype and datatype of dataspace elements.

(MSB - 2022/10/10, HDFFV-10734.)
(MSB - 2022/10/10, HDFFV-10734)

C++ Library:
------------
Expand Down
60 changes: 10 additions & 50 deletions src/H5.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,11 +541,6 @@ H5_term_library(void)
(void)H5MM_free(tmp_open_stream);
} /* end while */

#if defined H5_MEMORY_ALLOC_SANITY_CHECK
/* Sanity check memory allocations */
H5MM_final_sanity_check();
#endif /* H5_MEMORY_ALLOC_SANITY_CHECK */

/* Reset flag indicating that the library is being shut down */
H5_TERM_GLOBAL = FALSE;

Expand Down Expand Up @@ -716,46 +711,6 @@ H5get_free_list_sizes(size_t *reg_size /*out*/, size_t *arr_size /*out*/, size_t
FUNC_LEAVE_API(ret_value)
} /* end H5get_free_list_sizes() */

/*-------------------------------------------------------------------------
* Function: H5get_alloc_stats
*
* Purpose: Gets the memory allocation statistics for the library, if the
* --enable-memory-alloc-sanity-check option was given when building the
* library. Applications can check whether this option was enabled by
* detecting if the 'H5_MEMORY_ALLOC_SANITY_CHECK' macro is defined. This
* option is enabled by default for debug builds of the library and
* disabled by default for non-debug builds. If the option is not enabled,
* all the values returned with be 0. These statistics are global for the
* entire library, but don't include allocations from chunked dataset I/O
* filters or non-native VOL connectors.
*
* Parameters:
* H5_alloc_stats_t *stats; OUT: Memory allocation statistics
*
* Return: Success: non-negative
* Failure: negative
*
* Programmer: Quincey Koziol
* Saturday, March 7, 2020
*
*-------------------------------------------------------------------------
*/
herr_t
H5get_alloc_stats(H5_alloc_stats_t *stats /*out*/)
{
herr_t ret_value = SUCCEED; /* Return value */

FUNC_ENTER_API(FAIL)
H5TRACE1("e", "x", stats);

/* Call the internal allocation stat routine to get the values */
if (H5MM_get_alloc_stats(stats) < 0)
HGOTO_ERROR(H5E_RESOURCE, H5E_CANTGET, FAIL, "can't get allocation stats")

done:
FUNC_LEAVE_API(ret_value)
} /* end H5get_alloc_stats() */

/*-------------------------------------------------------------------------
* Function: H5__debug_mask
*
Expand Down Expand Up @@ -1222,10 +1177,12 @@ H5allocate_memory(size_t size, hbool_t clear)
FUNC_ENTER_API_NOINIT
H5TRACE2("*x", "zb", size, clear);

if (clear)
ret_value = H5MM_calloc(size);
else
ret_value = H5MM_malloc(size);
if (size != 0) {
if (clear)
ret_value = H5MM_calloc(size);
else
ret_value = H5MM_malloc(size);
}

FUNC_LEAVE_API_NOINIT(ret_value)
} /* end H5allocate_memory() */
Expand Down Expand Up @@ -1262,7 +1219,10 @@ H5resize_memory(void *mem, size_t size)
FUNC_ENTER_API_NOINIT
H5TRACE2("*x", "*xz", mem, size);

ret_value = H5MM_realloc(mem, size);
if (size != 0)
ret_value = H5MM_realloc(mem, size);
else if (mem)
ret_value = H5MM_xfree(mem);
Copy link
Contributor

Choose a reason for hiding this comment

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

I read here https://www.tutorialspoint.com/c_standard_library/c_function_realloc.htm, it said
size − This is the new size for the memory block, in bytes. If it is 0 and ptr points to an existing block of memory, the memory block pointed by ptr is deallocated and a NULL pointer is returned.
Do we need to use H5MM_xfree instead?


FUNC_LEAVE_API_NOINIT(ret_value)
} /* end H5resize_memory() */
Expand Down
Loading