Criticism on x86_64 nasm assembly strToInt and printInt implementation

Question

I am pretty new to assembly I would like criticism on two "methods" I have written. One converts string to int and the other prints an int to stdout. Any advise helps out.

bits 64

section .data
endl db 10, 0
endlLength equ $ - endl

section .bss
   string resb 100

section .text
global _start
global _strToInt
global _printInt

_start:
   mov rbp, rsp

   ;get int from user
   mov rax, 0
   mov rdi, 0
   lea rsi, [string]
   mov rdx, 100
   syscall

   ;make string to int
   push string
   call _strToInt
   pop r8

   push rax
   call _printInt
   pop  rax

   mov rax, 1
   mov rdi, 1
   mov rsi, endl
   mov rdx, endlLength
   syscall

   mov rax, 60
   mov rbx, 0
   syscall

   ;last value pushed to stack must be address to string. stores int in rax
_strToInt:
  ;prolog
  push rbp
  mov rbp, rsp
  ;save registers
  push rbx

  ;actual code
  xor rax, rax
  mov rbx, 10          ;will be used to multiply by 10 later

  mov rcx, [rbp + 16]
  cmp byte[rcx], '-'   ;check to see if value is negetive
  jne _convertStringLoop
  inc rcx              ;increment the address

  _convertStringLoop:
     xor rdx, rdx      ;clear rdx
     mul rbx           ;multiply rax by 10
     mov dl, [rcx]     ;get 1 byte from rcx address
     sub dl, '0'       ;seb by '0' to get actual value
     add rax, rdx      ;add it to rax
     inc rcx
  cmp byte[rcx], 10    ;see if new line char. exit if new line char
  je _exitConvertStringLoop
  cmp byte[rcx], 0     ;see if end of line char. exit if end of line
  jne _convertStringLoop

  _exitConvertStringLoop:
  mov rcx, [rbp + 16]
  cmp byte[rcx], '-'   ;if not negetive jump
  jne _exitStrToInt
  not rax              ;1's complement (make negetive)
  inc rax              ;2's complement (make negetive)

  _exitStrToInt:
  ;restore registers
  pop rbx
  ;epilog
  pop rbp
  ret

;last value pushed to stack will be printed
_printInt:
  ;prolog
  push rbp
  mov rbp, rsp
  ;save registers
  push rbx

  ;actual code
  mov rsi, rsp
  mov rax, [rbp + 16] ;get the value that user wants to print
  mov rbx, 10         ;will be used to divide by 10 later
  xor rcx, rcx

  cqo
  cmp rdx, -1         ;check to see if negetive
  jne _divisionLoop   ;if not negetive jump

  ;print negetive sign
  mov [rsi + 1], byte '-'
  mov rax, 1
  mov rdi, 1
  inc rsi
  mov rdx, 1
  syscall
  dec rsi

  ;convert to positive number
  mov rax, [rbp + 16]
  ;imul rax, -1
  dec rax
  not rax
  xor rcx, rcx

  _divisionLoop:
     xor rdx, rdx
     div rbx        ;divides number by 10 to move over last digit into rdx reg
     add dl, '0'
     dec rsi
     mov [rsi], dl
     inc rcx        ; count for how many digits added to stack
  cmp rax, 0
  jnz _divisionLoop ;jump if the division did not result in a zero

  ;print all the values
  mov rax, 1
  mov rdi, 1
  mov rdx, rcx
  syscall

  ;restore register
  pop rbx
  ;epilog
  pop rbp
  ret

One more thing I wanted to ask was is it better to do

     mov rax, -20
     mov rbx, -1
     imul rbx

or

     mov rax, -20
     dec rax
     not rax

when I know a number is negetive and I want to turn it to a positive.

Answer 1

Warming up

A good thing that I see missing from your program is writing comments about the function numbers and more.

mov rdi, 0    ; STDIN
mov rax, 0    ; SYS_READ
syscall
...
mov rdi, 1    ; STDOUT
mov rax, 1    ; SYS_WRITE
syscall

mov rdi, 0    ; OK
mov rax, 60   ; SYS_EXIT
syscall

Here's an improvement that you can make:

section .data
endl db 10, 0
endlLength equ $ - endl

The output to STDOUT is length driven. It's not useful to zero terminated this newline. All you could get is that ASCII character 0 gets displayed needlessly.

There are some errors in your code

mov rax, 60
mov rbx, 0
syscall

The first argument must go in RDI. You wrote RBX. Habit from 32-bit ?

---

_convertStringLoop:
  xor rdx, rdx      ;clear rdx
  mul rbx           ;multiply rax by 10
  mov dl, [rcx]     ;get 1 byte from rcx address
  sub dl, '0'       ;seb by '0' to get actual value
  add rax, rdx      ;add it to rax

You have misplaced that xor rdx, rdx instruction. The mul rbx instruction that follows will modify RDX as part of its operation. If the input that you get from the user at the keyboard is really big, then RDX can become non-zero! But wait! You need RDX to be zero so the addition can work correctly...

This will work fine zero-extending DL into EDX into RDX:

_convertStringLoop:
   mul   rbx              ; multiply rax by 10
   movzx edx, byte [rcx]  ; Get 1 byte from RCX address and put in RDX
   sub   dl, '0'          ; seb by '0' to get actual value
   add   rax, rdx         ; add it to rax

and this will work too and allows to not use RBX at all (saves many instructions):

_convertStringLoop:
   imul  rax, 10          ; multiply rax by 10
   movzx edx, byte [rcx]  ; Get 1 byte from RCX address and put in RDX
   sub   dl, '0'          ; seb by '0' to get actual value
   add   rax, rdx         ; add it to rax

---

;print negetive sign
mov [rsi + 1], byte '-'
mov rax, 1
mov rdi, 1
inc rsi
mov rdx, 1
syscall
dec rsi

This code overwrites the value of RBX that you're trying to preserve on the stack! You need to offset by a negative number or else decrement RSI beforehand:

; print negative sign
dec  rsi
mov  byte [rsi], '-'
mov  rdx, 1
mov  rdi, 1   ; STDOUT
mov  rax, 1   ; SYS_WRITE
syscall
inc  rsi

Some better ways of doing things

cqo
cmp rdx, -1         ;check to see if negetive
jne _divisionLoop   ;if not negetive jump

You can test if RAX contains a negative number simply by testing the register with itself and then inspecting the sign flag:

test rax, rax
jns  _divisionLoop   ; RAX is positive

---

cmp byte[rcx], '-'   ;if not negetive jump
jne _exitStrToInt
not rax              ;1's complement (make negetive)
inc rax              ;2's complement (make negetive)

The instruction set offers you the NEG instruction to negate a number:

cmp  byte [rcx], '-'   ; If not negative jump
jne  _exitStrToInt
neg  rax

---

cmp rax, 0
jnz _divisionLoop ;jump if the division did not result in a zero

To find out if a register is 0, you can test it with itself and inspect the zero flag. This shaves off a byte and generally produces faster code:

test  rax, rax
jnz   _divisionLoop

Answer 2

As Sep Roland pointed out, it's really important to specify the significance of the numeric constants you're using. Rather than adding comments on each line though, I'm a huge fan of using the nasm preprocessor to define C-style symbolic constants.

%define STDIN  0
%define STDOUT 1
%define STDERR 2

%define SYSCALL_READ  0
%define SYSCALL_WRITE 1
%define SYSCALL_EXIT  60

If these macros are defined in the same file, you can just use them like you would in C.

mov rax,SYSCALL_READ
mov rdi,STDIN
syscall
...
mov rax,SYSCALL_WRITE
mov rdi,STDOUT
syscall

Having thus specified what the constants represent, this frees up the space on the side for comments on why you're doing what you're doing, why you're not doing it a different way, important warnings for anyone who might later want to make changes, etc.

In other words, I prefer to use the preprocessor for the what and comments for the why. This is obviously just down to personal preference though, and to be quite honest I think most of us would prefer any comments anywhere in any format over the abyss of nothingness we usually get to wade through.

Regarding your code organization, I like the nested indentation you used, I've actually never seen that before, and it actually made me laugh. It's a great idea! I would still also recommend using local labels because it allows you to reuse label names as long as you don't do it within the same global label.

For example:

_strToInt:
  ;prolog
  push rbp
  mov rbp, rsp
  ;save registers
  push rbx

  ;actual code
  xor rax, rax
  mov rbx, 10          ;will be used to multiply by 10 later

  mov rcx, [rbp + 16]
  cmp byte[rcx], '-'   ;check to see if value is negetive
  jne _convertStringLoop
  inc rcx              ;increment the address

  ._convertStringLoop:
     xor rdx, rdx      ;clear rdx
     mul rbx           ;multiply rax by 10
     mov dl, [rcx]     ;get 1 byte from rcx address
     sub dl, '0'       ;seb by '0' to get actual value
     add rax, rdx      ;add it to rax
     inc rcx
  cmp byte[rcx], 10    ;see if new line char. exit if new line char
  je _exitConvertStringLoop
  cmp byte[rcx], 0     ;see if end of line char. exit if end of line
  jne _convertStringLoop

  ._exitConvertStringLoop:
  mov rcx, [rbp + 16]
  cmp byte[rcx], '-'   ;if not negetive jump
  jne _exitStrToInt
  not rax              ;1's complement (make negetive)
  inc rax              ;2's complement (make negetive)

  ._exit:
  ;restore registers
  pop rbx
  ;epilog
  pop rbp
  ret

Prepending a period to your _strToInt subroutine labels to convert them into local labels now means you could for instance write a _strToFloat subroutine that also contained a sensibly named loop label called .convertStringLoop. There are also a billion subroutines that could reasonably contain an ._exit label, so local labels allow you to use descriptive labels without polluting the module's global namespace.

Much less importantly, you don't need to declare BITS 64 in order to assemble in 64 bits. Nasm knows the output needs to be in 64 bits when you declare a 64-bit output format. This is necessary only when you want to assemble a flat binary file in long mode, since (as the documentation explains) raw binary files are probably going to be bootloaders or DOS files, both of which execute in 16-bit real mode.

Of course, there's no harm in explicitly declaring it anyways. If you felt compelled to explicitly declare the target processor of your choosing, however, I might instead suggest declaring the CPU feature level instead, although the default is again usually okay.