Repository navigation
r.mapcalc: Fix modulo by zero hang and speed up threads - #8031
Merged
Merged
Conversation
Integer modulus by zero, e.g., 5 % 0, hung r.mapcalc on x86 Linux because the SIGFPE handler returned and the division was executed again. The integer branch of mod now checks for a zero divisor and gives NULL as documented, so the result does not depend on the platform. The SIGFPE handler and the floating_point_exception flags are removed. Floating point exceptions are masked by default, so the flags were never set, but all threads wrote the shared flag for every cell, which made the computation scale poorly with threads. Domain errors still give NaN, which is NULL, and overflow still gives infinity. The fix, tests, and benchmarks were prepared with AI assistance (Claude Code with Opus 5.5).
The C library returns NaN for sin, cos, and tan of infinity, and NaN is NULL in GRASS. The comments and tests were updated with AI assistance (Claude Code with Opus 5.5).
nilason
reviewed
Oct 7, 2026
nilason
approved these changes
Oct 7, 2026
wenzeslaus
enabled auto-merge (squash)
October 7, 2026 14:00
Member
Author
|
I added explanation and a more detailed benchmark showing the speedup. I'm resolving the conversation and merging. A Claude Fable agent discovered that for the log function, the code, comments and user documentation do not match. It is unrelated to the goal here, so I'm leaving this for a separate PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Integer modulo by zero (
r.mapcalc "x = 5 % 0") hangs r.mapcalc. This adds a zero-divisor check and removes the SIGFPE handler and its per-cell flag. With floating point traps off by default, the handler only ever ran for this integer case, where it caused the hang, while resetting the shared flag for every cell slowed down computation with multiple threads.Fun fact: I used grass-legacy repo to investigate this, so there is some code history junkie stuff to ponder in the details block.
Edit: To clarify the history (thanks @nilason): the signal handler is older than the missing check. It dates back to 1990 and was meant to catch floating point errors as they happen during the computation and turn the affected cell into NULL, instead of relying on each function to check its inputs. Integer
%had its own zero check until the 2002 rewrite dropped it. Today, floating point errors don't raise the signal at all: they produce infinity or NaN, the functions check their inputs where they want NULL, and NaN is NULL in GRASS. So the handler wasn't needed for floating point, and this integer case was the only thing it caught.Details: cause, behavior, benchmarks, and tests (AI generated)
Cause: The integer
%did not check for a zero divisor. On x86, integer division by zero traps, the SIGFPE handler returns, and the CPU executes the same division again, so r.mapcalc spins forever. Integer/already had the check. Integer%checked for a zero divisor until the 2002 rewrite (de8f7a4fd9in grass-legacy) dropped the check.Flags: Floating point exceptions are masked by default, so the handler, which dates from 1990 (in its
sigaction()form from 2002), never runs for floating point operations, and on Windows it is not installed at all. Since the OpenMP support, all threads reset the one shared volatile flag to 0 before every cell and read it after.Division by zero, domain errors, and overflow without the flag: Floating point exceptions are masked by default (IEEE 754), so an invalid operation, division by zero, or overflow does not raise SIGFPE; it returns NaN or infinity and the computation continues. What a cell gets is the same before and after this PR, except integer
%:/and now in integer%,sqrtof a negative,logof a non-positive value or base, andpow/expof a negative base with a non-integer exponent.acos/asinoutside [-1, 1], floating point%by zero (fmod),sin/cos/tanof infinity, and 0/0 such aslog(1, 1). GRASS treats any NaN as NULL (Rast_is_d_null_value()isx != x): writing a row records it in the null file, and reading gives NULL back.exp(1000)orpow(10.0, 400), andlog(x, 1)for x other than 1, giveinfor-inf. GRASS writes and reads infinity like any other value, which the newtest_overflow_is_infinitychecks on all CI platforms.The
log(x, 1)case does not match the documentation, before or after this PR: the manual says that invalid arguments should give NULL, and thelog()source comment says that a base of 1.0 gives NULL, but the code givesinfor-inf(NULL only forlog(1, 1)). This PR keeps that behavior and the comment's claim, and has no test for it; it is left for a separate fix.API: This removes
pre_exec(),post_exec(),floating_point_exception, andfloating_point_exception_occurredfrom the C API.grass/calc.handgrass/defs/calc.hare installed with both build systems and libgrass_calc exported the symbols, so this is a change in the public API, although nothing marks the library as internal or external. In this repository, only r.mapcalc and lib/calc includegrass/calc.h, grass-addons has no users (checked in a clone from July 2026), and there are no Python bindings for the library.History: From grass-legacy and this repository's history:
src/Gmapcalc/mapcalc/fpe.c): the SIGFPE handler sets the two flags and re-installs itself withsignal().src/raster/r.mapcalc/mapcalc/math.c): floating point operations clear the flag, compute, and set NULL if the flag was set. Integer modulus checks*b == 0before%=, so it never relied on the flag. A floating point error printed a note and gave exit code 1, with the map still written (exit(allok), "fixed 11/99").de8f7a4fd9):volatileflags,sigaction()inpre_exec(), and the same per-cell pattern in thex*.cfunctions; integer%lost the zero check.87274dfb09): the functions move tolib/calc.d5c51924ea): threads compute rows in parallel and share the flag.The per-cell pattern can only work if the error is reported after the operation completes, so that execution reaches the flag check after the handler returns. An x86 integer division by zero is a fault that re-runs the instruction instead, so no check placed after the division is ever reached.
Uncertainties: I did not verify which platforms GRASS ran on between 1990 and 2002, or whether they trapped floating point errors by default. My understanding is that some architectures reported arithmetic errors as traps after the instruction (e.g., VAX), and that x87 reports unmasked exceptions at the next floating point instruction; whether that made the flags work in practice for GRASS is not known. Exit codes before November 1999 were not checked.
Behavior: Old is current main, likely intended is what the SIGFPE handler and flag checks were written for.
%by zero (5 % 0)/checks for zero)%by zero (5.0 % 0.0)acos(2),sqrt(-1),log(0),pow(-8.0, 0.5))exp(1000),pow(10.0, 400))%trap hangs)Platforms: Only the hang on x86-64 Linux was reproduced. The other platforms are expected from the code and the CPU behavior, not tested: x86 macOS installs the same handler, so it should hang too; Windows has no handler (
#ifndef _WIN32), so the trap should crash r.mapcalc; on ARM64 (Apple Silicon, Linux aarch64), integer division by zero does not trap and gives 0, so5 % 0should silently give 5. In C, it is undefined behavior anyway. With the zero check, the result is NULL everywhere, which the new tests check on all CI platforms.Benchmark:
f_divon 3000x20000 cells called directly from OpenMP threads, median of 7 runs, i7-10510U,-O2:sqrtbehaves the same.Whole r.mapcalc, 6000x6000 cells,
x = sqrt(a / b) + log(a / b) + a / b / b + ...with 25 such terms, one run each on the same machine:Before, the time stops improving at 3 threads and stays at 35-39 s; after, it keeps improving to 26 s at 7 threads, and is at least 16% faster from 4 threads up. The machine has 4 physical cores, so 5-8 threads use hyper-threading.
Benchmark at -O0 and -O2: Whole r.mapcalc on a Xeon W-2295 @ 3.00GHz (18 cores, 36 threads, shared with other users), 3000x3000 cells,
x = sqrt(a / b) + log(a / b) + a / b / b + sin(a) * cos(b) + exp(a, 0.5) + sqrt(b / a) + log(b / a)withaandbrandom double maps, i.e., 13 operations per cell which reset the flag before this PR. Wall time in seconds, median (min-max) of 5 runs, the builds interleaved within each run (figure above):At 1 thread the builds are equal within the min-max ranges at both optimization levels. At -O2, main stops improving at 8 threads and is no faster at 16, while this PR reaches 0.36 s at 8 threads and stays there at 16; this PR is 16% faster at 4 threads, 35% at 8, and 33% at 16. At -O0, the gap appears later, 13% at 8 threads and 32% at 16, because the unoptimized arithmetic hides the contention on the shared flag until more threads write it. The result maps are identical cell by cell across builds and thread counts. Beyond 18 threads the machine shares cores, so 16 threads is the maximum used.
Alternatives measured: Making the flag thread-local scaled with threads, but was 1.6x slower on one thread than the current code. Detecting errors with
<fenv.h>per cell was 3-40x slower and would turn overflow into NULL with a non-zero exit.Tests: The new pytest file checks that division and modulo by zero give NULL for all three types, and so do domain errors of each function whose flag check was removed and infinite input to sin, cos, and tan. It also checks correct values of those functions, overflow to infinity, and a zero divisor in only some cells, all with 1 and 4 threads. With the old library, exactly the four
%cases fail by timing out.The hang was reproduced, the alternatives were benchmarked, and the fix and tests were prepared with AI assistance (Claude Code with Opus 5.5). The -O0 and -O2 benchmark was run with Claude Code with Fable 5.1.
Suggested squash commit message