DrTimothyAldenDavis / SuiteSparse

The official SuiteSparse library: a suite of sparse matrix algorithms authored or co-authored by Tim Davis, Texas A&M University.
https://people.engr.tamu.edu/davis/suitesparse.html
Other
1.14k stars 258 forks source link

GCC warning about overflow in SPEX #708

Closed mmuetzel closed 7 months ago

mmuetzel commented 7 months ago

GCC warns about an out of bounds access in SPEX. See, e.g., the Ubuntu runner in CI:

[ 38%] Building C object SPEX/CMakeFiles/SPEX_static.dir/SPEX_Util/Source/SPEX_matrix_allocate.c.o
In file included from /home/runner/work/SuiteSparse/SuiteSparse/SPEX/SPEX_Util/Source/SPEX_matrix_allocate.c:22:
/home/runner/work/SuiteSparse/SuiteSparse/SPEX/SPEX_Util/Source/SPEX_matrix_allocate.c: In function ‘SPEX_matrix_allocate’:
/home/runner/work/SuiteSparse/SuiteSparse/SPEX/SPEX_Util/Source/SPEX_matrix_allocate.c:98:17: warning: ‘SPEX_mpq_init’ accessing 32 bytes in a region of size 4 [-Wstringop-overflow=]
   98 |     SPEX_CHECK (SPEX_mpq_init (A->scale)) ;
/home/runner/work/SuiteSparse/SuiteSparse/SPEX/SPEX_Util/Source/spex_util_internal.h:100:13: note: in definition of macro ‘SPEX_CHECK’
  100 |     info = (method) ;           \
      |             ^~~~~~
/home/runner/work/SuiteSparse/SuiteSparse/SPEX/SPEX_Util/Source/SPEX_matrix_allocate.c:98:17: note: referencing argument 1 of type ‘__mpq_struct *’
   98 |     SPEX_CHECK (SPEX_mpq_init (A->scale)) ;
/home/runner/work/SuiteSparse/SuiteSparse/SPEX/SPEX_Util/Source/spex_util_internal.h:100:13: note: in definition of macro ‘SPEX_CHECK’
  100 |     info = (method) ;           \
      |             ^~~~~~
In file included from /home/runner/work/SuiteSparse/SuiteSparse/SPEX/SPEX_Util/Source/spex_util_internal.h:108,
                 from /home/runner/work/SuiteSparse/SuiteSparse/SPEX/SPEX_Util/Source/SPEX_matrix_allocate.c:22:
/home/runner/work/SuiteSparse/SuiteSparse/SPEX/Include/SPEX.h:802:11: note: in a call to function ‘SPEX_mpq_init’
  802 | SPEX_info SPEX_mpq_init (mpq_t x) ;
      |           ^~~~~~~~~~~~~

I don't know if this is a bogus warning or if there is really an out-of-bounds issue.

DrTimothyAldenDavis commented 7 months ago

My SPEX co-authors and I have seen this too. We've looked at it closely and we are fairly sure that it is a spurious warning.

A->scale is an mpq_t type, and so is the input to SPEX_mpq_init. We have also tested this code under valgrind, and it doesn't report any errors.

It's a puzzle as to why some gcc versions (not all) complain about this statement. gcc 9.4.0 and gcc 13.2.0 don't complain, at least on my machine with MPFR 4.2.0, but gcc 11.4.0 does complain in the CI (with MPFR 4.1.0).

That leads me to believe that the warning is spurious.

DrTimothyAldenDavis commented 7 months ago

I tried it with MPFR 4.1.0 and gcc 13.2.0, and got no warnings, on my local machine.

DrTimothyAldenDavis commented 7 months ago

Correction... the function mpq_init is in GMP, not MPFR. SPEX_mpq_init is a simple wrapper around mpq_init, described here: https://gmplib.org/manual/Initializing-Rationals

and it looks to me that we are calling it correctly. sizeof(mpq_t) and sizeof (A->scale) are both 32.

DrTimothyAldenDavis commented 7 months ago

I installed gcc 11.4.0 on my desktop via spack. Using it to compile SPEX causes this warning to be emitted. If instead I use gcc 9.4.0 or gcc 13.2.0 (the latter being the most recent version of gcc on spack), then I get no warning.

I tried -Wstringop-overflow=n for n = 0 to 4 (I think 0 disables the warning but I'm not sure). n=0 makes the warning go away with gcc 11.4.0.

gcc 13.2.0 reports no warning, regardless of the -Wstringop-overflow=n flag, for any n.

Since a more recent gcc is silent about this (13.2.0), and an older gcc complains about it (11.4.0), and a still older gcc is also silent (9.4.0) I think this must be a spurious warning from an overly zealous check in gcc 11.4.0 that is now gone in gcc 13.2.0.

DrTimothyAldenDavis commented 7 months ago

I think it's because of this bug in gcc 11, that was fixed in gcc 12:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101854

DrTimothyAldenDavis commented 7 months ago

I fixed this issue just now, by simply disabling -Wstringop-overflow for gcc 11 when compiling SPEX. See https://github.com/DrTimothyAldenDavis/SuiteSparse/pull/716

DrTimothyAldenDavis commented 7 months ago

I think this is fixed now; I just need to check the CI results.

DrTimothyAldenDavis commented 7 months ago

The logs look fine now.