CODEWITHSUNDEEP
assemblyprintinggeometrynasm
Hanako
Hanako

Reputation: 3

Assembly - (NASM 32-bit) Printing a triangle of stars not working properly

EDIT: This problem is already solved. Many thanks to mbratch.

My code outputs:

Current output

But it should display this:

Expected output

I think problem is in the innerloops but I can't fix it, it works properly on the first loop but not on the succeeding ones.

Here's my code:

innerloop1: ;;for(j=n-i;j>0;j--)
mov bl, [i]
sub byte [num], bl
mov dl, [num]
mov byte [j], dl
cmp byte [j], 0
jle printStar

mov eax, 4
    mov ebx, 1
    mov ecx, space
    mov edx, spaceLen
    int 80h
dec dl
    jmp innerloop1

printStar:
mov eax, 4
    mov ebx, 1
    mov ecx, star
    mov edx, starLen
    int 80h

innerloop2: ;;for(k=0;k<(2*i)-1;k++)
mov al, [i]
mul byte [two]
dec al
cmp byte [k], al
jge printMe

mov eax, 4
    mov ebx, 1
    mov ecx, space
    mov edx, spaceLen
    int 80h

    inc byte [k]
    jmp innerloop2

printMe:
mov eax, 4
    mov ebx, 1
    mov ecx, star
    mov edx, starLen
    int 80h

    mov eax, 4
    mov ebx, 1
    mov ecx, newLine
    mov edx, newLineLen
    int 80h

    inc byte [i]
    jmp outerloop

    printSpace:
mov eax, 4
    mov ebx, 1
    mov ecx, space
    mov edx, spaceLen
    int 80h

Upvotes: 0

Views: 1809

Answers (1)

lurker
lurker

Reputation: 58254

There are lots of inefficiencies in your code and it could be much more clearly and concisely written. However, I'll just address the areas that are causing a functional problem.

There are a couple of problems with innerloop1. You are modifying [num] every time through the loop. Instead, you want to do it prior to the loop as an initializer for j. Secondly, you are counting on the value of dl being intact through the execution of the loop, but your mov edx, spaceLen clobbers it, as might the call to int 80h. So you can correct it by this:

    mov dl, [num]       ; INITIALIZE j=n-i
    sub dl, byte [i]
innerloop1: ;;for(j=n-i;j>0;j--)
                        ; REMOVED modification of 'num' here
    mov byte [j], dl
    cmp byte [j], 0
    jle printStar

    mov eax, 4
    mov ebx, 1
    mov ecx, space
    push dx             ; SAVE dx
    mov edx, spaceLen
    int 80h
    pop dx              ; RESTORE dx
    dec dl
    jmp innerloop1

In your second inner loop (innerloop2) you are relying on the pre-initialized value of k every time you enter the loop, which is no longer valid after the first time the loop is encountered.. So you must initialize it each time:

    mov byte [k], 0   ; INITIALIZE k=0
innerloop2: ;;for(k=0;k<(2*i)-1;k++)
    mov al, [i]
    mul byte [two]
    dec al
    cmp byte [k], al
    jge printMe

This all makes the code work. Some additional comments:

  • You need to be cautious about counting on the values of registers and watch where they may get altered
  • You should probably not rely on pre-initialized declarations to initialize loop variables or other data that can vary for that matter. It's generally good practice to explicitly initialize a value in code if you're going to change it regularly
  • Think about how to optimize your code (make it more concise and clear) now that it basically works.
  • Be more consistent in variable usage. Your asm program used num for what was n but also defined a value n, which was a little confusing.
  • Be consistent in your code indentation and spacing. It will make it much easier to read.
  • When doing constructs, such as for loops, try to maintain a consistent approach to doing them every time. It will reduce the chance of errors. For example, manage the loop variables in the same or similar ways.

Upvotes: 1

Related Questions