HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

x86 assembly function that determines whether an array is continuous

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
arrayfunctionx86determinesassemblythatwhethercontinuous

Problem

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:

  • The algorithm itself



  • Comments - am I too thorough? Not thorough enough?



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



  • 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
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
j

Solution

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.

Context

StackExchange Code Review Q#146165, answer score: 4

Revisions (0)

No revisions yet.