SWIFTSIM / SWIFT

Modern astrophysics and cosmology particle-based code. Mirror of gitlab developments at https://gitlab.cosma.dur.ac.uk/swift/swiftsim
http://www.swiftsim.com
GNU Lesser General Public License v3.0
88 stars 58 forks source link

Tests failing on ARM64: getopt and (un)signed char #17

Closed dacreman closed 3 years ago

dacreman commented 3 years ago

Running make check on an ARM64 system fails to build some tests. One problem is related assigning the return value of getopt to a default char. On ARM64 platforms the default char type is unsigned so compiler warnings are generated which are promoted to errors e.g.

testPeriodicBC.c:408:59: warning: result of comparison of constant -1 with expression of type 'char' is always true [-Wtautological-constant-out-of-range-compare]
  while ((c = getopt(argc, argv, "m:s:h:n:r:t:d:f:v:a:")) != -1) {
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~
1 warning generated.

Changing char to signed char should fix this. Files affected:

testActivePair.c
test27cells.c
test125cells.c
testPeriodicBC.c
test27cellsStars.c

Tested on the Isambard system (Thunder X2 processors) with GCC 9.3.0 and ARM 20.0 (based on LLVM 9.0.1) compilers called from Cray wrappers.

MatthieuSchaller commented 3 years ago

Thanks.

Ah the good memories of the char convention on ARM...

A short-term solution is to allow warnings by configuring SWIFT with --enable-compiler-warnings. It does not solve the actual problem but will let the code compile.

getopt() should return an int however if the posix standard is to be respected: https://man7.org/linux/man-pages/man3/getopt.3.html

LonelyCat124 commented 3 years ago

Hmm, I thought we got all the instances of char and turned them all into int8_t explicitly to avoid this. but we probably missed the ones in the tests. It does like in this case c should be an int though

MatthieuSchaller commented 3 years ago

Yep, I am pushing that change now.

MatthieuSchaller commented 3 years ago

That's now done. This mirror will be updated over night with the fix. The latest master on gitlab is now using the correct type here to get the return value of getopt().

MatthieuSchaller commented 3 years ago

Fixed in 5fdd4e10a8dd819f939df9e8d261bfa952ecffd3.