4
\$\begingroup\$

I wrote an x86 assembly function using MASM that tests for whether or not an array is continuous. That is to say, if an array with length n that starts with the value k, all the subsequent entries in the array are k+1, k+2, ... k+n.

I am interested in becoming better at x86 and x86-64 assembly and I'm wondering if I could get some feedback on the quality of:

  1. The algorithm itself
  2. Comments - am I too thorough? Not thorough enough?
  3. Label names - are they typically what you would expect handwritten x86 labels to look like?
  4. The part at the end where I set eax in three different cases then jump to a final pop esi/ret instruction - this felt rather ugly to me, but I couldn't think of a better way to do it

Assembly code:

.386
.MODEL FLAT, C
.CODE
; Checks if an array if integers is continuously increasing.
; I.e. if an array of length n starts with element k, all
; the elements in the array are k, k+1, k+2, k+3... k+n
; Uses: 
;   ESI - Loop counter (when loop >= length, done looping)
;   ECX - Data counter (compare for equality with k, k+1, etc...)
;   EDX - Pointer to start of array
; Params:
;   ESP+4 - Array pointer
;   ESP+8 - Array length
; Because of the PUSH ESI instruction at the start of the function,
; the value relative to ESP increases by 4 when they are accessed
; Returns:
; In EAX:
;   1 - If the array is continuous
;   0 - If the array is not continuous
;  -1 - If the function was passed an invalid (<1) length
iscontinuous PROC   
    ; Preserve ESI          
    push    esi
    ; Check for valid (>=1) length
    mov esi, dword ptr [esp+12]
    cmp esi, 1
    jl  invalid_length
    ; Load data array pointer into EDX and deref into ECX (for first number in data)
    mov edx, dword ptr [esp+8]
    mov ecx, dword ptr [edx]
    ; Set EAX as loop counter (start at 0 and keep going till EAX=ESI)
    xor eax, eax
continuity_loop:
    ; Compare array element with current position of counter (pointer to array + offset * sizeof(int))
    cmp dword ptr [edx+eax*4], ecx
    jne not_continuous
    inc     ecx
    inc eax

    cmp eax, esi
    jl  continuity_loop

; If we reached here without branching into not_continuous, we are good to go; return 1 indicating success
    mov eax, 1
    jmp end_iscontinuous    
not_continuous:
    xor eax, eax
    jmp end_iscontinuous
invalid_length:
    mov eax, -1
end_iscontinuous:
    pop esi
    ret 
iscontinuous ENDP
END

C code for test driver:

#include <stdio.h>

int __cdecl iscontinuous(int *data, int len);

int __cdecl wmain(int argc, wchar_t *argv[])
{
    int test[20] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 13, 14, 15, 16, 17, 18, 19, 20 };
    wprintf_s(L"Test 1 (expected pass): test[0..11] = %d\nTest 2 (expected fail): test[0..20] = %d\nTest 3 (expected pass): test[13..20] = %d\n", 
        iscontinuous(test, 11), 
        iscontinuous(test, 20), 
        iscontinuous(test+12, 7)
    );
    return 0;
}

Program output:

Test 1 (expected pass): test[0..11] = 1
Test 2 (expected fail): test[0..20] = 0
Test 3 (expected pass): test[13..20] = 1
\$\endgroup\$

2 Answers 2

Reset to default
4
\$\begingroup\$

1.The algorithm itself

...if an array with length n that starts with the value k, all the subsequent entries in the array are k+1, k+2, ... k+n.

What you're saying here is not entirely true! The last element in such an array would hold k+n-1

; Uses: 
;   ESI - Loop counter (when loop >= length, done looping)
;   ECX - Data counter (compare for equality with k, k+1, etc...)
;   EDX - Pointer to start of array

You are obviously free to choose which register to use for what purpose, but since the beginning of (programming) time these few registers have had their own dedicated uses. It would therefore increase the readability of your program if you respected this (ECX Counter, EDX Data, ESI Pointer):

; Uses: 
;   ECX - Loop counter (when loop >= length, done looping)
;   EDX - Data counter (compare for equality with k, k+1, etc...)
;   ESI - Pointer to start of array

Params:
;   ESP+4 - Array pointer
;   ESP+8 - Array length

At first glance this felt wrong. Rest assured it's correct, but the mention of something relative to ESP always only makes sense when it's absolutely clear where ESP is pointing to. I would have preferred it like:

Params:
;   1st param - Array pointer
;   2nd param - Array length

If you're really interested in becoming better at x86, you should avoid retrieving the parameters using ESP relative addressing. It works and it can shave off a few bytes, but it is very error prone! Imagine at some point in the future you need to preserve more than ESI. It would require closely checking all the code. Using prologue/epilogue code isn't that much more difficult:

push    ebp
mov     ebp, esp
push    esi
mov     esi, [ebp+8]   ;1st parameter - pointer
mov     ecx, [ebp+12]  ;2nd parameter - length
...
pop     esi
pop     ebp
ret

2.Comments - am I too thorough? Not thorough enough?

Your comments certainly explain well enough the purpose of the code, albeit this one (; Preserve ESI) is a bit superfluous.
What would greatly enhance the readability is you writing tail-comments in stead of whole-line-comments

mov edx, dword ptr [esp+8]  ;Load data array pointer
mov ecx, dword ptr [edx]    ;First number in data
xor eax, eax                ;Loop counter (start at 0 and keep going till EAX=ESI)

3.Label names - are they typically what you would expect handwritten x86 labels to look like?

You used very descriptive names for all of your labels. This is good. Please allow these 2 observations:

  • Whenever possible use local labels. Many assemblers allow prepending a dot in front of the name to declare a label locally. Refer to your manual.

  • Where labels are involved, redundancy becomes a good thing. I would have included an additional label .IsContinuous

    .IsContinuous:           <-- Superfluous, but enhances readability
    mov eax, 1
    jmp .EndIsContinuous    
.NotContinuous:
    xor eax, eax
    jmp .EndIsContinuous
.InvalidLength:
    mov eax, -1
.EndIsContinuous:

4.The part at the end where I set eax in three different cases then jump to a final pop esi/ret instruction - this felt rather ugly to me, but I couldn't think of a better way to do it

Looking at the code I've just written above, I don't think there's anything ugly about it. It's the cleanest way to do it, and overtime it'll prove that it is self-explanatory.

The main loop performs a redundant first/only iteration as mentioned in the answer by @DavidWohlferd. You solve this by a one-time jump towards the end of the loop:

    xor     eax, eax
    jmp     .start
.again:
    cmp     dword ptr [edx+eax*4], ecx  ;Compare element with current data
    jne     .NotContinuous
.start:
    inc     ecx                         ;Next data
    inc     eax                         ;Next index
    cmp     eax, esi                    ;Compare current index with length
    jb      .again

Here you see the importance of local labels. No need to invent unique names for all of these trivial locations.

Do note I changed the jl instruction by its unsigned counterpart jb. The numbers involved are inherently positive, and so jb seems more natural.

\$\endgroup\$
2
  • \$\begingroup\$ "It would therefore increase the readability of your program if you respected this" I disagree. x86 has so few registers that it hardly ever makes sense to paint yourself into a corner trying to respect outdated conventions. There's no difference between the general-purpose registers and they can all be used interchangeably, unless you're using one of the old-school instructions with hard-coded registers. I don't think this harms readability. Any competent assembly programmer should be able to figure this out, either from register-flush architectures or from reading compiler-generated code. \$\endgroup\$ Commented Jan 2, 2017 at 14:54
  • \$\begingroup\$ "If you're really interested in becoming better at x86, you should avoid retrieving the parameters using ESP relative addressing." I also couldn't disagree with this more! Beginning assembly programmers set up a stack frame and tear it down in each function using boilerplate prologue & epilogue. Advanced programmers know that this isn't necessary and won't pay the performance penalties. There's a reason that optimizing compilers don't do it, and if you're really interested in becoming better at x86, this is exactly what you should do, and you should become comfortable reading and doing it. \$\endgroup\$ Commented Jan 2, 2017 at 14:56
4
\$\begingroup\$

My thoughts, in no particular order:

  • I'm ok with the comments. Some things seem obvious to me, but I'd rather have an unneeded obvious comment than have something that ISN'T obvious (to me) lacking a comment.
  • The label names seem fine.
  • You might want to include the C prototype as a comment in the asm code. If someone wants to use this code, being able to copy/paste this into their C code might be valuable.
  • You might want to special case handling the first array entry. Not much point in comparing the value you just read with the memory location you just read it from. Maybe inc edx right before continuity_loop and move the inc ecx to right after?
  • How about changing the return codes so that -1 indicates success, 0 indicates bad length, and >0 indicates the mismatched index? This lets you return more useful information than just 'failed'. Also, it helps with how often you have to set eax (hint: move the xor eax, eax up after the push; then eax already contains the 'right' return value in 2/3 cases).
  • Spacing problem on inc ecx?

Note I haven't tried the things I'm suggesting. Sometimes things look good in abstract, but don't work out as neatly when you actually try to write them.

There might be some way to tighten up continuity_loop, but (other than moving the inc ecx) I'm not seeing it right now. If you end up making the other changes, I can give it a second look.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.