Fix the compiler warnings reported by CI, and build the examples - #742
Conversation
|
This looks good, but I still think that using plain |
|
@mreineck I totally agree. We should not use abs in .cpp code. The issue with |
|
I turned that warning in an error. To mitigate the issue. Let me try having a go at fixing this. |
|
When I include std:: in testutils, as in this new version, tests pass fine on my linux machine with GCC 11.4. Note that in some sources in Happy to insert |
|
The "illegal" errors are unrelated, I'm quite sure. I just have no idea what is causing them. |
@ahbarnett I'm not sure why CI breaks so often. One possible reason is, to accelerate the ci runs, @DiamonDinoia uses sccache in ci to cache the compiled object files(.o) from previous ci runs, if the previous ci run was ran on avx512 machine with flag -march=native, and the next ci run re-uses the cached object files and run on avx2 machine, there might be illegal instructions. What is the "key" that sccache used to distinguish changes(currently how sccache decide to reuse .o files or not), besides the C++ flags, does sscache distinguish cpu instructions? Currently, it seems with "-march=native", the ci may break with reused object files, I guess. |
|
Oh! If files compiled with But CI should definitely never do this ... And "illegal instruction" should not be truncated like this ... strange, |
Yes, not sure if it's sccache problem. If it is, we should tell it, machine changed, with -march=native, please rerun no reuse... |
Worth opening a separate issue for it. We could add the instruction set from lscpu to the key. Or manually set the vector width. This allows to test different vectorizations same as xsimd does. PS: @lu1and10 that's a very good guess! I was wondering what the problem was too. |
Good idea, we can retain the ci speedup from sccache and at the same time avoid illegal instructions. |
|
@mreineck I went ahead and remove I also fixed examples and one documentation file containing I ran this though an LLM to tell me if I missed something and it says it is okay. |
|
@blackwer I am a confused by |
|
Hi @DiamonDinoia, this is of course the cleanest approach to fix the issue (and also prevent it from happening again)! I wouldn't say that That said, I like the change very much as it is, even though it touches a lot of files! |
| int m = 0; | ||
| for (int m2 = -(N2 / 2); m2 <= (N2 - 1) / 2; ++m2) // loop in correct order over F | ||
| for (int m1 = -(N1 / 2); m1 <= (N1 - 1) / 2; ++m1) | ||
| ct += fkstart[m++] * exp(J * (m1 * x[jt] + m2 * y[jt])); // crude direct |
There was a problem hiding this comment.
Here, the exp seems to be missing a std::. Don't put too much trust in LLMs :)
There was a problem hiding this comment.
grep was my best friend here...
This whole package was written with as |
ahbarnett
left a comment
ahbarnett
left a comment
There was a problem hiding this comment.
Thanks for doing this. It's good you enforced the correct abs and exp etc in the library source and tests.
I feel that it makes the example codes much less readable. I would prefer the examples to simply using namespace std; because they are so limited, short, and designed for humans to read (Would it be a pain to just git checkout the examples from master, and fix the header from math.h to cmath ?). Ie, I think the PR got a little too big in scope, beyond just making sure the src/tests were secure.
In fact the whole thing makes me depressed about C++ ... what's the point of a language where you have to write std:: in front of every other function? Declaring complex vectors is particularly ugly now, as is any stdio stuff. It's not like we are inserting using namespace std; into a header file that affects other people here. It is just there to make clean, short, source codes for testing and examples.
Thoughts?
| c[j] = | ||
| 2 * ((double)rand() / RAND_MAX) - 1 + 1i * (2 * ((double)rand() / RAND_MAX) - 1); | ||
| c[j] = 2 * ((double)std::rand() / RAND_MAX) - 1 + | ||
| std::complex<double>(0.0, 1.0) * |
There was a problem hiding this comment.
Can we use I which was defined above for a reason? I must say this std:: everywhere is highly unpleasant! I'm not sure why in our example codes we're killing using namespace std; ....
| c[j] = | ||
| 2 * ((double)rand() / RAND_MAX) - 1 + 1i * (2 * ((double)rand() / RAND_MAX) - 1); | ||
| c[j] = 2 * ((double)std::rand() / RAND_MAX) - 1 + | ||
| std::complex<double>(0.0, 1.0) * |
| x[j] = PI * (2 * ((double)rand() / RAND_MAX) - 1); // uniform random in [-pi,pi) | ||
| x[j] = PI * (2 * ((double)std::rand() / RAND_MAX) - 1); // uniform random in [-pi,pi) | ||
| // note FINUFFT doesn't use std::vector types, so we need to make a pointer... | ||
| finufft_setpts(plan, M, x.data(), NULL, NULL, 0, NULL, NULL, NULL); |
There was a problem hiding this comment.
nullptr ? Surely it doesn't matter since setpts ignores them.
There was a problem hiding this comment.
I missed it. I'll fix it.
| x[j] = PI * (2 * ((float)std::rand() / RAND_MAX) - 1); // uniform random in [-pi,pi) | ||
| // note FINUFFT doesn't use std::vector types, so we need to make a pointer... | ||
| finufftf_setpts(plan, M, &x[0], NULL, NULL, 0, NULL, NULL, NULL); | ||
| finufftf_setpts(plan, M, &x[0], nullptr, nullptr, 0, nullptr, nullptr, nullptr); |
| int ier = finufft_makeplan(type, dim, Ns, +1, ntrans, tol, &plan, nullptr); | ||
| // step 2: send in M nonuniform points (just x, y in this case)... | ||
| finufft_setpts(plan, M, &x[0], &y[0], NULL, 0, NULL, NULL, NULL); | ||
| finufft_setpts(plan, M, &x[0], &y[0], nullptr, 0, nullptr, nullptr, nullptr); |
| @@ -2,25 +2,20 @@ | |||
| #include <finufft/spreadinterp.h> | |||
| #include <finufft/test_defs.h> | |||
There was a problem hiding this comment.
It seems like adding using namespace std::printf etc to test_defs.h would make this a whole lot less ugly...
There was a problem hiding this comment.
That's a good idea! We can also add I to test_defs.
|
|
||
| #include <finufft_common/common.h> | ||
| #include <cufinufft.h> | ||
| #include <finufft_common/common.h> |
There was a problem hiding this comment.
If this header ordering is important, please add a comment...
| F[k] = std::sin((FLT)0.7 * k) + IMA * std::cos((FLT)0.3 * k); // set F for t2 | ||
| ier = FINUFFT1D2(M, x, c, +1, 0, N, F, &opts); | ||
| if (ier != FINUFFT_WARN_EPS_TOO_SMALL) { | ||
| printf("1d2 tol=0:\twrong err code %d\n", ier); |
There was a problem hiding this comment.
I'm assuming printf is fine here, thank god! Not sure why it wasn't fine earlier...
| free(u); | ||
| if (isnan(errmax) || (errmax > errfail)) { | ||
| if (std::isnan(errmax) || (errmax > errfail)) { | ||
| printf("\tfailed! err %.3g > errfail %.3g\n", errmax, errfail); |
There was a problem hiding this comment.
hoping printf is fine here... please :)
|
In general I do not like using exp in c++ or abs without std:: because I never know if I am calling a compiler builtin with implicit conversion or an actual function. One way to make is safer is to use |
|
Once kerformdef is in master, we should tweak this PR on enforcing std:: and bring in. I think it's easy enough for v2.5 inclusion... thoughts? |
|
I will have a look after kerformdef is merged and propose a plan. |
…native The step branched on 'avx512 in /proc/cpuinfo' and otherwise fell through to -march=native, so the ISA the job built for depended on which runner it drew. That is the same shape as the stale-object bug lu1and10 diagnosed in #742: an arch flag that is not an explicit, stable string. c23a132 removed native from the two sccache-backed workflows; this is the last one left. Walk the x86-64 psABI levels against lscpu's flag list and pass the highest one the runner actually supports, capped at v3 because valgrind's JIT does not cover all of AVX-512 (which is what the old avx512 branch was really guarding against). A v2-only runner now gets -march=x86-64-v2 instead of a native string nobody can reproduce. Verified by extracting the step's run body from the parsed YAML and executing it under bash -e (GitHub's default shell) with cmake stubbed: v3 on this AVX2 host, and v2 / x86-64 on simulated flag sets. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
DiamonDinoia
changed the title
|
I finally had time to look at this and now I fixed (with @claude) all the compiler warnings. I tried to keep the changes minimal. Bundled with a small other issues that emerged while going at this. |
|
@ahbarnett or @lu1and10 if you are happy with the result we can merge, otherwise let me know what to change. |
The tree aligns consecutive `=` and trailing comments by hand, but `AlignConsecutiveAssignments: None` and `AlignTrailingComments: Leave` let clang-format flatten that alignment on every file it touches. Turning both on makes the formatter reproduce the existing style instead of fighting it. `AcrossEmptyLinesAndComments` matches the `AlignConsecutiveMacros` setting already in the file, and keeps blocks such as `finufft_default_opts` in one column across their blank lines. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
Walked every warning in the GitHub and Jenkins logs and fixed it at the source rather than suppressing it. The recurring ones: * unused parameters: examples, tests and perftests declared `int main(int argc, char *argv[])` without reading either. `main` is now spelled `int main()` everywhere it takes no arguments, C and C++ alike; the ones that genuinely parse argv are untouched. `int main()` in C is only diagnosed under -Wpedantic, which the build applies to the library targets in src/ and nothing else. * MSVC C4296 in interp.hpp: an `if constexpr` comparison that is always true for the instantiated widths. Guarded, with a static_assert next to it so the tail-handling stays provably complete. * -Wshadow / -Wsign-compare / -Wunused-variable in the spread and interp headers, foldrescale and binsort_bench. * the mwrap-generated mex gateways are generated code we do not edit, so matlab/CMakeLists.txt drops the warning flags for those targets only. Also folds in the code-review findings on the first pass: round_down/round_up gain runtime overloads so kernel_buffer_stride_runtime and its compile-time mirror share one implementation and cannot drift, plus a static_assert that the alignment fits in T before the mask is formed. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
80a7a49 made the PSWF the only kernel, so spread_kerformula 1-6 now return FINUFFT_ERR_KERFORMULA_NOTVALID. Two things were left behind: * nothing calls cyl_bessel_i any more, so the series implementation in src/common/utils.cpp, its __cpp_lib_math_special_functions feature detection, the two declarations in the header and the std-vs-series comparison in testutils all go. * devel/guru_benchmark.cpp swept formulas 0, 1, 3 and 4, so three of its four arms only measured SkipWithError. It now sweeps the surviving PSWF shape choices 7, 8 and 9; checked against the library that 0/7/8/9 give ier=0 and 1/3/4 give ier=24. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
Two .cpp files pulled the whole of std into the global namespace; fft.cpp then needed a comment explaining that <algorithm> was included because the directive hid std::min. Qualify the handful of uses instead and drop the note. The docs snippet in opts.rst gets the same treatment so the documented example matches the library's own style. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
include-what-you-use over the compile database, acting only on the library (the ~46 remaining suggestions are std-header shuffling in examples, tests and devel, which is churn for no gain): * src/common/utils.cpp did not include its own header, so nothing checked its definitions against finufft_common/utils.h. It also reached PI and std::max transitively. * kernel.cpp used MAX_AUTO_UPSAMPFAC/PI/PSWF0, std::min/max and std::function through other headers; pswf.cpp did the same for FINUFFT_ERR_PSWF_SETUP, std::fill and std::swap. * pswf.h used an unqualified size_t with no <cstddef>. Dependency headers were audited at the same time and are all the public entry points already - <xsimd/xsimd.hpp>, <fftw3.h>, <cufft.h>, <cuComplex.h>, <cuda_runtime.h>, <poet/poet.hpp>, thrust/*.h. Two look internal and are not: xsimd/config/xsimd_config.hpp has to be read before xsimd.hpp to override XSIMD_DEFAULT_ARCH when no architecture is supported, and ducc0's public fft.h only *declares* c2c - the header-only definition is in fftnd_impl.h, so that include stays, now with a comment saying why. IWYU's one opinion about a dependency points the wrong way (it wants xsimd/types/xsimd_batch.hpp in place of the umbrella header) and is rejected. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
The function takes an int debug it never reads (unlike its sibling set_kernel_shape_given_ns, which reports on it), so -Wextra flags it in every Debug build. Two of the three callers already passed a literal 0.
MAX_NF allows a fine-grid dimension up to 1e12, and set_nf_type12 hands one to next235, whose long argument holds 32 bits on Windows. The wrapped value is negative, the std::max(n, 1) inside clamps it to 1, and the call returns 2: a silently tiny grid rather than a crash. good_size_235 already works in size_t, so the narrow type only ever lived in the wrapper. fine_grid_len returns BIGINT for the same reason. testutils now rounds a dimension above 2^31, so the guard runs on the platform that has the bug. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
CI compiled the examples already - cmake_ci, cmake_sanitizers, valgrind and fortran all resolve to a preset inheriting dev, which sets FINUFFT_BUILD_EXAMPLES=ON - but no job ever ran one, so nothing checked what they compute, leak or return. Registering them as ctest tests turned up five separate problems, all fixed here: * three examples (simple2d1, gurumany1d1, spreadinterponly1d) were missing from examples/CMakeLists.txt entirely - the makefile globs, CMake enumerates. * the single-precision examples asked for N=1e5 modes in float. Rounding in the deconvolve step floors the relative error near 0.5*FLT_EPSILON*N, which at N=1e5 is ~6e-3, so makeplan returned FINUFFT_ERR_EPS_TOO_SMALL and the math check was meaningless. N drops to 1e4 (and tol to 1e-3 in guru1d1f); they now return ier=0 at 3.2e-4, 3.2e-4 and 8.7e-5. * four examples never deleted their `new finufft_opts`, a definite leak under ctest -T memcheck. The struct has no reason to be on the heap; many1d1 did not even pass it to the transform. * the CUDA examples discarded every cufinufft return value, so they exited 0 whatever failed. They now bail on ier > 1 (0 is success, 1 a usable warning). * threadsafe1d1 dropped ier on the floor, so as a ctest test it could not fail; it now reports like threadsafe2d2f already did, both setting the flag inside an `omp critical` rather than racing on it. Not a `reduction(max:)`: MSVC defaults to OpenMP 2.0, which has no min/max reductions (error C7660). * the three guru examples branched on `changeopts` to call makeplan with either &opts or NULL. Filling opts first and only touching a field inside the if deletes the else; in the C one it also deletes the malloc and the free() of a pointer that stays uninitialized whenever the demo is off, which is how it ships - undefined behaviour the memcheck job would eventually catch. The GitHub jobs need no workflow change: the presets above already build the examples, and powerpc sets the flag explicitly. Jenkins gets -DFINUFFT_BUILD_EXAMPLES=ON and runs ctest from the build root rather than build/test/cuda - the same tests plus the four CUDA examples, since perftest/cuda registers none. All 16 CPU examples pass in 2s and 132s at -j4 under the valgrind job's exact memcheck flags, with no definite leaks; the four CUDA ones pass on sm_89. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
The fortran CI preset builds with DUCC0 (fortran inherits dev-ducc in CMakePresets.json), and fortran/CMakeLists.txt drops guru1d1 from FORTRAN_EXAMPLES when DUCC0 is on. Neither guru example is therefore built by any CI job, and two defects sat there unnoticed. guru1d1.f and guru1d1f.f include 'fftw3.f' for the FFTW plan-mode constants, but the fortran targets never received FFTW's include directory, so the build stopped with "Cannot open included file 'fftw3.f'". The two guru targets now link finufft_fftlibs, which carries that directory in both FFTW configurations; FFTW_INCLUDE_DIRS is populated only when a system FFTW is found, not when CPM downloads one. The TARGET guard keeps the DUCC0 build configuring, since guru1d1 is absent there. guru1d1f.f then segfaulted. In single precision the rounding floor is 0.48 * eps_mach * gridlen, so tol=1e-5 at N=1e5 is unachievable: setpts returns ier=26, and execute dereferences a plan that has no grid. The example now asks for tol=1e-3 at N=1e4, matching its C++ twin. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
A warning nobody trips over is a warning that comes back: master had already grown a fresh -Wunused-parameter since this branch fixed the last batch. The COMPILE_WARNING_AS_ERROR property goes on the two library targets only, the same ones finufft_apply_compile_settings gives the warning flags to, so no dependency in the build tree is judged by our flags. Errors then demand that every CI compiler agree on what a warning is: - The library copies finufft_opts, and an implicit copy member reads every field, so clang reports the deprecated ones. Its own sources build with FINUFFT_NO_DEPRECATED_FIELDS, which drops the attribute. A pragma around the struct would not do: clang keys the diagnostic to the point where the implicit member is first required, and delayed template parsing, its default on Windows, moves that point out of any such region. The definition stays PRIVATE, so a user's call site still warns. - -fno-semantic-interposition is an ELF flag. clang accepts it on Mach-O and on COFF, then warns that it went unused, which the try_compile filter cannot observe. Add the flag on ELF targets only. - clang 18 reports a lone -fcx-limited-range as overriding the empty option it compares against, which clang 19 fixed, so clang builds disable that diagnostic. RelWithDebInfo also listed the Release flags twice; it now appends the Debug flags alone. - MSVC reports narrowing conversions, shadowing and alignas padding, which GCC and clang leave to -Wconversion, -Wshadow and -Wpadded. This project asks for none of the three, so disable that set and hold every compiler to one level. C4702 joins it for xsimd, whose templates this library instantiates. The MSVC set applies to every configuration, not only the ones that add /W4: /W3 is CMake's default there, so a Release build reports C4244 as well. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
docs/opts.rst promises that showwarn=0 suppresses warnings, and makeplan.hpp keeps that promise: nine error paths print unconditionally and then throw, four showwarn gates guard a message that does not throw, and the two sets never overlap. check_sigma printed unconditionally. tolsweep already defaults showwarn=0 and passes it to opts, yet contributed 849 of the test suite's 864 warning lines, because it sweeps tol past the achievable floor on purpose. The error still always speaks; only the warning now obeys showwarn. The suite drops to 12 warning lines, all in tests where the warning is the assertion. One warning still escapes the gate: the non-OpenMP branch of makeplan.hpp warns that opts.nthreads>1 is ignored, without consulting showwarn. That build is rare and the site is left untouched here. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
`run_tolsweep_double` takes 1353 s under valgrind in CI, against ctest's 1500 s default. Running the examples as tests raises the suite from 26 to 42 entries, and the valgrind workflow calls `ctest -j` with no bound, so the extra contention pushes tolsweep past the default and the job fails on a timeout rather than on a defect. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
CPUA band that brackets 1.00 resolved nothing; read the table, not the point estimate. FFT backends: perftest plot
how the benchmarks are measuredRatio is master/PR-head time: >1 means the PR is faster. Time is makeplan plus setpts plus execute on both halves; the GPU host transfers stage the harness's own test data, so no library change moves them and they are left out. Each case runs 8 rounds with the two binaries interleaved and their order alternating; each arm is tabulated at its median round and the band spans the per-round ratios. A ratio is bold where the band excludes 1.00, which is where the run resolved a change; every other row resolved nothing. Every option a caller may leave alone is left alone (sorting, upsampling factor, kernel choice), so a change to one of finufft's heuristics shows up here as the change in time it causes. The thread count is the exception: a case is defined by the count it runs at. per-case timings
microarchitecture and compilerMicroarchitecture: Compiler: Flags: perftest commandsGPU
cuperftest plot
how the benchmarks are measuredRatio is master/PR-head time: >1 means the PR is faster. Time is makeplan plus setpts plus execute on both halves; the GPU host transfers stage the harness's own test data, so no library change moves them and they are left out. Each case runs 8 rounds with the two binaries interleaved and their order alternating; each arm is tabulated at its median round and the band spans the per-round ratios. A ratio is bold where the band excludes 1.00, which is where the run resolved a change; every other row resolved nothing. Every option a caller may leave alone is left alone (sorting, upsampling factor, kernel choice), so a change to one of finufft's heuristics shows up here as the change in time it causes. The thread count is the exception: a case is defined by the count it runs at. per-case timings
device and toolkitDevice: cuperftest commands |
mreineck
left a comment
mreineck
left a comment
There was a problem hiding this comment.
Looks good to me! Just a few minor comments...
| finufft_setpts(plan, M, &x[0], &y[0], nullptr, 0, nullptr, nullptr, nullptr); | ||
| // step 3: do the adjoint of the planned transform. This maps | ||
| // c strength data, to F output, and is identical to the type 1 with isign=+1. | ||
| finufft_execute_adjoint(plan, &c[0], &F[0]); |
There was a problem hiding this comment.
I agree, and then probably also NULL->nullptr.
| nmodes[2] = 1; | ||
|
|
||
| ier = cufinufftf_makeplan(type, dim, nmodes, iflag, ntransf, tol, &dplan, NULL); | ||
| if (ier > 1) return ier; |
There was a problem hiding this comment.
Here and in analogous places: perhaps change to if (ier > 0), since the special "eps too small" warning case will go away soon (or already is fully gone)?
Bare abs() with <cmath> in scope is not guaranteed to resolve to the floating-point overload; on some toolchains it picks the C int abs(), silently truncating the argument. Switch the two FP call sites in the CUDA spreader to std::abs to match the workaround suggested in flatironinstitute#740.
cufinufft_makeplan returned FINUFFT_WARN_EPS_TOO_SMALL (code 1) when tol was clamped up to eps_mach. That code was the only positive non-error code the library produced. Both clamp sites in setup_spreadinterp already warn on stderr, so the return code carried nothing the caller cannot read there. Drop the plan flag and return 0. The enum entry stays for ABI and is annotated as retired. This discharges the two FIXMEs that asked for the legacy code-1 mapping in safe_call.h and the local alias in c_interface.cpp to go once the GPU stopped returning the code. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
The checks used ier > 1 because code 1 was a warning the caller could tolerate. That code is retired, so every code the library returns is an error. Compare against 0 in tests, perftests and examples. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
6 participants
Supersedes the original contents of this PR (issue #740). The large example
modernization that was here has been split off into a separate branch; what is
left is the minimal set of changes that clears the warnings and stops the
examples from rotting again.
GitHub and Jenkins logs, fixed at the source rather than suppressed: unused
argc/argv, MSVC C4296 ininterp.hpp,-Wshadow/-Wsign-compare/-Wunused-variablein the spread and interp headers.mainis now spelledint main()everywhere it takes no arguments, C and C++ alike. Themwrap-generated mex gateways are generated code, so
matlab/CMakeLists.txtdrops the warning flags for those targets only.
the four
guru_benchmarkarms measured onlySkipWithErrorsince the ES andKaiser-Bessel formulas started returning
FINUFFT_ERR_KERFORMULA_NOTVALID.rotted: three were missing from the CMake build, the single-precision ones
asked for more modes than float can resolve and returned
FINUFFT_ERR_EPS_TOO_SMALL, four leaked theirfinufft_opts, the CUDA onesignored every return code, and
threadsafe1d1droppedierso it could notfail as a test. The three guru examples now fill
optsfirst and only touch afield inside the
if, which deletes theelseand, in the C one, afree()of a pointer that stays uninitialized whenever the demo is off.
the compile database, library sources only.
Verified: 0 warnings and a green ctest on both the
dev(FFTW) anddev-duccpresets with
FINUFFT_BUILD_DEVEL=ON; the 16 CPU example tests pass under thevalgrind job's exact memcheck flags with no definite leaks; the CUDA build and
its 4 example tests pass on sm_89.