Can someone explain what I need to change in this 80x86 assembly program?

Question

Here's the code:

.586
.MODEL FLAT

INCLUDE io.h                ; header file for input/output

.STACK 4096

.DATA
prompt1 BYTE    "Enter n1", 0
prompt2 BYTE    "Enter n2", 0
n1 dword ?
n2 dword ?
gcdp dword ?
remp dword ?

string  BYTE    40 DUP (?)  
resultLbl BYTE  "gcd is:",0 

.CODE
_MainProc   PROC
        input prompt1, string, 40   
        atod string             
        mov n1, eax

        input prompt1, string, 40   
        atod string             
        mov n2, eax

        push n2
        push n1
        call gcd
        add esp, 8

        dtoa string, eax
        output resultLbl, string

        mov eax, 0
        ret
_MainProc   ENDP

gcd PROC
        push ebp
        mov ebp, esp
        push n2
        push n1
        mov eax, n1
        mov gcdp, eax
        mov eax, n2
        mov remp, eax

   L1:  mov eax, gcdp
        cdq
        idiv remp
        mov ebx, remp
        mov gcdp, ebx
        mov remp, edx
        cmp edx, 0
        jnz L1

        mov eax, gcdp

        pop ebx
        pop edx
        pop ebp
        ret
gcd ENDP

END

And here's the problem (as stated by my teacher): "reading the parameters from stack is missing. please make sure you are reading your n2 and n1 with byte ptr [ebp+8] and byte ptr [ebp+12], also you don't have to push n1, n2 and pop n1n2 in procedure. The rest looks fine."

So...what's up? What needs to change and what's redundant?

Answer 1

What needs to change and what's redundant?

These globals are not needed:

n1 dword ?
n2 dword ?
gcdp dword ?
remp dword ?

Use registers instead. Lets also move string out of the .data section (initialized data) and into the .data? section (uninitialized data):

.DATA
prompt1     BYTE "Enter n1", 0
prompt2     BYTE "Enter n2", 0
resultLbl   BYTE "gcd is:",0 

.data?
string      BYTE 40 DUP (?) 

.CODE
_MainProc   PROC

    input   prompt1, string, 40     
    atodw   string             
    mov     esi, eax

    input   prompt1, string, 40     
    atod    string             
    mov     edi, eax

    push    edi
    push    esi
    call    gcd
    add     esp, 8

    dtoa    string, eax
    output  resultLbl, string

    mov     eax, 0
    ret
_MainProc   ENDP

gcd PROC
    push    esi
    push    edi
    push    ebx

    mov     esi, [esp + 16] ;n1
    mov     edi, [esp + 20] ;n2

L1:  
    mov     eax, esi
    cdq
    idiv    edi
    mov     ebx, edi
    mov     esi, ebx
    mov     edi, edx
    cmp     edx, 0
    jnz     L1

    mov     eax, esi

    pop     ebx
    pop     edi
    pop     esi
    ret
gcd ENDP

END _MainProc

We "technically" don't have to save esi, edi, or ebx in the gcd proc since nothing external is going to interface with our code, but lets learn the correct way to start. If we don't save those registers, then the params will be at [esp + 4], and [esp + 8]. We can also get a bit advanced and get rid of the string global var and just use the stack to hold the string.