[nilason]

Use an explicit 48-bit modular multiplication helper for the LCG jump-ahead calculations. This preserves the intended modulo 2^48 behaviour while avoiding overflowing intermediate uint64_t products reported by Coverity.

Addresses CID 1687667 introduced by bb6a75c

Created with assistance by Codex.

Updated, see #8013 (comment)

[echoix]

Opus 5.5 had nothing to say here

[wenzeslaus]

Model battle: Fable says this is could be // coverity[integer_overflow] . But seriously, this PS is a lot of code and the code in question is a very special kind of code. This well justifies an exception for checkers.

AI generated details

The intermediate product does exceed 2^64 and gets reduced modulo 2^64 by the hardware. That is what Coverity calls an overflow. This reduction cannot change the result, for two independent reasons:

• It is defined behavior. The C standard (C11 6.2.5p9) says unsigned arithmetic can never overflow; a result outside the range is reduced modulo 2^N. Only signed overflow is undefined. Every operand here is uint64_t, so the compiler is not free to assume anything or miscompile.
• The reduction is invisible after the mask. The code only keeps the low 48 bits. Reducing modulo 2^64 discards bits 64 and above, and the mask then discards bits 48 to 63. Since 2^48 divides 2^64, the low 48 bits of the full product and the low 48 bits of the wrapped product are the same bits. Formally, (xy mod 2^64) mod 2^48 == xy mod 2^48 for all x, y.

The magnitude confirms wrap actually happens: the multiplier has 35 bits and the state 48 bits, so products reach 83 bits, and in lcg_jump two 48-bit values multiply to 96 bits. None of that matters to the low 48 bits.

Empirically, the review agent compared the wrapped product against an exact 128-bit product on 20 million random pairs, and I compared it against the 24-bit split on 50 million pairs plus edge cases such as all-ones and the largest power of two. All agreed. This is also the standard implementation of lrand48 in C libraries, and the comment that #7979 added and the PR deletes states exactly this argument.

So the PR fixes a scanner's heuristic, not a defect. The only things that would make the wrap a real problem are a signed operand or a modulus that does not divide 2^64, and neither is present.

[nilason]

Model battle: Fable says this is could be // coverity[integer_overflow] . But seriously, this PS is a lot of code and the code in question is a very special kind of code. This well justifies an exception for checkers.

I expected you would cross-examine this, so I dared to create the PR. In light of this, I had a chat with GPT and it explained the issue so that I basically understand what the code actually does, and confirmed this is a false positive. It did however suggest to lift this critical multiplication to a lightweight function mul48() with added documentation, which I found rather sympathetic. The function's arithmetic will with all likelihood be inlined by compiler anyways. I throw this suggestion in, but I'm completely fine with only adding coverity[integer_overflow] above each of the four lines with issue. (coverity[integer_overflow] will be needed in any case, but I'm not quite sure where Coverity will raise the issue with the mul48() added, hopefully only once in the helper function).

Suggested commit message:

lib/gis: clarify intentional overflow in 48-bit LCG arithmetic

Add `mul48()` to encapsulate multiplication modulo 2^48 and use it in
`lcg_step()` and `lcg_jump()`.

The multiplication may overflow `uint64_t` and wrap modulo 2^64. This is
intentional and does not affect the result modulo 2^48, since 2^48
divides 2^64. Consequently, masking the wrapped product to 48 bits gives
the same result as computing the full product modulo 2^48.

Centralising the operation in `mul48()` makes this property explicit and
avoids repeating potentially misleading unsigned multiplication
throughout the LCG implementation. It also provides a single location
for documenting or suppressing static-analysis warnings about the
intentional wraparound.

[wenzeslaus]

GCC and Clang inline the function for most -O settings, so it does not add much cost overall. Edit: ...except with -O0 which we are using in CI at this point for some reason and I remember people were using it here and there as a workaround for some bad math, but that should be gone.

Still, adding "coverity ignore" lines would also be a valid solution.

Details (AI generated)

Measured with gcc 13.3 and clang 18.1 on x86-64 by compiling lib/gis/lrand48.c from this branch and from main with the same flags, then comparing the disassembly of every function and the raw .text bytes. "Identical" means identical to the code compiled from current main, which the branch starts from.

level	gcc 13	clang 18	with static inline
-O0	real call in lcg_step() and lcg_jump()	same	same
-Og	real call	inlined, .text identical	inlined with both
-O1	inlined, same code, functions reordered	inlined, identical	inlined, identical
-O2, -O3, -g -O2	inlined, .text identical	inlined, .text identical	inlined, identical
-Os	inlined, identical	inlined, one imul operand commuted in lcg_jump(), one mov fewer	unchanged

With the project's -g -O2, the .text section is byte-identical to main with both compilers; mul48 survives only as a name in the debug information. Where it is a call, each multiplication becomes two argument moves and a call, about 80 bytes of text in lcg_step(), the function behind every draw, and lcg_jump(); the public G_random_*() functions are unchanged at every level. static inline changes one cell, gcc at -Og, and leaves -O0 as the only level with a call.

-O0 is not only a debugging case. Most CI jobs that build GRASS compile without optimization, because setting CFLAGS replaces the -g -O2 default of configure, and CMake without a build type adds no -O flag:

build	optimization	why
ubuntu.yml (Autotools)	none	sets CFLAGS without -O
gcc.yml	none	sets CFLAGS without -O
cmake.yml	none	no CMAKE_BUILD_TYPE; the default is set only for MSVC
osgeo4w.yml	likely none	build_osgeo4w.sh sets CFLAGS="$CFLAGS -pipe"; inherited CFLAGS not checked
pytest.yml	-O2	CFLAGS unset, so the configure default applies
macos_install.sh	-O2	explicit
Docker images	-O2, Release for the Debian CMake stage	explicit

Instruction counts at -O0 and -O2, from a standalone copy of lcg_step() and lcg_jump() compiled as non-static functions, main -> this branch. Where mul48() is a real call, its body is counted separately:

function	gcc -O2	clang -O2	gcc -O0	clang -O0
lcg_step()	8 -> 8, identical	6 -> 6	12 -> 15 + 11 in mul48()	10 -> 13 + 10 in mul48()
lcg_jump()	28 -> 28, identical apart from labels	32 -> 31	51 -> 62	48 -> 51

The clang -O2 difference in lcg_jump() comes from register allocation in the standalone copy, with one imul operand commuted and one mov fewer; compiled as part of lrand48.c, the .text is identical with both compilers, as above.

Part of the -O0 cost is a redundant mask. The branch masks the product inside mul48() and then the sum, (mul48(a, x) + c) & MASK48, where main masks once, (a * x + c) & MASK48. The inner mask does not change the result, because the low 48 bits of a sum depend only on the low 48 bits of its terms. A multiply-add helper, muladd48(a, x, c) returning (a * x + c) & MASK48, keeps a single mask. Instruction counts for lcg_step() with each variant:

	main	this branch (mul48())	muladd48()
gcc -O2	8	8	8
clang -O2	6	6	6
gcc -O0	12	15 + 11 in mul48()	13 + 15 in muladd48()
clang -O0	10	13 + 10 in mul48()	11 + 12 in muladd48()

At -O0 the call costs more than the extra AND, and passing three arguments makes muladd48() no cheaper than mul48(). Only a macro, or the expression as written on main, gives main's code at every level.

After this change Coverity will report the multiplication once, inside mul48(), so one annotation there covers it. The description still describes the earlier __int128 version, which avoided the wrap; this version wraps on purpose, as the suggested commit message says.

[nilason]

Still, adding "coverity ignore" lines would also be a valid solution.

Added, hopefully this will remove all raised coverity issues.

[wenzeslaus]

I looked at the runtime impact more closely. -O2 has the same instructions, so there is no difference, but with -O0, there is a measurable difference in both a synthetic test and with an existing tool. However, in other cases, we also prefer cleaner code and rely on optimization, so that will be the case here with this change too.

Details (AI generated)

Measured with gcc 13.3 and clang 18.1 on x86-64, pinned to one core, with lib/gis/lrand48.c from main and from this branch.

Synthetic test: a loop of 10^8 G_random_double() calls, compiled together with lrand48.c at the same level. Median of 7 interleaved runs, in ns per draw:

	main	this branch	difference
gcc -O0	4.31	6.01	+1.70 ns (+39 %)
clang -O0	4.17	5.55	+1.38 ns (+33 %)
gcc -O2	2.12	2.12	none
clang -O2	2.14	2.12	none (noise)

Existing tool: r.sim.water from #7980, which draws through G_random_double() in its walker loop. All of GRASS was built with the ubuntu.yml CI flags (-fPIC -Wvla -ffp-contract=off, no -O), and libgrass_gis from main and from this branch were swapped between runs. 700x750 cells, duration=2, nprocs=1, random_seed=1, 5 interleaved rounds:

	runs (s)	median
main	41.00, 40.94, 40.72, 40.78, 40.98	40.94
this branch	42.94, 42.98, 42.86, 42.97, 42.91	42.94

That is +2.0 s (+4.9 %), with every run of this branch slower than every run of main. The output depth is identical. r.sim.water was not timed at -O2, where the compiled .text of lrand48.c is byte-identical to main.

[nilason]

wenzeslaus left a comment (OSGeo/grass#8013) <#8013 (comment)> I looked at the runtime impact more closely. -O2 has the same instructions, so there is no difference, but with -O0, there is a measurable difference in both a synthetic test and with an existing tool. However, in other cases, we also prefer cleaner code and rely on optimization, so that will be the case here with this change too.

For anyone compiling with -O0, speed isn't the main priority , I'd say. So that shouldn't be a concern here either.

[wenzeslaus]

I agree that the slower -O0 case is not an issue and that in other cases, we would go with more readable code (and rely on optimization to solve it for us). We may need to follow up with using -O2 in CI.