Infinite Loop Happening In MASM X86 Assembly Program

Question

I'm currently in a computer organization and architecture class at my university, and we've been working with MASM a lot, and I've been toying around with some programs out of my own interest. Right now I'm writing a program that asks the user to input 10 numbers into an array, then it will calculate the largest number in the array. However, my issue is that unless the biggest number just so happens to be the very last number inputted, this program will crash and an infinite loop will form. I checked the contents of the ecx, and it appears that it goes... 10 9 8 7 6 5 4 3 2 1 -1 -2 -3, so it just skips 0. I'm sort of lost here, and not really sure what I'm doing wrong. Excuse my coding style, as I am still very new to this, but just trying to understand. Thanks!

INCLUDE Irvine32.inc

.data
    currentNum DWORD ?
    largestNum DWORD ?
    numbers DWORD 10 DUP(0)
    prompt BYTE "Enter a number:  ", 0
    message BYTE "The largest number is:  ", 0

.code
main PROC
    mov ecx, LENGTHOF numbers
    mov esi, OFFSET numbers
    mov eax, esi

    L1 :
        mov edx, OFFSET prompt
        call WriteString
        call ReadInt
        mov[esi], eax
        add esi, TYPE numbers
        loop L1

    mov ecx, LENGTHOF numbers
    mov esi, OFFSET numbers
    mov eax, [esi]
    mov largestNum, eax
    mov currentNum, eax

    L2:
        mov eax, [esi]
        mov currentNum, eax
        add esi, TYPE numbers
        cmp eax, largestNum
        ja setNewMax
        loop L2

    setNewMax:
        mov eax, currentNum
        mov largestNum, eax
        loop L2

    mov eax, largestNum
    mov edx, OFFSET message
    call WriteString
    call WriteDec
    call Crlf
    call Crlf

    exit

main ENDP


END main```

So, your first `loop L2` instruction will run ecx times, right? What do you think happens once that instruction hits zero? Since it doesn't jump back to L2, it just proceeds to the next statement, which is setNewMax. Probably not what you intended. FYI, when it hits the second loop instruction, it will decrement ecx again, underflowing to 0xffffffff. That means it's going to run for a long time. — David Wohlferd, May 21 '20 at 07:20
There's zero reason to copy the current array element to a static / global variable `currentNum`. Just use registers; that's what they're for. Registers are like local vars in C (you can also use stack space if you run out of registers). — Peter Cordes, May 21 '20 at 07:38
Your loop body only needs to be a few instructions long, like `cmp eax, [esi]` / `jna` over a `mov eax, [esi]` to update a max, then pointer increment + loop. — Peter Cordes, May 21 '20 at 07:50

Tobias · Answer 1 · 2020-05-21T08:14:22.007

2

The problem lies with the two loop instructions:

       ...
       ja    setNewMax
       loop  L2
setNewMax:
       mov   eax, currentNum
       mov   largestNum, eax
       loop  L2

The first loop (just before the setNewMax label here decrements ecx, discovers that it is zero and does not jump. Hence, it just continues executing the next instruction, i.e. mov eax, currentNum. Three instructions later, it encounters another loop instructions. It decreases ecx (which is now -1 and not zero) and hence takes the loop.

You could insert a jump out of this loop like so:

       ...
       ja    setNewMax
       loop  L2
       jmp   exitLoop
setNewMax:
       mov   eax, currentNum
       mov   largestNum, eax
       loop  L2
exitLoop:
       ...

Alternatively, you could go with just a simple loop instruction and write the second part of your loop like this:

       ...
       jna   endOfloop
setNewMax:             ; <- this label is not necessary anymore
       mov   eax, currentNum
       mov   largestNum, eax
endOfloop:
       loop  L2

Arguably, this code might be a bit easier to reason about because there is now only one loop instruction, and by inverting the logic of the ja jump, you even save an instruction :).

edited May 21 '20 at 08:14

answered May 21 '20 at 07:21

Tobias

1,321
6
11

That's not the only possible fix. The other obvious way to fix this would be not doing the tail-duplication optimization in the first place, instead doing a `jna` over the `mov` instructions. But that would cost an extra taken branch in the common case instead of not-taken. (The OP obviously doesn't care about performance, though, given how they keep copying data to named static variables instead of using registers, and [they're using the `loop` instruction at all](https://stackoverflow.com/questions/35742570/why-is-the-loop-instruction-slow-couldnt-intel-have-implemented-it-efficiently).) – Peter Cordes May 21 '20 at 07:35
@PeterCordes Yes, I am aware that there are other solutions, but in questions that are (implicitly) about understanding code, I feel it is important to stick as close to the original code as possible. After all, this question is about understanding the behaviour of the `loop` instruction, not optimisation. – Tobias May 21 '20 at 07:51
My comment started off as a nit-pick about your choice of phrasing: *You will have to* implies there aren't other solutions. But in fact it's *easier* IMO to fix it by turning the conditional into a standard idiomatic `if` with a forward jump using the opposite condition. Although it's not like SO is short of questions about how to do an `if` in asm, and probably hardly anyone ever finds them. – Peter Cordes May 21 '20 at 08:02
@PeterCordes Point taken. I have updated my answer accordingly. – Tobias May 21 '20 at 08:15

Infinite Loop Happening In MASM X86 Assembly Program

1 Answers1