ZeroDivide bug in float printing

list,

There were discussion on this list about this bug in the debian package:

st> 0.1
Object: 1 error: The program attempted to divide a number by zero
ZeroDivide(Exception)>>signal (ExcHandling.st:254)
SmallInteger(Number)>>zeroDivide (SysExcept.st:1426)
Fraction>>setNumerator:setDenominator: (Fraction.st:485)
Fraction class>>numerator:denominator: (Fraction.st:66)
Fraction>>- (Fraction.st:151)
...
st> 4294967296 * 4294967296
0

Last night I tracked it down. It only happens with:

 * 64 bit arch
 * gcc 4.9.x

The reason is smalltalk has to detect integer multiplication overflow
reliably; and there were 3 ways in the code:

 1. use __builtin_mul_overflow
 2, see if the result is too big or too small
 3, basically a * b / b == a ? OK : OVERFLOW

gcc 5+ and clang use 1. 32 bit arch use 2. they all works. However, on
gcc 4.9 and 64 bit it has to fall back to 3 which does not work. The
reason is gcc is being overly smart and optimize it away. gcc does
provide a switch "-fwrapv" to disable this optimization, but I am not
sure what other thing it may cause and we dont want to get caught in the
compiler shenanigan again in other compilers or other platform.

I propose to change 3 into a more conservative and slower test
such as:

(MAX_ST_INT / abs(b)) < abs(a) ? OVERFLOW : OK

I will follow up with a patch

Derek

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Derek -

good job tracking down the source of that bug - note that, per the
contributing guide:

  "If you are contributing non-trivial amounts of code to GNU Smalltalk,
  you will be asked to sign a copyright assignment form provided by the
  FSF."

you have already given enough information for someone to fix this
particular bug; so you can probably skip the explicit patch if you like
- of course, if you would like to contribute more in the future, do feel
free to begin the copyright assignment process


http://smalltalk.gnu.org/development/contributing

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

To fix 64bit mode integer multiply overflow detection.
This method should be more robust wrt. compiler optimization


diff --git a/libgst/interp.inl b/libgst/interp.inl
index dbc631b..3583b19 100644
--- a/libgst/interp.inl
+++ b/libgst/interp.inl
@@ -199,15 +199,23 @@ mul_with_check (OOP op1, OOP op2, mst_Boolean *overflow)
   /* This fallback method uses a division to do overflow check */
   else
     {
-      if COMMON ((((uintptr_t) (a | b)) < (1L << (ST_INT_SIZE / 2))
-          || b == 0
-          || result / b == a)
-                 && !INT_OVERFLOW (result))
-        return FROM_INT (result);
-      else
-        *overflow = true;
+      intptr_t abs_a = ABS (a);
+      intptr_t abs_b = ABS (b);
+      intptr_t limit = (1L << (ST_INT_SIZE / 2 - 1));
+      if (COMMON ( abs_a < limit && abs_b < limit ))
+ goto legal;
+      if (UNCOMMON (b == 0L))
+ goto legal;
+      if (UNCOMMON (INT_OVERFLOW (result)))
+ goto overflow;
+      if (UNCOMMON (MAX_ST_INT / abs_b < abs_a))
+ goto overflow;
+    legal:
+      return FROM_INT (result);
     }
 
+ overflow:
+  *overflow = true;
   return FROM_INT (0);
 #endif
 }

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

> On 29. Mar 2019, at 00:07, Derek Zhou <[hidden email]> wrote:
>
> list,
>

thanks for looking into it!


> There were discussion on this list about this bug in the debian package:

Are you aware of:

http://git.savannah.gnu.org/cgit/smalltalk.git/commit/libgst/interp.inl?id=72ada189aba0283c551ead16635c1983968080b8


> Last night I tracked it down. It only happens with:
>
> * 64 bit arch
> * gcc 4.9.x

Why is this version of GCC used? What prevents the usage of GCC8? Can the
above be backported to the debian package? And let's get a new release out
of the door (maybe skip VisualGST...)


Overflow checking requires lots of thinking. E.g. have you considered
where abs(a) < 0? I would prefer if we can exclusively use the compiler
primitive.



_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Holger Freyther writes:
That's good; however there are poor poeple still stuck with old
compilers
>> I propose to change 3 into a more conservative and slower test
>> such as:
>>
>> (MAX_ST_INT / abs(b)) < abs(a) ? OVERFLOW : OK
>
> Overflow checking requires lots of thinking. E.g. have you considered
> where abs(a) < 0? I would prefer if we can exclusively use the compiler
> primitive.
compiler primitives are nice; but there should be fallback path that
also work

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

> On 29. Mar 2019, at 10:15, Derek Zhou <[hidden email]> wrote:
>
>
>> http://git.savannah.gnu.org/cgit/smalltalk.git/commit/libgst/interp.inl?id=72ada189aba0283c551ead16635c1983968080b8
> That's good; however there are poor poeple still stuck with old
> compilers

Is this true for mainstream OS and CPU platforms? Do you have an example?
When I added the builtin (supported by then gcc and now present day clang)
my assumption was sooner than later everyone will be able to use a modern
compiler.

Even for old CentOS there is support for newer compilers. Do you mind
sharing your specific example?



>> Overflow checking requires lots of thinking. E.g. have you considered
>> where abs(a) < 0? I would prefer if we can exclusively use the compiler
>> primitive.
> compiler primitives are nice; but there should be fallback path that
> also work

I think it depends. Undefined behavior is difficult to grok and it feels
we are walking very close to the edge.

By skimming the code I feel we have to look very carefully at it. Not
because I think it is wrong but because it is not trivial. The cognitive
load for using the builtin is a lot lower.

Let me try to review this in depth.

holger
_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Fri, 29 Mar 2019 09:07:48 +0000 Holger wrote:
also, there are a number of distros that fully support i686 computers,
such as debian/parabola/arch32/connochaetos - parabola i686 with a fully
graphical desktop, a 5.0 kernel, and GCC v8.2.1 can run on a pentium
2 with 128MB RAM - arch is actually working on a port for 486; which is
incomplete, but already runs linux 5 and GCC 8

for that reason, in order to support 99% of PC users, while still
allowing for a variety of popular distros, the oldest platforms that
need to be supported are the oldest of the current LTS distros, such as
debian - the current GCC in debian stable (debian9) is v6.3.0, and very
soon that will be v8.3.0 (debian10)


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Fri, 29 Mar 2019 09:07:48 +0000 Holger wrote:
> And let's get a new
> release out of the door (maybe skip VisualGST...)

what is the problem with VisualGST? - isnt that just the GTK class
browser (aka 'gst-browser') - the "about" modal in mine shows:
"VisualGST 0.8.0" - that is from the arch 'smalltalk' package v3.2.91,
and is working well on parabola/arch

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk