Skip to content

Commit

Permalink
Do not cast pointers to arrays of differently sized integers. (#485)
Browse files Browse the repository at this point in the history
Casting between `int64_t *` and `int32_t *` does *not* maintain the
values in the array. Instead, it tells the compiler to interpret the
memory at that pointer as an array of a different type (i.e., two
`int32_t` elements "become" one `int64_t` element). That can lead to all
kinds of errors and is likely not what was intended.

Remove the pointer casts to allow the compiler to emit an error on
compile-time instead of potentially causing, e.g., an array overflow on
runtime if `sunindextype` has a different size from `KLU_INDEXTYPE`.

---------

Signed-off-by: Markus Mützel <markus.muetzel@gmx.de>
Co-authored-by: Cody Balos <balos1@llnl.gov>
  • Loading branch information
2 people authored and gardner48 committed Jun 20, 2024
1 parent 9d92648 commit 0056cb8
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 26 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ allocated.
Fix bug on LLP64 platforms (like Windows 64-bit) where `KLU_INDEXTYPE` could be
32 bits wide even if `SUNDIALS_INT64_T` is defined.

Check if size of `SuiteSparse_long` is 8 if the size of `sunindextype` is 8
when using KLU.

## Changes to SUNDIALS in release v7.0.0

### Major Feature
Expand Down
16 changes: 16 additions & 0 deletions cmake/tpl/SundialsKLU.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,22 @@ message(STATUS "KLU_INCLUDE_DIR: ${KLU_INCLUDE_DIR}")
if(KLU_FOUND AND (NOT KLU_WORKS))
# Do any checks which don't require compilation first.

if(SUNDIALS_INDEX_SIZE MATCHES "64")
# Check size of SuiteSparse_long
include(CheckTypeSize)
set(save_CMAKE_EXTRA_INCLUDE_FILES ${CMAKE_EXTRA_INCLUDE_FILES})
list(APPEND CMAKE_EXTRA_INCLUDE_FILES "klu.h")
set(save_CMAKE_REQUIRED_INCLUDES ${CMAKE_REQUIRED_INCLUDES})
list(APPEND CMAKE_REQUIRED_INCLUDES ${KLU_INCLUDE_DIR})
check_type_size("SuiteSparse_long" SIZEOF_SUITESPARSE_LONG)
set(CMAKE_EXTRA_INCLUDE_FILES ${save_CMAKE_EXTRA_INCLUDE_FILES})
set(CMAKE_REQUIRED_INCLUDES ${save_CMAKE_REQUIRED_INCLUDES})
message(STATUS "Size of SuiteSparse_long is ${SIZEOF_SUITESPARSE_LONG}")
if(NOT SIZEOF_SUITESPARSE_LONG EQUAL "8")
print_error("Size of 'sunindextype' is 8 but size of 'SuiteSparse_long' is ${SIZEOF_SUITESPARSE_LONG}. KLU cannot be used.")
endif()
endif()

# Create the KLU_TEST directory
set(KLU_TEST_DIR ${PROJECT_BINARY_DIR}/KLU_TEST)
file(MAKE_DIRECTORY ${KLU_TEST_DIR})
Expand Down
3 changes: 3 additions & 0 deletions doc/shared/RecentChanges.rst
Original file line number Diff line number Diff line change
Expand Up @@ -76,3 +76,6 @@ where not allocated.

Fix bug on LLP64 platforms (like Windows 64-bit) where ``KLU_INDEXTYPE`` could be
32 bits wide even if ``SUNDIALS_INT64_T`` is defined.

Check if size of ``SuiteSparse_long`` is 8 if the size of ``sunindextype`` is 8
when using KLU.
39 changes: 13 additions & 26 deletions src/sunlinsol/klu/sunlinsol_klu.c
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,6 @@
#define COMMON(S) (KLU_CONTENT(S)->common)
#define SOLVE(S) (KLU_CONTENT(S)->klu_solver)

/*
* -----------------------------------------------------------------
* typedef to handle pointer casts from sunindextype to KLU type
* -----------------------------------------------------------------
*/

#if defined(SUNDIALS_INT64_T)
#define KLU_INDEXTYPE int64_t
#else
#define KLU_INDEXTYPE int
#endif

/*
* -----------------------------------------------------------------
* exported functions
Expand Down Expand Up @@ -266,10 +254,9 @@ int SUNLinSolSetup_KLU(SUNLinearSolver S, SUNMatrix A)
{
/* Perform symbolic analysis of sparsity structure */
if (SYMBOLIC(S)) { sun_klu_free_symbolic(&SYMBOLIC(S), &COMMON(S)); }
SYMBOLIC(S) =
sun_klu_analyze(SUNSparseMatrix_NP(A),
(KLU_INDEXTYPE*)SUNSparseMatrix_IndexPointers(A),
(KLU_INDEXTYPE*)SUNSparseMatrix_IndexValues(A), &COMMON(S));
SYMBOLIC(S) = sun_klu_analyze(SUNSparseMatrix_NP(A),
SUNSparseMatrix_IndexPointers(A),
SUNSparseMatrix_IndexValues(A), &COMMON(S));
if (SYMBOLIC(S) == NULL)
{
LASTFLAG(S) = SUN_ERR_EXT_FAIL;
Expand All @@ -280,8 +267,8 @@ int SUNLinSolSetup_KLU(SUNLinearSolver S, SUNMatrix A)
Compute the LU factorization of the matrix
------------------------------------------------------------*/
if (NUMERIC(S)) { sun_klu_free_numeric(&NUMERIC(S), &COMMON(S)); }
NUMERIC(S) = sun_klu_factor((KLU_INDEXTYPE*)SUNSparseMatrix_IndexPointers(A),
(KLU_INDEXTYPE*)SUNSparseMatrix_IndexValues(A),
NUMERIC(S) = sun_klu_factor(SUNSparseMatrix_IndexPointers(A),
SUNSparseMatrix_IndexValues(A),
SUNSparseMatrix_Data(A), SYMBOLIC(S), &COMMON(S));
if (NUMERIC(S) == NULL)
{
Expand All @@ -294,8 +281,8 @@ int SUNLinSolSetup_KLU(SUNLinearSolver S, SUNMatrix A)
else
{ /* not the first decomposition, so just refactor */

retval = sun_klu_refactor((KLU_INDEXTYPE*)SUNSparseMatrix_IndexPointers(A),
(KLU_INDEXTYPE*)SUNSparseMatrix_IndexValues(A),
retval = sun_klu_refactor(SUNSparseMatrix_IndexPointers(A),
SUNSparseMatrix_IndexValues(A),
SUNSparseMatrix_Data(A), SYMBOLIC(S), NUMERIC(S),
&COMMON(S));
if (retval == 0)
Expand All @@ -321,7 +308,7 @@ int SUNLinSolSetup_KLU(SUNLinearSolver S, SUNMatrix A)
{
/* Condition number may be getting large.
Compute more accurate estimate */
retval = sun_klu_condest((KLU_INDEXTYPE*)SUNSparseMatrix_IndexPointers(A),
retval = sun_klu_condest(SUNSparseMatrix_IndexPointers(A),
SUNSparseMatrix_Data(A), SYMBOLIC(S), NUMERIC(S),
&COMMON(S));
if (retval == 0)
Expand All @@ -333,12 +320,12 @@ int SUNLinSolSetup_KLU(SUNLinearSolver S, SUNMatrix A)
if (COMMON(S).condest > (ONE / uround_twothirds))
{
/* More accurate estimate also says condition number is
large, so recompute the numeric factorization */
large, so recompute the numeric factorization */
sun_klu_free_numeric(&NUMERIC(S), &COMMON(S));
NUMERIC(S) =
sun_klu_factor((KLU_INDEXTYPE*)SUNSparseMatrix_IndexPointers(A),
(KLU_INDEXTYPE*)SUNSparseMatrix_IndexValues(A),
SUNSparseMatrix_Data(A), SYMBOLIC(S), &COMMON(S));
NUMERIC(S) = sun_klu_factor(SUNSparseMatrix_IndexPointers(A),
SUNSparseMatrix_IndexValues(A),
SUNSparseMatrix_Data(A), SYMBOLIC(S),
&COMMON(S));
if (NUMERIC(S) == NULL)
{
LASTFLAG(S) = SUN_ERR_EXT_FAIL;
Expand Down

0 comments on commit 0056cb8

Please sign in to comment.