Works fine for me in 2.8.2 on Windows XP. Does the image you run it on have to have any special features - size, layers, ... - to cause the problem?

The script does not run on the image, it creates a logo. Any if I have picture open in Gimp or not, the result is the same.

I can't find it in the File->Create->Logos menu... only in a new AlphaToLogo menu in a Script-Fu top-level menu (this is wrong, btw - the script should be modified to add itself to the Alpha to Logo menu in the Filters top-level menu).

How do you run the script?

I run the script by:

Filters -> Script-Fu -> Logos2 -> Nugget...

The script asks for the text, of course, but I see there is also AlphaToLogo menu for Nugget, but I don't know what is that supposed to do. 

I am referring always to the Logo Creation under Filters -> Script-Fu -> Logos2 -> Nugget...

Ah, non-standard menu paths... :)

No problems running the script this way, either. Could be the settings, can you specify some that make it fail?

Problems with third party scripts should be discussed on the user mailing list or via the IRC channel unless it is definite that a script has uncovered a bug. There are too many of third party scripts and many have were written for older versions of GIMP and have not been updated by their authors for a more recent version.

The attached script was written for GIMP 2.4 and needs updating. It won't run in GIMP 2.6 or later due to syntax errors in variable declarations on lines 34, 35, 36, and 171. Those variable declarations do not have an initial value.

In the script attached to the bug report, there are three
calls to 'rand'  (rand 4294967295) with a value that causes
script-fu to loop forever.

A workaround is to use a value less than 2147483647.

The bug is easily reproducible pasting

(random  4294967295)

in the script-fu console.

With the workaround in gimp-2.8.2 the syntax errors 
are not fatal, the script competes successfully.

Ah, so it's Linux-specific - there have been differences in rand behavior on Windows and Linux platforms before.

The rand and random functions are defined in Scheme code so I don't see why it would be Linux specific. I can't reproduce the problem in either GIMP 2.6 or git master. I don't have 2.8 on my machine. I get different numbers each time I call either rand or random using an argument of 4294967295.

Well, it is not exactly repeatable, I've now seen it return
a random number, but after restarting GIMP and repeating
the experiment a few times, now the Script-fu console is
always hanging on (rand 4294967295).

it happens also with gimp-2.9 from git master

Thank you, now we know that it is a bug in the Gimp.

It is possible to reproduce the bug on a 64 bits platform
(excluding Win64) typing in a script-fu console

(set! *seed* 1) (random 4294967295)

 
One problem is that tinyscheme is using the C type
'long' for its integral types. 'long' is platform dependent:
32 bits on 32 bits platforms and win64, 64 bits on the other
64 bits platforms.

Probably the random related functions were developed on a 32bits
platform, on which LONG_MAX is 2147483647L and all integers are,
one way or the other, implicitely truncated to values less than
that. This is not true on all 64 bits platforms.

I don't see the bug mentioned in comment #13. I can't reproduce under Linux with the git master version of GIMP.

Can you still reproduce this with 2.8.10?

Created attachment 268057 [details]
test program

Yes, I can reproduce it with latest gimp-2-8 and master git branches.

Attached a C program implementing in C the scheme functions:

srand, msrg-rand and random
https://git.gnome.org/browse/gimp/tree/plug-ins/script-fu/scripts/script-fu-compat.init#n26
https://git.gnome.org/browse/gimp/tree/plug-ins/script-fu/scripts/script-fu-compat.init#n31
https://git.gnome.org/browse/gimp/tree/plug-ins/script-fu/scripts/script-fu-compat.init#n67

it permits to reproduce on a 32bit system, the behaviour
observed on 64bit system when pasting in GIMP's script-fu
console the following snippet:

(srand 1) (random 4294967295)

if you do not turn optimizations on, the tail call is not
eliminated and it crashes quite soon, for a stack overflow,
enabling optimization I waited awhile, (longer than reasonable
for that call) before hitting Ctrl-C.

Another way to convince yourself is to use guile (which I hope
is portable), in a terminal if I type (from GIMP's root folder)

(tail -c +972 plug-ins/script-fu/scripts/script-fu-compat.init | head -57 && echo "(srand 1) (random 4294967295)") | guile

and press Enter, it hangs.

As I said in a previous comment, the problem is tinyscheme using
the C type 'long' which is platform-dependent.

Created attachment 289064 [details] [review]
Proposed fix for bug 690544

In 32 bits, the problem is not visible because numbers bigger than 2^31-1 are clamped:

> 4294967295
2147483647

therefore typing (random 4294967295) will be internally translated to (random 2147483647) which works just fine.

The actual problem here is that the random number generator can't deal with numbers that large, and correctly detects that the numbers in the requested range would be biased, but fails to realize that retrying until finding one that is not, will only lead to an endless loop. Attached is an *untested* patch that approaches the problem by extracting several numbers when requesting a range greater than 2^31-1.

I don't have a 64 bit system to test this in, sorry.

The approach is to generate a base-2^30 number of two or three digits as necessary whenever the range is out of the generator's capabilities.

Also, please note that I'm not acquainted with Scheme enough to be sure of whether my tail recursivity is correct.

The patch assumes that if the platform supports more than 32 bits, it supports at least 64 bits. I hope that's the case for all platforms that GIMP supports.

I forgot to add that this patch only works for range requests up to 2^60*(2^31-1). I doubt anyone wants random numbers bigger than that, though. In order to fail in that range, it would require a machine with a suitably large word size, to start with (e.g. a 128-bit machine). So, while that is a theoretical concern, it's of no practical significance.

Created attachment 289068 [details] [review]
Proposed fix for bug 690544 v2

Sorry for the spam. I said:

> The patch assumes that if the platform supports more than 32 bits, it supports
at least 64 bits.

This new patch removes that limitation.

Good to see you are still around Pedro :)

Created attachment 289823 [details] [review]
Prooposed fix for bug 690544 v3

I managed to test a modified version of the above patch (one with reduced values, as if prepared for a 16 bit machine). When doing so, I detected that some input numbers overflowed the ceiling calculation.

The new attached patch fixes this overflow. It also reworks the loops, getting rid of some locals, and adds more explanations that will hopefully make life easier for reviewers.

Furthermore, it adds full support for negative input, which the previous patch lacked. It also errors out if the input is too large (that can only happen on a 128-bit machine, probably). It fixes the case of n=-2147483648 which can never return -2147483647 in the current random implementation, and with this patch it can (though the period may be too short for that to happen anyway).

Just as the former, it relies on integer constants that exceed LONG_MAX to be clamped to LONG_MAX (LONG_MIN for negatives) in order to work in platforms with word sizes other than 32 or 64 bits.

Just as before, I couldn't test in a 64-bit machine, but I could test with a reduced version and it works just as well.

Note that for the random generator to work properly, bug #738951 needs to be fixed as well. To test without applying that patch, the following modulo definition can be used to replace the existing built-in one:

(define (modulo a b)
  (let ((x (remainder a b)))
    (if (= x 0) x (if (eqv? (> x 0) (< b 0)) (+ x b) x))))

In most cases (barring those where #738951 applies) the output is compatible with the previous generator. The only exception in 32 bits is (random -2147483648) which I expect will be too uncommon to be a concern.

Created attachment 289855 [details] [review]
Proposed patch for bug 690544 v4

Yet another revision of the patch.

This one fixes the bias detection loop, which was wrong in two ways.

First, the original random function from http://www.cs.cmu.edu/afs/cs/project/ai-repository/ai/lang/scheme/code/math/random/rand2.scm has a bug in that (> r slop) should be (>= r slop), otherwise some numbers would be less likely to appear than others, breaking uniformity, or may not appear at all (which is the very reason for that loop, in the first place). As a trivial example to illustrate the problem, in the original, (random 2147483647) would never produce 0 as output.

Second, that loop was not prepared to handle negative input, which is the only kind of input that it receives with this patch. It just did not discard any biased output when the input was negative.

As a consequence of the first fix, it's likely that the generated numbers become different after many extractions, or with certain arguments. If the very same numbers are desired for compatibility, it's better to leave that fix out. The second one is necessary, though.

Most likely not specific to the Linux platforms (maybe it just doesn't happen (yet) on the Windows platforms, see comment 13). And we got a patch.

This might be 2.10 material.

I suggest a title change: Script-fu (rand 4294967295) hangs on machines with 64-bit longs

Thanks. So, I got this ready to push, any objections?

Pushed to git master

commit 139801f222a8af0fdcdfbd2e90ed8a046d617259
Author: Pedro Gimeno <bugzilla.gnome@personal.formauri.es>
Date:   Sun May 29 15:03:13 2016 +0200

    Bug 690544 - Script-fu (rand 4294967295) hangs on machines with 64-bit longs


and cherry-picked to gimp-2-8

commit 544ad2dec47c7795bdc254c6f656c68e5c2b4bc4
Author: Pedro Gimeno <bugzilla.gnome@personal.formauri.es>
Date:   Sun May 29 15:03:13 2016 +0200

    Bug 690544 - Script-fu (rand 4294967295) hangs on machines with 64-bit longs

I assume this to be fixed, please reopen if someone can still reproduce it.

Review of attachment 289855 [details] [review]:

The patch should not have been commited as it uses numbers that are not supported by TinyScheme. The code may appear to have solved the problem reported but the code will not work as the author intended. I would have marked the patch "rejected".

I'm reopening this report. The solution should not be based on the use of bignums when bignums is not a supported feature in TinyScheme.

Lowering the status Importance from critical to normal as the code no longer causes an infinite loop using the test cases shown in this report.

The bug only affects platforms where a long is 64-bit wide. Have you tested it in such a platform without the patch? Does (random 4294967295) work in it?

You're misunderstanding the patch. It assumes longs, not bignums.

In a platform with 32-bit longs, the minimum parameter 'n' is -2147483648. Therefore the returned random number (returned by internal-random in the patch) is always between -2147483647 and 2147483647.

In such a platform, the first (if (>= n -2147483647) ...) will always be true, and it will return immediately without having used any number out of the range of a machine with 32-bit longs.

The rest of the patch applies to platforms with more than 32 bits, and uses numbers that are valid for them, or numbers that when clamped to LONG_MIN or LONG_MAX (via atol) keep the algorithm working. The goal of version 2 of the patch was precisely to ensure that when such clamping occurred, the function was still valid, so that it also worked in platforms with a long whose number of bits is between 33 and 63 inclusive, should one exist.

The code is carefully written taking into account the limitations of TinyScheme and works as intended.

Created attachment 329233 [details] [review]
Follow-up patch to v4

This is a follow-up to the previous patch. It does the following:

- It takes the function (internal-random) out of the other function. No need to define it every time.
- Removes an unnecessary (abs) call.

In my previous comment, I said that the first (if (>= n -2147483647) ...) will always be true in a 32 bit platform. That's not quite right; I apologize. The patch was written long ago and I didn't remember the details very well. But the code will behave properly even when passed -2147483648 as a parameter. In that case, the second (if (>= n -1152921504606846975) ...) will be triggered, and since that number is out of range for 32 bits, it will count as LONG_MIN (that's how function atol used by TinyScheme works) and therefore it will enter that part, using 2 random numbers to produce the requested 31 random bits, as it should (because the generator used can't generate 31 full bits).

Created attachment 329291 [details] [review]
Follow-up patch to v4

The (abs) call was necessary, per my comment #22. Updated patch attached. Here's a diff -w of this patch to facilitate review:

diff --git a/plug-ins/script-fu/scripts/script-fu-compat.init b/plug-ins/script-fu/scripts/script-fu-compat.init
index 88f5c02..1ca0d1a 100644
--- a/plug-ins/script-fu/scripts/script-fu-compat.init
+++ b/plug-ins/script-fu/scripts/script-fu-compat.init
@@ -66,7 +66,6 @@
 ; Pair           (aabcd)      0.504       0.501
 ; Bust           (abcde)      0.3024      0.3058
 
-(define (random n)
 (define (internal-random n)
   (let* (
         (n (inexact->exact (truncate n)))
@@ -82,6 +81,7 @@
   )
 )
 
+(define (random n)
   ; Negative numbers have a bigger range in twos complement platforms
   ; (nearly all platforms out there) than positive ones, so we deal with
   ; the numbers in negative form.

Regarding the patch in comment #34 I would keep internal-random internal to random unless the intention is to add another random number generator function that will also use internal-random.

What's the status here?

The bug itself is fixed. What remains is just a problem of style and visibility. There's a global function that is defined by (random), called (internal-random). It could be either made local or moved out of the (random) function to make it more obvious that it's global. Kevin didn't like the latter, and I didn't send a patch for the former because I didn't want to keep spamming, and being a problem of style, it's not really important.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gimp/issues/442.