patternModerate
GCD/LCM calculator in x86 NASM assembly
Viewed 0 times
lcmx86assemblygcdcalculatornasm
Problem
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, 1
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, 1
Solution
1) In a language like C or C++, you wouldn't do something like this:
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:
Should be this:
2) In a language like C or C++, you wouldn't do something like this:
Instead, you might do this:
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:
Should be this:
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:
Should be this:
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:
Should be this:
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:
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:
Is 100% meaningless gibberish.
I don't know what this code should be (and am completely unable to see
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, EAXShould be this:
xor EAX, EAX2) 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
retShould 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
retFor 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 discardedShould 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 * number2Note: 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
ret5) 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
Code Snippets
foo (1,2);
x = bar (2.5);xor EAX, EAXxor EAX, EAXb = a * 5 + 2298;
c = b / 9;kelvin_x9 = fahrenheit * 5 + 2298;
kelvin = kelvin_x9 / 9;Context
StackExchange Code Review Q#24288, answer score: 12
Revisions (0)
No revisions yet.