OSDev.org

Posted: **Thu Jul 11, 2013 5:41 am**

Hi again!

I'm hoping some comments / criticism on this small asm testing I wrote. The program reads user input into array and then prints out the array using c library (through the pcasm books inc files)

The program works but am I doing something terribly badly?

Code: Select all

;nasm -f elf -o subs_arrays1.o subs_arrays1.asm 
;gcc -m32 -lc -o subs_arrays1 driver.o subs_arrays1.o ../pcasm_shared/asm_io.o
;./subs_arrays1 


%include "../pcasm_shared/asm_io.inc"

;initialized data
segment .data
	prompt_message db "Enter a number: ", 0
	
;unintitialized data
segment .bss
	array_user_input_numbers resd 3
;code
segment .text

	global asm_main

	asm_main:

	;*********************************************************
	;*********************************************************

	for_user_input_begin:
		mov		ecx, 0
		mov		ebx, array_user_input_numbers

		for_user_input_body:
			mov		eax, prompt_message
			call	print_string

			push	dword ebx ;ebp + 8	
			call	get_input
			add		esp, 4

			call	print_nl
		
			add		ebx, 4
			inc		ecx

			cmp		ecx, 3
			je		for_user_input_end

			jmp		for_user_input_body

	for_user_input_end:

	;*********************************************************
	;*********************************************************

	for_print_begin:
		mov		ecx, 0
		mov		ebx, array_user_input_numbers

		for_print_body:
			
			mov		eax, [ebx]
			call	print_int
			call	print_nl
			
			inc		ecx
			add		ebx, 4

			cmp		ecx, 3
			je		for_print_end

			jmp		for_print_body

	for_print_end:

	;*********************************************************
  	;*********************************************************

    mov eax, 1        
    mov ebx, 0        
    int 80h

    ;*********************************************************
    ;*********************************************************

    get_input:
    ;	push	ebx
    ;	push	eax

    	push	ebp
    	mov		ebp, esp
  
    	call	read_int

    	mov		ebx, [ebp + 8]	;how come this be 8 and not 4?
	mov		[ebx], eax

   	pop ebp

	;	pop	eax
	;	pop	ebx

	ret

Posted: **Thu Jul 11, 2013 8:33 am**

Return address is 4 bytes, pushed ebp is 4 bytes.

Posted: **Thu Jul 11, 2013 5:23 pm**

Silly me

Posted: **Fri Jul 12, 2013 12:49 am**

eino wrote:am I doing something terribly badly?

I do not want to be mean but I have a couple of notes. You asked for criticism. Please note that you can fix these things easily. These are my own opinions.

- The general "look & feel" of the code is not very good. I found it hard to read even the logic is quite simple. I would not trust this code based on the impression I get when looking at it. It may work but it is "not always enough". I just want to point out that the code will be considered as low-quality.

- It is hard to say whether the above should be considered being "bad" at this point. If you are learning things, it would not be fair to expect production-quality code.

- I recommend that you study calling conventions that are commonly used. For example, are you sure that your subroutines will keep ecx register intact? You used that for looping. These kind of bugs are quite common. In assembly programming, you are free to use your own calling convention. However, please be aware of this issue.

- The above is not an issue if you are aware of it already and have taken that into account. However, it is hard for someone who reads your code to verify correctness unless you have comments like "subroutines will keep all registers intact".

Hopefully this helps. Do not be disappointed if this sounds unpleasant. You would say this same to yourself later if you continue your interesting hobby.

OSDev.org

Asking for comments on babysteps asm program

Asking for comments on babysteps asm program

Re: Asking for comments on babysteps asm program

Re: Asking for comments on babysteps asm program

Re: Asking for comments on babysteps asm program