gbranden pushed a commit to branch master
in repository groff.
commit 17f18cc8cbedb6fcb83b531f253a288a19ec36e0
Author: G. Branden Robinson <[email protected]>
AuthorDate: Fri Aug 16 14:53:13 2024 -0500
[troff]: Fix Savannah #66001 (saturating arith).
* src/roff/troff/number.cpp (is_valid_term): When reading an overlong
integer literal, save the value corresponding to the longest valid
digit sequence, and use that if overflow occurs.
(scale): Demote "numeric overflow" errors to "integer value saturated"
warnings in category "range".
* src/roff/troff/hvunits.h (vunits::to_units)
(class vunits: operator +, operator -, operator *):
(hunits::to_units)
(class hunits: operator +, operator -, operator *):
* src/roff/troff/number.cpp (vunits::vunits, hunits::hunits): Impose
saturating arithmetic on overflowing operations. Demote overflow from
error, throwing warning in "range" category if overflow would occur,
and describe integer result as "saturated" rather than "wrapped".
* src/roff/groff/tests/arithmetic-works.sh: Enable warnings in "range"
category. Update test expectations. Drop check of operation that is
no longer meaningful with refactored arithmetic parsing.
* doc/groff.texi.in (Numeric Expressions):
* man/groff.7.man (Numeric expressions):
* NEWS: Document it.
Fixes <https://savannah.gnu.org/bugs/?66001>.
---
ChangeLog | 29 ++++++++++++++
NEWS | 4 ++
doc/groff.texi.in | 12 ++++--
man/groff.7.man | 11 ++++--
src/roff/groff/tests/arithmetic-works.sh | 50 +++++++++++++----------
src/roff/troff/hvunits.h | 26 ++++++------
src/roff/troff/number.cpp | 68 ++++++++++++++++++++------------
7 files changed, 135 insertions(+), 65 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index dfd57971a..9959ac25a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,32 @@
+2024-08-16 G. Branden Robinson <[email protected]>
+
+ * src/roff/troff/number.cpp (is_valid_term): When reading an
+ overlong integer literal, save the value corresponding to the
+ longest valid digit sequence, and use that if overflow occurs.
+ (scale): Demote "numeric overflow" errors to "integer value
+ saturated" warnings in category "range".
+
+ * src/roff/troff/hvunits.h (vunits::to_units)
+ (class vunits: operator +, operator -, operator *):
+ (hunits::to_units)
+ (class hunits: operator +, operator -, operator *):
+ * src/roff/troff/number.cpp (vunits::vunits, hunits::hunits):
+ Impose saturating arithmetic on overflowing operations. Demote
+ overflow from error, throwing warning in "range" category if
+ overflow would occur, and describe integer result as "saturated"
+ rather than "wrapped".
+
+ * src/roff/groff/tests/arithmetic-works.sh: Enable warnings in
+ "range" category. Update test expectations. Drop check of
+ operation that is no longer meaningful with refactored
+ arithmetic parsing.
+
+ * doc/groff.texi.in (Numeric Expressions):
+ * man/groff.7.man (Numeric expressions):
+ * NEWS: Document it.
+
+ Fixes <https://savannah.gnu.org/bugs/?66001>.
+
2024-08-16 G. Branden Robinson <[email protected]>
* src/roff/troff/number.cpp (get_vunits, get_hunts, get_number)
diff --git a/NEWS b/NEWS
index ff3f839bc..1344eb4e6 100644
--- a/NEWS
+++ b/NEWS
@@ -39,6 +39,10 @@ o The `color`, `cp`, `linetabs`, and `vpt` requests now
interpret
with a register interpolation as an argument, the outcome agrees with
an `if` test of the register's value.
+o GNU troff now implements saturating rather than wrapping integer
+ arithmetic. Where before overflow would cause an error diagnostic,
+ the formatter now emits a warning diagnostic in the "range" category.
+
o The formatter no longer reports warnings in the "el" category. There
was only one; it threw a warning diagnostic if enabled (which it was
not by default) if it inferred an imbalance between `ie` and `el`
diff --git a/doc/groff.texi.in b/doc/groff.texi.in
index f51e66a35..208afe15f 100644
--- a/doc/groff.texi.in
+++ b/doc/groff.texi.in
@@ -6190,6 +6190,9 @@ used for computing results depends on the host machine
but is at least a
range of �2 billion.@footnote{If that's not enough, see the
@cite{groff_tmac@r{(5)}} man page for the @file{62bit.tmac} macro
package.}
+@cindex saturating arithmetic
+Arithmetic saturates rather than wrapping; if overflow would occur, GNU
+@command{troff} emits a warning in category @samp{range}.
@cindex arithmetic operators
@cindex operators, arithmetic
@@ -6209,8 +6212,12 @@ to their left and right; they are @code{+} (addition),
@code{-}
(subtraction), @code{*} (multiplication), @code{/} (truncating
division), and @code{%} (modulus). @dfn{Truncating division} rounds to
the integer nearer to zero, no matter how large the fractional portion.
-Overflow and division (or modulus) by zero are errors and abort
-evaluation of a numeric expression.
+@cindex division by zero
+@cindex modulus by zero
+@cindex zero, division and modulus by
+Division and modulus by zero are errors and abort evaluation of a
+numeric expression.
+
@cindex unary arithmetic operators
@cindex operators, unary arithmetic
@cindex negation
@@ -6219,7 +6226,6 @@ evaluation of a numeric expression.
@opindex +
@cindex @code{if} request, and the @samp{!} operator
@cindex @code{while} request, and the @samp{!} operator
-
Arithmetic unary operators operate on the numeric expression to their
right; they are @code{-} (negation) and @code{+} (assertion---for
completeness; it does nothing). The unary minus must often be used
diff --git a/man/groff.7.man b/man/groff.7.man
index b3c1ef163..1d3b759dd 100644
--- a/man/groff.7.man
+++ b/man/groff.7.man
@@ -976,6 +976,12 @@ for the
.I 62bit.tmac
macro package.)
.
+Arithmetic saturates rather than wrapping;
+if overflow would occur,
+.I @g@troff
+emits a warning in category
+.RB \%\[lq] range \[rq].
+.
.
.P
Arithmetic infix operators perform a function on the numeric expressions
@@ -997,9 +1003,8 @@ and
rounds to the integer nearer to zero,
no matter how large the fractional portion.
.
-Overflow and division
-(or modulus)
-by zero are errors and abort evaluation of a numeric expression.
+Division and modulus by zero
+are errors and abort evaluation of a numeric expression.
.
.
.P
diff --git a/src/roff/groff/tests/arithmetic-works.sh
b/src/roff/groff/tests/arithmetic-works.sh
index f7bb699ad..6cbcb81a1 100755
--- a/src/roff/groff/tests/arithmetic-works.sh
+++ b/src/roff/groff/tests/arithmetic-works.sh
@@ -57,13 +57,13 @@ input='.
#
# 2: .l=1512
#
-# 3: .l=24, .H=24
+# 3: .l=2147483640, .H=24
#
-# 4: .p=40, .V=40
+# 4: .p=2147483640, .V=40
#
# The blank lines are due to the `vs` increase.
-output=$(echo "$input" | "$groff" -T ascii)
+output=$(echo "$input" | "$groff" -w range -T ascii)
echo "$output"
echo "checking that vertical spacing is correctly incremented" >&2
@@ -72,11 +72,11 @@ echo "$output" | grep -Fqx '1: .v=80' || wail
echo "checking that line length is correctly incremented" >&2
echo "$output" | grep -Fqx '2: .l=1512' || wail
-echo "checking that setting huge line length does not overflow" >&2
-echo "$output" | grep -Fqx '3: .l=24, .H=24' || wail
+echo "checking that setting huge line length saturates" >&2
+echo "$output" | grep -Fqx '3: .l=2147483640, .H=24' || wail
-echo "checking that setting huge page length does not overflow" >&2
-echo "$output" | grep -Fqx '4: .p=40, .V=40' || wail
+echo "checking that setting huge page length saturates" >&2
+echo "$output" | grep -Fqx '4: .p=2147483640, .V=40' || wail
input='.
.ec @
@@ -91,7 +91,7 @@ c: @nc
d: @nd
.'
-output=$(echo "$input" | "$groff" -T ascii)
+output=$(echo "$input" | "$groff" -w range -T ascii)
echo "$output"
# The vunits and hunits constructors in src/roff/troff/number.cpp don't
@@ -132,28 +132,22 @@ input='.
5: (-1)*2147483647 -> @ni
.nr i -1
6: decr -2147483648 -> @ni
-.nr i -2147483648
-.nr i (-1)*@ni
-7: (-1)*(-2147483648) -> @ni
.'
-output=$(echo "$input" | "$groff" -T ascii)
+output=$(echo "$input" | "$groff" -w range -T ascii)
echo "$output"
-# 2147483647
echo "checking assignment of huge value to register" >&2
echo "$output" \
- | grep -Fqx '1: 99999999999999999999 -> 1410065407' || wail
+ | grep -Fqx '1: 99999999999999999999 -> 999999999' || wail
-# -2147483648
echo "checking assignment of huge negative value to register" >&2
echo "$output" \
- | grep -Fqx '2: (-99999999999999999999) -> -1410065407' || wail
+ | grep -Fqx '2: (-99999999999999999999) -> -999999999' || wail
echo "checking assignment of 2^31 - 1 to register" >&2
echo "$output" | grep -Fqx '3: assign 2147483647 -> 2147483647' || wail
-# -2147483648
echo "checking incrementation of register" >&2
echo "$output" | grep -Fqx '4: incr 2147483647 -> -2147483648' || wail
@@ -163,10 +157,24 @@ echo "$output" | grep -Fqx '5: (-1)*2147483647 ->
-2147483647' || wail
echo "checking decrementation of register" >&2
echo "$output" | grep -Fqx '6: decr -2147483648 -> -2147483648' || wail
-# 2147483647
-echo "checking negation of negatively register" >&2
-echo "$output" | grep -Fqx '7: (-1)*(-2147483648) -> 0' \
- || wail
+# A quirk that GNU troff arithmetic has is that one cannot directly
+# assign INT_MIN to a register, because the negative sign is parsed
+# separately from the digit sequence. (This is true even when the
+# negative sign is not intrepreted as a decrementation operator.)
+#
+# .nr a 2147483647
+# .tm a=\na
+# a=2147483647
+# .nr a \na*-1
+# .tm a=\na
+# a=-2147483647
+# .nr a -1
+# .tm a=\na
+# a=-2147483648
+# .nr a \na*-1
+# troff:<standard input>:7: warning: integer value saturated
+#
+# This seems okay to GBR.
test -z "$fail"
diff --git a/src/roff/troff/hvunits.h b/src/roff/troff/hvunits.h
index a6f6fb456..63de9c405 100644
--- a/src/roff/troff/hvunits.h
+++ b/src/roff/troff/hvunits.h
@@ -1,4 +1,4 @@
-/* Copyright (C) 1989-2020 Free Software Foundation, Inc.
+/* Copyright (C) 1989-2024 Free Software Foundation, Inc.
Written by James Clark ([email protected])
This file is part of groff.
@@ -93,7 +93,7 @@ inline units vunits::to_units()
{
units r;
if (ckd_mul(&r, n, vresolution))
- error("integer multiplication wrapped");
+ warning(WARN_RANGE, "integer multiplication saturated");
return r;
}
@@ -107,7 +107,7 @@ inline vunits operator +(const vunits & x, const vunits & y)
vunits r;
r = x;
if (ckd_add(&r.n, r.n, y.n))
- error("integer addition wrapped");
+ warning(WARN_RANGE, "integer addition saturated");
return r;
}
@@ -116,7 +116,7 @@ inline vunits operator -(const vunits & x, const vunits & y)
vunits r;
r = x;
if (ckd_sub(&r.n, r.n, y.n))
- error("integer subtraction wrapped");
+ warning(WARN_RANGE, "integer subtraction saturated");
return r;
}
@@ -125,7 +125,7 @@ inline vunits operator -(const vunits & x)
vunits r;
// Why? Consider -(INT_MIN) in two's complement.
if (ckd_mul(&r.n, x.n, -1))
- error("integer multiplication wrapped");
+ warning(WARN_RANGE, "integer multiplication saturated");
return r;
}
@@ -147,7 +147,7 @@ inline vunits operator *(const vunits & x, int n)
vunits r;
r = x;
if (ckd_mul(&r.n, x.n, n))
- error("integer multiplication wrapped");
+ warning(WARN_RANGE, "integer multiplication saturated");
return r;
}
@@ -156,7 +156,7 @@ inline vunits operator *(int n, const vunits & x)
vunits r;
r = x;
if (ckd_mul(&r.n, n, x.n))
- error("integer multiplication wrapped");
+ warning(WARN_RANGE, "integer multiplication saturated");
return r;
}
@@ -210,7 +210,7 @@ inline units hunits::to_units()
{
units r;
if (ckd_mul(&r, n, hresolution))
- error("integer multiplication wrapped");
+ warning(WARN_RANGE, "integer multiplication saturated");
return r;
}
@@ -224,7 +224,7 @@ inline hunits operator +(const hunits & x, const hunits & y)
hunits r;
r = x;
if (ckd_add(&r.n, r.n, y.n))
- error("integer addition wrapped");
+ warning(WARN_RANGE, "integer addition saturated");
return r;
}
@@ -233,7 +233,7 @@ inline hunits operator -(const hunits & x, const hunits & y)
hunits r;
r = x;
if (ckd_sub(&r.n, r.n, y.n))
- error("integer subtraction wrapped");
+ warning(WARN_RANGE, "integer subtraction saturated");
return r;
}
@@ -243,7 +243,7 @@ inline hunits operator -(const hunits & x)
r = x;
// Why? Consider -(INT_MIN) in two's complement.
if (ckd_mul(&r.n, x.n, -1))
- error("integer subtraction wrapped");
+ warning(WARN_RANGE, "integer subtraction saturated");
return r;
}
@@ -265,7 +265,7 @@ inline hunits operator *(const hunits & x, int n)
hunits r;
r = x;
if (ckd_mul(&r.n, x.n, n))
- error("integer multiplication wrapped");
+ warning(WARN_RANGE, "integer multiplication saturated");
return r;
}
@@ -274,7 +274,7 @@ inline hunits operator *(int n, const hunits & x)
hunits r;
r = x;
if (ckd_mul(&r.n, x.n, n))
- error("integer multiplication wrapped");
+ warning(WARN_RANGE, "integer multiplication saturated");
return r;
}
diff --git a/src/roff/troff/number.cpp b/src/roff/troff/number.cpp
index bc92e955d..1ece5dc1d 100644
--- a/src/roff/troff/number.cpp
+++ b/src/roff/troff/number.cpp
@@ -1,4 +1,4 @@
-/* Copyright (C) 1989-2020 Free Software Foundation, Inc.
+/* Copyright (C) 1989-2024 Free Software Foundation, Inc.
Written by James Clark ([email protected])
This file is part of groff.
@@ -129,12 +129,12 @@ bool get_vunits(vunits *res, unsigned char si, vunits
prev_value)
break;
case INCREMENT:
if (ckd_add(&i, prev_value.to_units(), v))
- error("integer incrementation wrapped");
+ warning(WARN_RANGE, "integer incrementation saturated");
*res = i;
break;
case DECREMENT:
if (ckd_sub(&i, prev_value.to_units(), v))
- error("integer decrementation wrapped");
+ warning(WARN_RANGE, "integer decrementation saturated");
*res = i;
break;
default:
@@ -157,12 +157,12 @@ bool get_hunits(hunits *res, unsigned char si, hunits
prev_value)
break;
case INCREMENT:
if (ckd_add(&i, prev_value.to_units(), h))
- error("integer incrementation wrapped");
+ warning(WARN_RANGE, "integer incrementation saturated");
*res = i;
break;
case DECREMENT:
if (ckd_sub(&i, prev_value.to_units(), h))
- error("integer decrementation wrapped");
+ warning(WARN_RANGE, "integer decrementation saturated");
*res = i;
break;
default:
@@ -182,11 +182,11 @@ bool get_number(units *res, unsigned char si, units
prev_value)
break;
case INCREMENT:
if (ckd_add(res, prev_value, u))
- error("integer incrementation wrapped");
+ warning(WARN_RANGE, "integer incrementation saturated");
break;
case DECREMENT:
if (ckd_sub(res, prev_value, u))
- error("integer decrementation wrapped");
+ warning(WARN_RANGE, "integer decrementation saturated");
break;
default:
assert(0 == "unhandled case in get_number()");
@@ -205,11 +205,11 @@ bool get_integer(int *res, int prev_value)
break;
case INCREMENT:
if (ckd_add(res, prev_value, i))
- error("integer incrementation wrapped");
+ warning(WARN_RANGE, "integer incrementation saturated");
break;
case DECREMENT:
if (ckd_sub(res, prev_value, i))
- error("integer decrementation wrapped");
+ warning(WARN_RANGE, "integer decrementation saturated");
break;
default:
assert(0 == "unhandled case in get_integer()");
@@ -351,19 +351,19 @@ static bool is_valid_expression(units *u, int
scaling_unit,
break;
case '+':
if (ckd_add(u, *u, u2)) {
- error("integer addition wrapped");
+ warning(WARN_RANGE, "integer addition saturated");
return false;
}
break;
case '-':
if (ckd_sub(u, *u, u2)) {
- error("integer subtraction wrapped");
+ warning(WARN_RANGE, "integer subtraction saturated");
return false;
}
break;
case '*':
if (ckd_mul(u, *u, u2)) {
- error("integer multiplication wrapped");
+ warning(WARN_RANGE, "integer multiplication saturated");
return false;
}
break;
@@ -393,6 +393,7 @@ static bool is_valid_term(units *u, int scaling_unit,
{
bool is_negative = false;
bool is_overflowing = false;
+ units saved_u = 0; // for use when reading an overlong number
for (;;)
if (is_parenthesized && tok.is_space())
tok.next();
@@ -416,10 +417,9 @@ static bool is_valid_term(units *u, int scaling_unit,
position = (scaling_unit == 'v'
? curdiv->get_vertical_position().to_units()
: curenv->get_input_line_position().to_units());
- // We don't permit integer wraparound with this operator.
if (ckd_sub(&tmp, *u, position)) {
- error("numeric overflow");
- return false;
+ tmp = INT_MAX;
+ warning(WARN_RANGE, "integer value saturated");
}
*u = tmp;
if (is_negative)
@@ -466,7 +466,7 @@ static bool is_valid_term(units *u, int scaling_unit,
if (is_negative) {
// Why? Consider -(INT_MIN) in two's complement.
if (ckd_mul(u, *u, -1))
- error("integer multiplication wrapped");
+ warning(WARN_RANGE, "integer multiplication saturated");
}
return true;
case '.':
@@ -486,16 +486,19 @@ static bool is_valid_term(units *u, int scaling_unit,
do {
// If wrapping, don't `break`; eat and discard further digits.
if (!is_overflowing) {
+ saved_u = *u;
if (ckd_mul(u, *u, 10))
is_overflowing = true;
if (ckd_add(u, *u, c - '0'))
is_overflowing = true;
+ if (is_overflowing)
+ *u = saved_u;
}
tok.next();
c = tok.ch();
} while (csdigit(c));
if (is_overflowing)
- error("integer value wrapped");
+ warning(WARN_RANGE, "integer value saturated");
break;
case '/':
case '*':
@@ -623,7 +626,7 @@ static bool is_valid_term(units *u, int scaling_unit,
tok.next();
if (is_negative) {
if (ckd_mul(u, *u, -1))
- error("integer multiplication wrapped");
+ warning(WARN_RANGE, "integer multiplication saturated");
}
return true;
}
@@ -642,13 +645,12 @@ units scale(units n, units x, units y)
return (n * x) / y;
}
double res = n * double(x) / double(y);
- // We don't implement integer wraparound when scaling.
if (res > INT_MAX) {
- error("numeric overflow");
+ warning(WARN_RANGE, "integer value saturated");
return INT_MAX;
}
else if (res < INT_MIN) {
- error("numeric overflow");
+ warning(WARN_RANGE, "integer value saturated");
return INT_MIN;
}
return int(res);
@@ -671,9 +673,17 @@ vunits::vunits(units x)
if (ckd_add(&n, x, vcrement))
is_overflowing = true;
}
+ if (is_overflowing) {
+ if (x < 0) {
+ warning(WARN_RANGE, "integer value saturated");
+ n = INT_MIN;
+ }
+ else {
+ warning(WARN_RANGE, "integer value saturated");
+ n = INT_MAX;
+ }
+ }
n /= vresolution;
- if (is_overflowing)
- error("integer addition wrapped");
}
}
@@ -694,9 +704,17 @@ hunits::hunits(units x)
if (ckd_add(&n, x, hcrement))
is_overflowing = true;
}
+ if (is_overflowing) {
+ if (x < 0) {
+ warning(WARN_RANGE, "integer value saturated");
+ n = INT_MIN;
+ }
+ else {
+ warning(WARN_RANGE, "integer value saturated");
+ n = INT_MAX;
+ }
+ }
n /= hresolution;
- if (is_overflowing)
- error("integer addition wrapped");
}
}
_______________________________________________
Groff-commit mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/groff-commit
