bugGNU roff - Bugs: bug #66001, [troff] implement saturating...

 
 

bug #66001: [troff] implement saturating arithmetic

Submitter:  G. Branden Robinson <gbranden>
Submitted:  Wed 17 Jul 2024 09:47:20 PM UTC
   
 
Category:  Core Severity:  1 - Wish
Item Group:  Feature change Status:  Fixed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  1.24.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Tue 20 Aug 2024 12:10:38 AM UTC, comment #2: 

Where'd the "Planned Release" field go?? :(


commit 17f18cc8cbedb6fcb83b531f253a288a19ec36e0
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   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>.


G. Branden Robinson <gbranden>
Group administrator
Fri 16 Aug 2024 08:37:06 PM UTC, comment #1: 

I think I've got it licked.  All tests passing.


commit bee5cf5dc98600efe48b96014572be9c056419c3
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Fri Aug 16 14:53:13 2024 -0500

    [troff]: Fix Savannah #66001 (saturating arith).

    * 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.

    * doc/groff.texi.in (Numeric Expressions):
    * man/groff.7.man (Numeric expressions):
    * NEWS: Document it.

    Fixes <https://savannah.gnu.org/bugs/?66001>.

    [I decided against reverting commit 19e6016e89, 31 July.  The test
    script has evolved since then.]

diff --git a/ChangeLog b/ChangeLog
index a4df1c4ec..3e5432d34 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,24 @@
+2024-08-16  G. Branden Robinson <g.branden.robinson@gmail.com>
+
+       * 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.
+
+       * 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 <g.branden.robinson@gmail.com>

        * src/roff/troff/number.cpp (get_vunits, get_hunts, get_number)
diff --git a/NEWS b/NEWS
index 48abf9a10..1e1f3d864 100644
--- a/NEWS
+++ b/NEWS
@@ -39,6 +39,9 @@ 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.
+
 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..93b316c2a 100644
--- a/doc/groff.texi.in
+++ b/doc/groff.texi.in
@@ -6189,7 +6189,8 @@ @node Numeric Expressions
 32-bit signed integer, which suffices to represent magnitudes within a
 range of <B1>2 billion.@footnote{If that's not enough, see the
 @cite{groff_tmac@r{(5)}} man page for the @file{62bit.tmac} macro
-package.}
+package.}  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
diff --git a/man/groff.7.man b/man/groff.7.man
index b3c1ef163..37083245d 100644
--- a/man/groff.7.man
+++ b/man/groff.7.man
@@ -976,6 +976,12 @@ .SH "Numeric expressions"
 .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
diff --git a/src/roff/groff/tests/arithmetic-works.sh b/src/roff/groff/tests/arithmetic-works.sh
index f7bb699ad..f5a10a054 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
@@ -137,7 +137,7 @@ input='.
 7: (-1)*(-2147483648) -> @ni
 .'

-output=$(echo "$input" | "$groff" -T ascii)
+output=$(echo "$input" | "$groff" -w range -T ascii)
 echo "$output"

 # 2147483647
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 (jjc@jclark.com)

 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 b887501c7..cdaca35f9 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 (jjc@jclark.com)

 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 subtraction wrapped");
+      warning(WARN_RANGE, "integer subtraction 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 subtraction wrapped");
+      warning(WARN_RANGE, "integer subtraction 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 subtraction wrapped");
+      warning(WARN_RANGE, "integer subtraction 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;
@@ -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 '.':
@@ -495,7 +495,7 @@ static bool is_valid_term(units *u, int scaling_unit,
       c = tok.ch();
     } while (csdigit(c));
     if (is_overflowing)
-      error("integer value wrapped");
+      warning(WARN_RANGE, "integer value saturated");
     break;
   case '/':
   case '*':
@@ -623,7 +623,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;
 }
@@ -671,9 +671,17 @@ vunits::vunits(units x)
       if (ckd_add(&n, x, vcrement))
        is_overflowing = true;
     }
+    if (is_overflowing) {
+      if (x < 0) {
+       warning(WARN_RANGE, "negative integer value saturated");
+       n = INT_MIN;
+      }
+      else {
+       warning(WARN_RANGE, "positive integer value saturated");
+       n = INT_MAX;
+      }
+    }
     n /= vresolution;
-    if (is_overflowing)
-      error("integer addition wrapped");
   }
 }

@@ -694,9 +702,17 @@ hunits::hunits(units x)
       if (ckd_add(&n, x, hcrement))
        is_overflowing = true;
     }
+    if (is_overflowing) {
+      if (x < 0) {
+       warning(WARN_RANGE, "negative integer value saturated");
+       n = INT_MIN;
+      }
+      else {
+       warning(WARN_RANGE, "positive integer value saturated");
+       n = INT_MAX;
+      }
+    }
     n /= hresolution;
-    if (is_overflowing)
-      error("integer addition wrapped");
   }
 }


G. Branden Robinson <gbranden>
Group administrator
Wed 17 Jul 2024 09:47:20 PM UTC, original submission:  

For what the bugs dependent on bug #64301 want, saturating arithmetic would be vastly more useful than wrapping arithmetic.

However #64301 is still a crasher (when the compiler flag `-ftrapv` is used) so it needs to be fixed on its own terms.

Restrict that bug to those terms and undertake the saturation work here.

G. Branden Robinson <gbranden>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by gbranden (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-08-20 gbranden Planned ReleaseNone 1.24.0
    2024-08-20 gbranden StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2024-07-17 gbranden Dependencies- bugs #63587 is dependent

    Back to the top

    Powered by Savane 3.13-df36.
    Corresponding source code