Why is this variably turning negative?

Question

So my error says that I don't have the rights to access this memory. This is the code for it, I'm trying to get the collaz series working. But my n is getting negative even though it shoudln't.

const int number = 1000000;

//Chain Vars-------------------
int chainLength = 0;
int startingNumber = 0;
int chain = 0;
int n = 0;
//----------------------------

for (int i = 2; i <= 1000000; i++)
{
    n = i;
    chain = 0;

    while (n != 1 && n >= i)
    {
        chain++;
        if ( (n % 2) == 0)
        {
            n = n / 2;
        }
        else
        {
            n = n * 3 + 1;
        }
    }
}

Pastebin: bad - posting code on SO: good. Posting all code: bad - posting code related to question: good — Glenn Teitelbaum, Jan 07 '16 at 21:51
Off-by one error. Indexing of arrays starts at zero, not 1. If it worked in C#, you just got lucky - the code would be just as flawed in C# even if it appeared to work for your chosen test cases. — Peter, Jan 07 '16 at 21:54
@PaulMcKenzie an algorithm is an algorithm. Array bounds are array bounds. Both irrespective of language. — Klitos Kyriacou, Jan 07 '16 at 22:04
@KlitosKyriacou You're late to the game. The code was not posted when I made the comment -- it was a "pastebin" link that I have no access to. Thus I am referring to the general "it works in C#" — PaulMcKenzie, Jan 07 '16 at 22:25

jpo38 · Accepted Answer · 2016-01-07T22:29:43.540

1

In your for loop, you do:

for (int i = 2; i <= 1000000; i++)
{
    n = i;
    chain = 0;

    while (n != 1 && n >= i)
    {
        chain++;
        if ( (n % 2) == 0)
        {
            n = n / 2;
        }
        else
        {
            n = n * 3 + 1;
        }
    }

    //Store the chain length in cache
    cache[i] = chain + cache[n];
    //-------------------------------

    if (cache[i] > chainLength)
    {
        chainLength = cache[i];
        startingNumber = i;
    }
}

At some point while (n != 1 && n >= i) most likely ends up with n being greater than 1000000. You'll then access cache (when you'll do cache[n]) out of bounds (which are [0:1000000]).

Add std::cout << "i is " << i << std::endl; before the while loop. Add std::cout << "n is " << n << std::endl; after. Run the program, you'll get (after some seconds):

...
i is 113381
n is 85036
i is 113382
n is 56691
i is 113383
n is -1812855948
Erreur de segmentation (core dumped)

Here you are. Now, you can use a debugger, identify the bug, fix the bug (most likely rework your loop), and make it work! ;-)

Tip: As n becomes negative, maybe it reached int's max value...then simply use a bugger type (like long long int or uint64_t). Then, you'll most likely not get any overlow (unless you make number bugger).

C# does not manage memory as C++ does. You may get no error if accessing the array out of bounds here (or, as commented above, you just got lucky). I'm not familiar with C#. Accessing arrays must always be avoided it may have undetermined behaviour (could or could not lead to a crash).

edited Jan 07 '16 at 22:29

answered Jan 07 '16 at 21:54

jpo38

20,821
10
70
151

I haven't run it myself, but I can be 100% sure that after the loop exits n will either be == 1 or < 1000000. – Klitos Kyriacou Jan 07 '16 at 22:02
I thought that "n != 1" would only end if it is 1 ... how would i fix that? i can't seem to find the fix :/ – slow Jan 07 '16 at 22:05
@sLowDowN - or.. if `n` gets negative - see my post :) – Danny_ds Jan 07 '16 at 22:06
@Danny_ds I'm the one who upvoted your answer as it's the only correct one so far. But why do you say "not so"? I know n goes negative, and my 100% certainty that n == 1 or < 1000000 still remains valid when n is negative. – Klitos Kyriacou Jan 07 '16 at 22:11
@KlitosKyriacou - Well, yes you're right about n being < 1000000 :) I thought you meant n would not be out of bounds for `cache[]`. And it only gets negative because gets too high. I'll remove that comment. Thanks for the upvote. – Danny_ds Jan 07 '16 at 22:20
@jpo38 C# always does bounds checking on array accesses (unless the compiler can check the bounds statically). In contrast, C++ typically does not do bounds checking but instead exhibits undefined behaviour. Most compilers will add bounds checking if you specify "debug mode". – Klitos Kyriacou Jan 07 '16 at 22:23

Danny_ds · Answer 2 · 2016-01-07T22:27:42.740

1

After running and debugging the program:

    //Store the chain length in cache
    cache[i] = chain + cache[n];

n seems to be 0x93f20374 (at i being 113383) which is negative -1812855948, or would be positive 2482111348 - but overflows to become -1812855948.

while (n != 1 && n >= i)

Loop ends with negative n, causing cache[n] to crash.

edited Jan 07 '16 at 22:27

answered Jan 07 '16 at 22:00

Danny_ds

11,201
1
24
46

how can n get get to negative? Because n only divides by 2 or multiplies by 3 +1 ... this should be a really easy solution but i can't see it :D – slow Jan 07 '16 at 22:10
@sLowDowN - Because it overflows `n` is a signed int, and gets to +2482111348 (unsignded), which converts to -1812855948 with an `int`. – Danny_ds Jan 07 '16 at 22:12

score 1 · Answer 3 · answered Jan 07 '16 at 22:13

1

Like jpo38 said :

Tip: As n becomes negative, maybe it reached int's max value...use a debugger to verify that, simply do, before your while loop:

That was my problem, i then changed "int n" to "long long n" because "long n" was still to small and now it gives me the right answer. Thanks everyone :) So easy but sometimes it's the small things you don't see.

answered Jan 07 '16 at 22:13

slow

566
4
17

Damned....I mentioned the overflow in my post but why am I not getting any more reputation for that! :-( – jpo38 Jan 07 '16 at 22:27
This got me thinking: can it be mathematically proven that n is not going to overflow when you use 64-bit integers? – Klitos Kyriacou Jan 07 '16 at 22:59

Why is this variably turning negative?

3 Answers3