GCD/LCM calculator in x86 NASM assembly

Question

After creating a similar program in C++, I've decided to try to write it in x86 assembly language (the type of assembly I was taught in college).

I'd like to receive feedback regarding syntax and clarity. The macros used here were provided by my college, so I don't think that's a concern.

%include "macros.s"

.DATA

num_lbl:   DB   "> Numbers (2): ", 0
gcd_lbl:   DB   "*** GCD: ", 0
lcm_lbl:   DB   "*** LCM: ", 0

num1:   DD   0
num2:   DD   0

num1_cpy:   DD   0
num2_cpy:   DD   0

gcd:   DD   0
lcm:   DD   0

.CODE
.STARTUP

xor       EAX, EAX
xor       EBX, EBX
xor       ECX, ECX
xor       EDX, EDX
xor       EDI, EDI
xor       ESI, ESI

main:
     nwln
     nwln
     PutStr   num_lbl
     nwln
     nwln
     GetLInt   [num1]
     GetLInt   [num2]

     mov   EAX, [num1]
     mov   [num1_cpy], EAX
     mov   EBX, [num2]
     mov   [num2_cpy], EBX

     call   calc_euclidean
     call   calc_lcm

     nwln
     PutStr   gcd_lbl
     PutLInt   [gcd]
     nwln
     PutStr   lcm_lbl
     PutLInt   [lcm]
     nwln

     .EXIT

calc_euclidean:
               mov   EAX, [num2]
               cmp   EAX, 0
               jne   chk_swap

               mov   EAX, [num1]
               mov   [gcd], EAX

               ret

calc_lcm:
         mov   EAX, [num1_cpy]
         mov   EDX, [num2_cpy]
         mul   EDX

         mov   EDI, EAX

         xor   EBX, EBX

         mov   EDX, EDI
         shr   EDX, 16
         mov   EAX, EDI
         mov   BX, [gcd]
         div   BX

         mov   SI, AX
         mov   [lcm], SI

         ret         

chk_swap:
         mov   EAX, [num1]
         mov   EBX, [num2]
         cmp   EAX, EBX
         jl    swap

after_check:
            jmp   loop

swap:
     mov   EAX, [num1]
     mov   EBX, [num2]

     ; temp
     mov   ECX, [num2]

     ; num2 = num1
     ; num1 = temp
     mov   EBX, EAX
     mov   EAX, ECX

     mov   [num1], EAX
     mov   [num2], EBX

     jmp   after_check

loop:
     mov   EDX, [num1]
     shr   EDX, 16
     mov   EAX, [num1]
     mov   BX, [num2]
     div   BX

     mov   EDI, [num1]
     mov   ESI, [num2]
     mov   EDI, ESI
     mov   [num1], EDI
     mov   [num2], EDX

     jmp   calc_euclidean

Answer 1

Comments

@Brendan has explained why comments in assembly are very important. I want to show an alternate style of commenting for assembly language which I feel has certain advantages.

The usual way to comment assembly is to use a comment on the right side of every line. I'll use this snippet from @Brandan's answer as an example. By the way, Brandan's comments are quite good; I am not picking on his examples.

 mov EAX, [num1_cpy]   ;eax = number1
 mov EDX, [num2_cpy]   ;edx = number2
 mul EDX               ;edx:eax = number1 * number2

This is a good practice, and as it is a commone one, the reader of your code will be comfortable with it. There are, however, a few small disadvantages to this style:

• Long comments, when needed, are slightly awkward.
• It is natural, in this style, to tell what each instruction is doing, when what is more needed is to tell why each instruction is doing something. With discipline, you can avoid that trap, as Brandan does, but I see much assembly code that falls prey to it. I believe that the comment-per-line style subtly encourages it.

To avoid these disadvantages, I prefer, in many cases, to comment the code in chunks, like this:

 ; edx:eax = number1 * number2
 mov EAX, [num1_cpy]
 mov EDX, [num2_cpy]
 mul EDX
 ; overflow if result doesn't fit in 32 bits
 test EDX, EDX
 jne .overflow

Indentation

The varying levels of indentation are very confusing. If they are an accident of cut-and-paste, then never mind. If they are a result of using tabs in your source, then I would recommend to not use tabs. If, however, you are attempting to convey information by varying the indentation, I have to admit that I didn't understand what the indentation was trying to convey. I would recommend using only one level of indentation.

Flow of control

The flow of control is what we used to call "spaghetti code." Here I've taken your code and removed everything but flow control:

     ...
main:
     ...
     call   calc_euclidean
     call   calc_lcm
     ...
     .EXIT

calc_euclidean:
     ...
     jne   chk_swap
     ...
     ret

calc_lcm:
     ...
     ret

chk_swap:
     ...
     jl    swap

after_check:
    jmp   loop

swap:
     ...
     jmp   after_check

loop:
     ...
     jmp   calc_euclidean

main is simple. It calls two subroutines, then returns. No problem here.

It appears that the remaining code is all a part of calc_euclidean, except for calc_lcm. calc_lcm is a standalone subroutine, so what's it doing inside calc_euclidean? We'll move calc_lcm to the end.

At the end of swap, there's a jump to after_check, which immediately jumps to loop, the section which follows swap. The jump to after_check can be removed, as well as the labe after_check.

Now there's this sequence:

chk_swap:
     ...
     jl    swap

after_check:
    jmp   loop

swap:

With the label after_check removed, this can be replaced with:

chk_swap:
    ...
    jnl   loop

Here's the new, simpler flow control:

     ...
main:
     ...
     call   calc_euclidean
     call   calc_lcm
     ...  
     .EXIT

calc_euclidean:
     ...
     jne   chk_swap
     ...
     ret
chk_swap:
     ...
     jnl   loop
     ...
loop:
     ...
     jmp   calc_euclidean

calc_lcm:
     ...
     ret

I think there could be more improvement possible; I will leave that as an exercise for the reader.

Answer 2

1) In a language like C or C++, you wouldn't do something like this:

foo           (1,2);
x = bar       (2.5);

It makes it harder to read the code because the function name makes no sense without its parameters. The same is true for assembly. Basically, this:

xor       EAX, EAX

Should be this:

xor EAX, EAX

---

2) In a language like C or C++, you wouldn't do something like this:

b = a * 5 + 2298;
c = b / 9;

Instead, you might do this:

kelvin_x9 = fahrenheit * 5 + 2298;
kelvin = kelvin_x9 / 9;

In assembly, because you can't give registers descriptive names (in the same way that you can give variables descriptive names in higher level languages) and because often you do things that are semantically wrong (e.g. using shifts when you mean multiplication, using LEA when you're not loading an effective address, etc) it's extremely important for code maintainability to use comments. Basically this:

calc_lcm:
     mov   EAX, [num1_cpy]
     mov   EDX, [num2_cpy]
     mul   EDX

     mov   EDI, EAX

     xor   EBX, EBX

     mov   EDX, EDI
     shr   EDX, 16
     mov   EAX, EDI
     mov   BX, [gcd]
     div   BX

     mov   SI, AX
     mov   [lcm], SI

     ret

Should be this:

calc_lcm:
     mov EAX, [num1_cpy]   ;eax = number1
     mov EDX, [num2_cpy]   ;edx = number2
     mul EDX               ;edx:eax = number1 * number2

     mov EDI, EAX          ;edi = number1 * number2, high 32-bits of number discarded

     xor EBX, EBX

     mov EDX, EDI          ;edx = number1 * number2
     shr EDX, 16           ;edx = number1 * number2 / 65536, dx:ax = number1 * number2
     mov EAX, EDI          ;eax = number1 * number2, (can be deleted as EAX already contained this value)
     mov BX, [gcd]         ;bx = GCD
     div BX                ;ax = number1 * number2 / GCD, dx = number1 * number2 % GCD

     mov SI, AX            ;si = number1 * number2 / GCD
     mov [lcm], SI
     ret

For assembly there's only 2 types of bugs - comments that don't describe sane logic, and code that doesn't do what the comments say it should. This allows you to debug the code effectively and drastically reduces the chance of missing mistakes (in addition to the hopefully obvious massive improvement in code readability).

---

3) In a language like C or C++, you would create lots of bugs caused by things like integer overflows because the language doesn't provide a sane/easy way to detect these bugs. In assembly you should not create lots of bugs caused by things like integer overflows because you can easily test for overflows.

For example, this:

calc_lcm:
     mov EAX, [num1_cpy]   ;eax = number1
     mov EDX, [num2_cpy]   ;edx = number2
     mul EDX               ;edx:eax = number1 * number2

     mov EDI, EAX          ;edi = number1 * number2, high 32-bits of number discarded

Should be this:

calc_lcm:
     mov EAX, [num1_cpy]   ;eax = number1
     mov EDX, [num2_cpy]   ;edx = number2
     mul EDX               ;edx:eax = number1 * number2

     test EDX, EDX         ;Will the result fit in 32 bits?
     jne .overflow         ; no, error
     mov EDI, EAX          ;edi = number1 * number2

Note: A better idea would be to do "movzx ebx,word [GCD]" and "div ebx" to divide EDX:EAX by the GCD instead of pointlessly converting the 64-bit value you had in EDX:EAX into a 32-bit value in DX:AX.

---

4) A compiler will check if (e.g.) functions are being called with the correct number of parameters, and will make sure that various registers are saved/restored, etc where necessary. In assembly there is none of that; so you have to do it manually, so you have to be able to do it manually. To be able to do it manually you need to document routines properly. For example, this:

calc_lcm:

Should be this:

;Calculate Lowest Common Multiple
;
;Input:
; [num1_cpy]  Some number
; [num2_cpy]  Some other number
; [GCD]       The GCD
;
;Output:
; [lcm]       The result
;
;Trashed:
; eax, edx, ebx, esi, edi

calc_lcm:

Normally, you'd pass parameters in registers and return parameters in registers to avoid wasting time on loading/storing where avoidable. For example, the entire "calc_lcm" code might be:

;Calculate Lowest Common Multiple
;
;Input:
; eax         Some number
; edx         Some other number
; ebx         The GCD
;
;Output:
; eax         The result
;
;Trashed:
; edx

calc_lcm:
     mul EDX               ;edx:eax = number1 * number2
     div EBX               ;eax = (number1 * number2) / GCD, edx = (number1 * number2) % GCD
     ret

---

5) If you're writing assembly you should write assembly; and not attempt to obfuscate the code by inventing your own language that makes it difficult read the code and impossible to effectively optimise the code.

For example, this code:

 nwln
 nwln
 PutStr   num_lbl
 nwln
 nwln
 GetLInt   [num1]
 GetLInt   [num2]

Is 100% meaningless gibberish.

I don't know what this code should be (and am completely unable to see ways of optimising it that would have been obvious if it was assembly) because I have no idea what any of these macros do. I can't tell which registers get trashed, I can't see which pushes/pops/moves are redundant. I might be able to assume that it'd be safe to delete the first 2 nwln lines and change the string to num_lbl: DB "\n\n> Numbers (2): ", 0 but then that could introduce all sorts of bugs because I didn't bother with the hassle of finding and reading the macros that may do more (or less) than whatever I've assumed they might do.