Number guessing game for x86

Question

I'm new to assembly and I've just finished this guessing game that I wanted to make to improve my skills in this language. It runs in a Unix environment and does the following:

• generates a random number between 0 and 255,
• the user inputs a guess belonging to that range,
• the program converts it into an integer,
• the program then emits a feedback based on whether the input is too low, high or right,
• the program gets back to the first step if it's not the right number else it's the end.

I haven't noticed any bugs yet; it seems to work properly. There's a few things I would like to address in the future; tell me what I should add to this list after you've taken a look at the code below:

• the code is not made to be robust: it is very error-prone if the user puts in the wrong type of data and/or too many chars,
• the switch or match part, no matter what you call it, of the guess function feels a bit weird to me; there's a lot of unnecessary assignment to the registers and I do the same things multiple times (like doing the comparison)
• the range 0-255 is a bit weird, often it's 1-100 but it's not unplayable: the worst case scenario is 1+log_2(256) = 9 moves in worst case dichotomic search; also it avoids further manipulation of the random number which could easily cause errors during development.

It's written using AT&T syntax.

main.s

.section .data

.equ SUCCESS, 0

.equ EXIT, 1

.equ SYS_CALL, 0x80

.section .text

.globl _start
_start:
    call random
    
    pushl %eax       # Save random on the stack as arg
    call guess
    addl $4, %esp
    
    movl $SUCCESS, %ebx
    movl $EXIT, %eax
    int $SYS_CALL

random.s

.section .data

.equ GET_RAND, 355

.equ GET_RAND_FLAGS, 0

.equ SYS_CALL, 0x80

.section .bss
.equ RAND_SIZE, 1
.lcomm RAND, RAND_SIZE

.section .text

# Output between 0 and 255
.globl random
.type random, @function
random:
    pushl %ebp
    movl %esp, %ebp

    movl $GET_RAND, %eax
    movl $RAND, %ebx
    movl $RAND_SIZE, %ecx
    movl $GET_RAND_FLAGS, %edx
    int $SYS_CALL

    movl RAND, %eax

    movl $0, RAND    # clear buffer

    movl %ebp, %esp
    popl %ebp
    ret

guess.s

.section .data
MSG_RIGHT: .ascii "That's the right number!\n" 
MSG_LOWER: .ascii "Lower!\n" 
MSG_HIGHER: .ascii "Higher!\n" 

.equ LEN_RIGHT, 25
.equ LEN_LOWER, 7
.equ LEN_HIGHER, 8

.equ READ, 3
.equ WRITE, 4

.equ STDIN, 0
.equ STDOUT, 1

.equ SYS_CALL, 0x80

.equ CHAR_NEW_LINE, 10
.equ CHAR_ZERO, 48

.section .bss
.equ BUFFER_SIZE, 4 # not much size needed, just 4 bytes would suffice 
                    # (from "0" to "255" + '\n')
.lcomm BUFFER_DATA, BUFFER_SIZE

.section .text

# INPUT: the secret number
#
# iteratively guess the number and get a feedback
# until win
#
# -4(%ebp) -> user integer input 
# -8(%ebp) -> winning flag ($1 = win) 
.globl guess
.type guess, @function
guess:
    pushl %ebp
    movl %esp, %ebp
    movl $0, -8(%ebp) # clear flag

    get_input:
        # clear input
        movl $0, -4(%ebp)

        movl $READ, %eax
        movl $STDIN, %ebx
        movl $BUFFER_DATA, %ecx
        movl $BUFFER_SIZE, %edx
        int $SYS_CALL
    
    # convert string into number
    atoi: 
        movl $0, %ecx # current digit
        movl $0, %edx # current result
        movl $BUFFER_DATA, %ebx # pointer into %ebx
        
    atoi_next:
        cmpb $CHAR_NEW_LINE, (%ebx)  # is the char '\n'?
        je atoi_end

        # update the current result
        imull $10, %edx  # %edx *= 10
        movb (%ebx), %cl # %ecx = char (putting in %ecx before hand to prevent overflow)
        subb $CHAR_ZERO, %cl    # %ecx -= '0'
        addl %ecx, %edx

        movb $0, (%ebx)  # clear behind myself

        incl %ebx        # move the pointer
        jmp atoi_next

    atoi_end:
        movl $0, (%ebx)     # clear again

        movl %edx, -4(%ebp) # save int input as loc var
        movl 8(%ebp), %ebx  # put random into %ebx

    # case input == random
    movl $1, -8(%ebp)  # right flag
    movl $MSG_RIGHT, %ecx
    movl $LEN_RIGHT, %edx
    cmpl %ebx, -4(%ebp)
    je write

    # case input < random
    movl $0, -8(%ebp)  # wrong flag
    movl $MSG_HIGHER, %ecx
    movl $LEN_HIGHER, %edx
    cmpl %ebx, -4(%ebp)
    jl write

    # case input > random
    movl $0, -8(%ebp)  # wrong flag
    movl $MSG_LOWER, %ecx
    movl $LEN_LOWER, %edx
    cmpl %ebx, -4(%ebp)
    jg write

    write:
        movl $WRITE, %eax
        movl $STDOUT, %ebx
        int $SYS_CALL

    cmpl $0, -8(%ebp)  # is the flag false?
    je get_input

    movl %ebp, %esp
    popl %ebp
    ret

and to compile: build.sh (execute it in the same directory as main.s, random.s and guess.s)

#build set of assembly files together

#MODIFY BEGIN - /!\ filenames without extensions
main="main"                 # entry-point
out="out"                   # output executable
dependencies=("guess" "random")     # dependencies of the project
#MODIFY STOP

as --32 $main.s -o $main.o

obj_files="$main.o"
for dep in ${dependencies[@]}; do
    as --32 $dep.s -o $dep.o
    obj_files+=" $dep.o"
done

ld -m elf_i386 $obj_files -o $out

rm $main.o
for dep in ${dependencies[@]}; do
    rm $dep.o
done 

Then you can just run ./out

Answer 1

movl RAND, %eax
movl $0, RAND    # clear buffer

In the context of this program it's probably fine, but RAND has a size of 1 so generally you should be treating it as being one byte, to avoid loading unrelated data or overwriting some adjacent buffer.

Clearing the buffer seems unnecessary.

the switch or match part, no matter what you call it, of the guess function feels a bit weird to me, there's a lot of unnecessary assigning to the registers and I do the same things multiple times (like doing the comparison)

Yes, and you can remove some of it. You can do one comparison, and branch several times, even with the stores to the "continue flag" and the loads of ecx and edx in between, since none of that affects the flags. You only need to do movl $0, -8(%ebp) for the "higher" case, for the "lower" case it is already zero at that point.

jg write

write:
    movl $WRITE, %eax

That jg doesn't do anything useful. Regardless of whether it is taken or not, the same code is executed next: the movl.

Other stuff

• Reading from 8-bit registers is safe as far as I know, writing to 8-bit registers is subject to various quirks. I recommend avoiding it if reasonably possible, which it usually is by using movzx to avoid having to load a byte from memory into a byte register, and by doing most operations in 32-bit registers. Truncating the result to a byte is usually free since storing the value from a byte register to memory is fine. Losing a little bit of performance is not really important in this program specifically but in general you may as well avoid the pitfalls.
• The best way to zero a (32-bit or 64-bit) register is xor same, same.
• You can make the assembler find the size of a string constant. That works with .equ too.
• Speaking of .equ, these don't have to go in the data section, though there is no harm to organizing your files that way. There is no data, they're assemble-time constants. But if you use the . - label trick to calculate a string length (. means "current address") then that must be located directly after the string itself.

Answer 2

In addition to what @harold wrote...

    movl %esp, %ebp
    movl $0, -8(%ebp) # clear flag

Expecting the stack space below ESP to keep its value is arguably a bug. It does happen to work in this program because you haven't installed any signal handlers, so nothing from inside the process will mess up your user-space stack. But the i386 System V ABI doesn't include a red-zone (unlike x86-64 System V) so this isn't safe in general. Use sub $8, %esp.

See https://stackoverflow.com/questions/52258402/is-it-valid-to-write-below-esp for a Windows question that also mentions some stuff about doing the same on Linux. Also my code-golf bigint Fibonacci answer in x86 machine code heavily (ab)uses stack space (because pop is slightly faster than lodsd :P)

---

Instead of defining syscall number constants like GET_RAND yourself, #include <asm/unistd.h> or #include <sys/syscall.h>. (and name your file foo.S so gcc -c foo.S will preprocess before assembling).

Then you can use mov $__NR_getrandom, %eax. My answer on Printing an integer as a string with AT&T syntax, with Linux system calls instead of printf shows an example of doing this (and building with gcc -Wall -static -nostdlib foo.S bar.S. In your case you'd add -m32 since this is 32-bit code.)

---

String literals can go in .section .rodata which gets mapped into read-only pages. That's where compilers put read-only data when compiling for Linux (the Windows equivalent is .rdata, and macOS calls it something else IIRC). You can look at compiler output for examples. (On Godbolt, uncheck the "filter directives" option so you can see the compiler's use of .section and .data / .text directives.)

In your case, you'd actually save a page of memory footprint by avoiding .rodata and just using .data + .bss, because you do unfortunately use some space in .bss. (.bss is contiguous with .data, implemented as the "memsz" of the mapping being larger than the "filesz" in the ELF program headers, so the later bytes get zero-filled when the kernel mmaps the file during execve.) And since your total size is smaller than one 4k page, putting ASCII constant data in with your read/write static storage avoids needing a separate page just for read-only no-exec. (Older ld versions used to put .rodata at one end of the .text section, in the same read-only + exec mapping. But modern ld doesn't, keeping data out of executable pages to harden against Spectre and ROP attacks.)

So if you're optimizing for memory footprint, you could put your read-only .ascii data in .data like you're doing now. But the "right" place for it is in .section .rodata where compilers put it.

Of course if you were optimizing for memory footprint, you'd put the text in .text in with the machine code, and just use stack space for your buffers, e.g. for your input buffer you could push $0 / mov %esp, %ecx to get a pointer to that 4-byte buffer you created. In general you don't need to be using static storage (the equivalent of C global / static variables) for your read/write data. The space you need only lives for the duration of the function that uses it, since you only use one random number and use a system call to get it, not running a PRNG for multiple random numbers in one run of the program. You're already using some local variables (with EBP as a traditional frame pointer); you could do the same for your other variables.

---

.equ CHAR_ZERO, 48

...

subb $CHAR_ZERO, %cl

This looks a lot less clear and intuitive to me than sub $'0', %cl. Some constants are not arbitrary or magic and it's actually clearer to see them right where they're used, if you write them in a semantically meaningful way like '0' instead of 48.

You don't need to zero the input buffer as you loop over it; read returns a length in bytes read, 0 meaning EOF (e.g. if the user hit control-D on a terminal, or if redirected from a file). And negative for error which you should check for.

Your atoi loop could run until seeing a non-digit or until reading all the input bytes. Or you could do one movl $0, (%ecx) right after movl $BUFFER_DATA, %ecx before the system call so you still have a terminator on a short read. Zeroing a byte at a time inside the loop is slower.

As harold mentioned, movl $0, %ecx ; movb (%ebx), %cl is generally slower than movzbl (%ebx), %ecx to do a zero-extending load of a byte, not messing around with partial registers. Also, you only need to load it once; you can compare against the register.

See NASM Assembly convert input to integer? for a loop that reads until it sees a non-digit character (no counter). In that case, reserve at least 5 bytes but read only 4, so there's always a terminating byte even if the user enters 12345\n, so you don't read outside the input array.
It shows some fun tricks like using sub $'0', %ecx / cmp $9, %ecx / ja non_digit to combine the check for [0-9] range with the ASCII -> integer conversion of the candidate digit.

See https://stackoverflow.com/questions/32034178/convert-from-ascii-to-integer-in-att-assembly for a 32-bit AT&T syntax atoi that uses a counted loop with a pretty inefficient loop structure and some wasted mov instructions.

---

   # case input == random
    movl $1, -8(%ebp)  # right flag
    movl $MSG_RIGHT, %ecx
    movl $LEN_RIGHT, %edx
    cmpl %ebx, -4(%ebp)
    je write

    # case input < random
    movl $0, -8(%ebp)  # wrong flag
    movl $MSG_HIGHER, %ecx
    movl $LEN_HIGHER, %edx
    cmpl %ebx, -4(%ebp)
    jl write

    # case input > random
    movl $0, -8(%ebp)  # wrong flag
    movl $MSG_LOWER, %ecx
    movl $LEN_LOWER, %edx
    cmpl %ebx, -4(%ebp)
    jg write                  ## redundant, we already know this is true

    write:
        movl $WRITE, %eax
        movl $STDOUT, %ebx
        int $SYS_CALL

    cmpl $0, -8(%ebp)  # is the flag false?
    je get_input

It's good that you factor out the redundant instructions from each branch (setting up EAX/EBX and doing the int $0x80). That's something beginners often miss.

But this could be smaller and more efficient. It runs setup instructions in each block before deciding to overwrite them with something else, which is a tradeoff between maybe needing a jmp instruction somewhere (and probably larger static size) vs. fewer instructions executed (dynamic size).

Also you're using a boolean variable on the stack to keep track of right vs. wrong. If you set up EAX/EBX before branching, duplicating the int $0x80 twice would be cheaper than that.

You also don't have to keep redoing the cmp; mov doesn't affect FLAGS so you can just jl to reuse the same compare result that you did je with. And the final jg is not useful; at that point we know the g condition is true so jg will be taken. And we're jumping to the next instruction, where fall-through would go if the condition was false. We should just remove it and fall through into the write: label.

  ...
  # rewrite earlier part of the function to have atoi result in EDX instead of EBX
  # so we can hoist the EBX setup without putting it after the CMP before the JE (which would defeat macro-fusion on modern Intel/AMD)

    movl $WRITE,  %eax
    movl $STDOUT, %ebx

   # cmp input, random
    cmp   %edx, -4(%ebp)
    je    write_correct

  # else incorrect one way or the other, set up for lower first
    mov   $MSG_HIGHER, %ecx
    mov   $LEN_HIGHER, %edx   
    jb   write_try_again      # if input < key, use this msg+length

    # else   input > random
    mov  $MSG_LOWER, %ecx
    mov  $LEN_LOWER, %edx     # if we make both lengths the same, omit this
    # fall through to use this msg+length
 write_try_again:
    int   $SYS_CALL       # write(stdout, msg, len)
    jmp   get_input


 write_correct:
    mov   $MSG_CORRECT, %ecx
    mov   $LEN_CORRECT, %edx
    int   $SYS_CALL
    
   ...
    ret

Duplicating the int $0x80 is sort of a tail duplication optimization, repeating a small amount of code instead of storing a boolean and branching on it later to split apart those two paths of flow-control. (BTW, your original code could still have just branched on FLAGS still set from cmpl %ebx, -4(%ebp) after the int $0x80, because Linux's system-call ABI preserves FLAGS.)

In the worst case (higher), this runs 10 uops (cmp/jcc counting as 1) before reaching jmp get_input. vs. your original's worst case of 16 when all the branches fall through. And total code-size is smaller. (Most of the instructions are about the same length, like 5 or 6 bytes for mov-immediate to register or memory respectively, three for cmp reg, 4(%ebp), two for jcc. So total static machine-code size is also smaller.)

As I mentioned in the comments, making both "wrong" messages the same length (padded with a space) could shorten the code. And perhaps make less branchy code a win, like ecx = MSG_LOWER + (input<rand)*8 or something, perhaps with xor %ecx, %ecx ; mov $LEN_WRONG, %edx ; cmp ... ; setb %cl (filling ECX because of the earlier xor-zeroing) ; lea MSG_LOWER(,%ecx,8) to add 0 or 8 according to whether the compare was "below" or not.

I used unsigned compare (above / below) since we don't accept signed input. Treating them as signed positive integers is fine, too, since you're always zero-extending 8-bit to 32-bit, guaranteeing non-negative. But thinking about the 8-bit random number itself, that means it's unsigned.

---

I find "right" sounds like a direction when we're also using "lower" and "higher" names. So I'd prefer renaming the symbol to MSG_CORRECT. And lets have the assembler calculate string lengths for us:

.section .rodata
MSG_CORRECT:  .ascii "That's the right number!\n" 
LEN_CORRECT = . - MSG_CORRECT          # same as .equ  LEN_CORRECT, . - MSG_CORRECT
                                       # but arguably more readable syntax especially when the definition includes a .

MSG_LOWER:    .ascii "Lower! \n"       # pad this by one space to make the lower/higher message lengths the same
LEN_LOWER = . - MSG_LOWER

MSG_HIGHER:   .ascii "Higher!\n" 
LEN_LOWER = . - MSG_HIGHER