# Criticism on x86_64 nasm assembly strToInt and printInt implementation

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. • Welcome to CodeReview@SE. For X86 integer negation, you'd have to argue why to avoid the proper instruction. – greybeard Sep 10 '20 at 5:37 • thank you for the recommendation! – Dagar Sep 12 '20 at 22:17 ## 2 Answers ### 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


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


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


;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

• Thank you so MUCH! I really do appreciate you going over my code. I'm actually in tears of JOY! – Dagar Sep 12 '20 at 22:16
• Then good tears, I hope... – Sep Roland Sep 12 '20 at 22:18

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_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

._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
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.

• Thank you for telling me how to make local labels it saves me a lot of time!!! And I appreciate the criticism it means a lot! – Dagar Oct 6 '20 at 3:22