search

LLNL / sundials

Official development repository for SUNDIALS - a SUite of Nonlinear and DIfferential/ALgebraic equation Solvers. Pull requests are welcome for bug fixes and minor changes.
https://computing.llnl.gov/projects/sundials
BSD 3-Clause "New" or "Revised" License
526 stars 130 forks source link

Do not cast pointers to arrays of differently sized integers. #485

Closed mmuetzel closed 6 months ago

mmuetzel commented 6 months ago

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.

balos1 commented 6 months ago

@mmuetzel Looks like clang-format is failing. You can go to the failed job, download the "diff" artifact and apply the patch with git to fix the styling without installing clang-format.

mmuetzel commented 6 months ago

As long as we can reliably count on compilers to catch the size discrepancy and halt with an error, I think this is great. That said, to head off user GitHub issues complaining that SUNDIALS won't compile, it would be ideal to combine this change with a corresponding check for type compatibility at the CMake level.

I don't know if all compilers will reliably error if the pointer types don't match. They might only emit a warning or don't have a diagnostic for that at all. But with the cast, even compilers that have that diagnostic wouldn't be able to detect that there is an issue. So, I think this is a step in the right direction.

Do you prefer to add the check during configuration in this PR? Or would it be ok if I opened a separate PR for that?

Looks like clang-format is failing. You can go to the failed job, download the "diff" artifact and apply the patch with git to fix the styling without installing clang-format.

Thanks for the hint. I downloaded that diff and merged it into the commit for this PR.

drreynolds commented 6 months ago

I don't know if all compilers will reliably error if the pointer types don't match. They might only emit a warning or don't have a diagnostic for that at all. But with the cast, even compilers that have that diagnostic wouldn't be able to detect that there is an issue. So, I think this is a step in the right direction.

I agree.

Do you prefer to add the check during configuration in this PR? Or would it be ok if I opened a separate PR for that?

I'm okay with either. Perhaps another developer has a preference?

balos1 commented 6 months ago

@mmuetzel Please add the CMake check to this PR.

mmuetzel commented 6 months ago

The new commit adds a configure check for the size of SuiteSparse_long if SUNDIALS_INDEX_SIZE is "64" and fails if it is something other than 8 (i.e., a 64-bit integer). I also added a note about that change to the changelogs. Is the wording ok?