Skip to content

Commit 582b724

Browse files
TimWollaTimWolla
authored
random: Fix γ-section implementation for Randomizer::getFloat() (#12402)
The reference implementation of the "Drawing Random Floating-Point Numbers from an Interval" paper contains two mistakes that will result in erroneous values being returned under certain circumstances: - For large values of `g` the multiplication of `k * g` might overflow to infinity. - The value of `ceilint()` might exceed 2^53, possibly leading to a rounding error when promoting `k` to double within the multiplication of `k * g`. This commit updates the implementation based on Prof. Goualard suggestions after reaching out to him. It will correctly handle inputs larger than 2^-1020 in absolute values. This limitation will be documented and those inputs possibly be rejected in a follow-up commit depending on performance concerns.
1 parent e7fa42e commit 582b724

File tree

4 files changed

+154
-8
lines changed

4 files changed

+154
-8
lines changed

‎NEWS‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,10 @@ PHP NEWS
1313
. Fixed bug GH-8143 (Crashes in zend_accel_inheritance_cache_find since
1414
upgrading to 8.1.3 due to corrupt on-disk file cache). (turchanov)
1515

16+
- Random:
17+
. Fix Randomizer::getFloat() returning incorrect results under
18+
certain circumstances. (timwolla)
19+
1620
- SOAP:
1721
. Fixed bug GH-12392 (Segmentation fault on SoapClient::__getTypes).
1822
(nielsdos)

‎ext/random/gammasection.c‎

Lines changed: 57 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@
1313
| Authors: Tim Düsterhus <timwolla@php.net> |
1414
| |
1515
| Based on code from: Frédéric Goualard |
16+
| Corrected to handled appropriately random integers larger than 2^53 |
17+
| converted to double precision floats, and to avoid overflows for |
18+
| large gammas. |
1619
+----------------------------------------------------------------------+
1720
*/
1821

@@ -46,6 +49,12 @@ static double gamma_max(double x, double y)
4649
return (fabs(x) > fabs(y)) ? gamma_high(x) : gamma_low(y);
4750
}
4851

52+
static void splitint64(uint64_t v, double *vhi, double *vlo)
53+
{
54+
*vhi = v >> 2;
55+
*vlo = v & UINT64_C(0x3);
56+
}
57+
4958
static uint64_t ceilint(double a, double b, double g)
5059
{
5160
double s = b / g - a / g;
@@ -74,9 +83,19 @@ PHPAPI double php_random_gammasection_closed_open(const php_random_algo *algo, p
7483
uint64_t k = 1 + php_random_range64(algo, status, hi - 1); /* [1, hi] */
7584

7685
if (fabs(min) <= fabs(max)) {
77-
return k == hi ? min : max - k * g;
86+
if (k == hi) {
87+
return min;
88+
} else {
89+
double k_hi, k_lo;
90+
splitint64(k, &k_hi, &k_lo);
91+
92+
return 0x1p+2 * (max * 0x1p-2 - k_hi * g) - k_lo * g;
93+
}
7894
} else {
79-
return min + (k - 1) * g;
95+
double k_hi, k_lo;
96+
splitint64(k - 1, &k_hi, &k_lo);
97+
98+
return 0x1p+2 * (min * 0x1p-2 + k_hi * g) + k_lo * g;
8099
}
81100
}
82101

@@ -92,9 +111,23 @@ PHPAPI double php_random_gammasection_closed_closed(const php_random_algo *algo,
92111
uint64_t k = php_random_range64(algo, status, hi); /* [0, hi] */
93112

94113
if (fabs(min) <= fabs(max)) {
95-
return k == hi ? min : max - k * g;
114+
if (k == hi) {
115+
return min;
116+
} else {
117+
double k_hi, k_lo;
118+
splitint64(k, &k_hi, &k_lo);
119+
120+
return 0x1p+2 * (max * 0x1p-2 - k_hi * g) - k_lo * g;
121+
}
96122
} else {
97-
return k == hi ? max : min + k * g;
123+
if (k == hi) {
124+
return max;
125+
} else {
126+
double k_hi, k_lo;
127+
splitint64(k, &k_hi, &k_lo);
128+
129+
return 0x1p+2 * (min * 0x1p-2 + k_hi * g) + k_lo * g;
130+
}
98131
}
99132
}
100133

@@ -110,9 +143,19 @@ PHPAPI double php_random_gammasection_open_closed(const php_random_algo *algo, p
110143
uint64_t k = php_random_range64(algo, status, hi - 1); /* [0, hi - 1] */
111144

112145
if (fabs(min) <= fabs(max)) {
113-
return max - k * g;
146+
double k_hi, k_lo;
147+
splitint64(k, &k_hi, &k_lo);
148+
149+
return 0x1p+2 * (max * 0x1p-2 - k_hi * g) - k_lo * g;
114150
} else {
115-
return k == (hi - 1) ? max : min + (k + 1) * g;
151+
if (k == (hi - 1)) {
152+
return max;
153+
} else {
154+
double k_hi, k_lo;
155+
splitint64(k + 1, &k_hi, &k_lo);
156+
157+
return 0x1p+2 * (min * 0x1p-2 + k_hi * g) + k_lo * g;
158+
}
116159
}
117160
}
118161

@@ -128,8 +171,14 @@ PHPAPI double php_random_gammasection_open_open(const php_random_algo *algo, php
128171
uint64_t k = 1 + php_random_range64(algo, status, hi - 2); /* [1, hi - 1] */
129172

130173
if (fabs(min) <= fabs(max)) {
131-
return max - k * g;
174+
double k_hi, k_lo;
175+
splitint64(k, &k_hi, &k_lo);
176+
177+
return 0x1p+2 * (max * 0x1p-2 - k_hi * g) - k_lo * g;
132178
} else {
133-
return min + k * g;
179+
double k_hi, k_lo;
180+
splitint64(k, &k_hi, &k_lo);
181+
182+
return 0x1p+2 * (min * 0x1p-2 + k_hi * g) + k_lo * g;
134183
}
135184
}

‎ext/random/tests/03_randomizer/methods/getFloat_extreme_range.phpt‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
--TEST--
2+
Random: Randomizer: getFloat(): Extreme ranges are handled correctly
3+
--FILE--
4+
<?php
5+
6+
use Random\Engine;
7+
use Random\Randomizer;
8+
9+
final class TestEngine implements Engine
10+
{
11+
private array $values = [
12+
"\x64\xe8\x58\x79\x3e\xf6\x38\x00",
13+
"\x65\xe8\x58\x79\x3e\xf6\x38\x00",
14+
"\x00\x00\x00\x00\x00\x00\x00\x00",
15+
];
16+
17+
public function generate(): string
18+
{
19+
return \array_shift($this->values);
20+
}
21+
}
22+
23+
$r = new Randomizer(new TestEngine());
24+
25+
$min = -1.6e308;
26+
$max = 1.6e308;
27+
printf("%.17g\n", $min);
28+
printf("%.17g\n\n", $max);
29+
30+
printf("%.17g\n", $r->getFloat($min, $max));
31+
printf("%.17g\n", $r->getFloat($min, $max));
32+
printf("%.17g\n", $r->getFloat($min, $max));
33+
34+
?>
35+
--EXPECT--
36+
-1.6e+308
37+
1.6e+308
38+
39+
-1.5999999999999998e+308
40+
-1.6e+308
41+
1.5999999999999998e+308

‎ext/random/tests/03_randomizer/methods/getFloat_opposite_signs.phpt‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
--TEST--
2+
Random: Randomizer: getFloat(): Opposite signs are handled correctly
3+
--FILE--
4+
<?php
5+
6+
use Random\Engine;
7+
use Random\Randomizer;
8+
9+
final class TestEngine implements Engine
10+
{
11+
private array $values = [
12+
"\x63\xe8\x58\x79\x3e\xf6\x38\x00",
13+
"\x64\xe8\x58\x79\x3e\xf6\x38\x00",
14+
"\x65\xe8\x58\x79\x3e\xf6\x38\x00",
15+
"\x66\xe8\x58\x79\x3e\xf6\x38\x00",
16+
"\x67\xe8\x58\x79\x3e\xf6\x38\x00",
17+
];
18+
19+
public function generate(): string
20+
{
21+
return \array_shift($this->values);
22+
}
23+
}
24+
25+
$r = new Randomizer(new TestEngine());
26+
27+
$min = -1;
28+
$max = 1;
29+
printf("%.17g\n", $min);
30+
printf("%.17g\n\n", $max);
31+
32+
$prev = null;
33+
for ($i = 0; $i < 5; $i++) {
34+
$float = $r->getFloat($min, $max);
35+
printf("%.17f", $float);
36+
if ($prev !== null) {
37+
printf(" (%+.17g)", ($float - $prev));
38+
}
39+
printf("\n");
40+
41+
$prev = $float;
42+
}
43+
?>
44+
--EXPECT--
45+
-1
46+
1
47+
48+
-0.78005908680576086
49+
-0.78005908680576097 (-1.1102230246251565e-16)
50+
-0.78005908680576108 (-1.1102230246251565e-16)
51+
-0.78005908680576119 (-1.1102230246251565e-16)
52+
-0.78005908680576130 (-1.1102230246251565e-16)

0 commit comments

Comments
 (0)