6
\$\begingroup\$

I'm trying to learn x86 assembly and hopefully move on to projects of the scope that more than one person would be involved.

This is a (relatively) simple subroutine written in Intel x86 assembly using MASM that copies one string onto the end of another. I named it strglue to avoid conflict with the C library function strcat (since the driver for this method is a C program) and the MASM macro CatStr.

I'm wondering if I can have some insight on the quality of:

  1. The code itself (optimality of the instructions and register usage)
  2. The documentation
  3. The indentation of the loops and labels

.386
.MODEL FLAT, C
.DATA
.CODE

    ; Glue together (concatenate) two strings
    ; PARAMS:
    ;  [ESP+4] - Pointer to first character in null-terminated destination string
    ;  [ESP+8] - Pointer to first character in null-terminated source string
    ; USES:
    ;  EAX: Holds the destination string during execution
    ;  ECX: Holds the source string during execution:
    ;  EDX: Lower 8 bits (DL) holds characters being copied from source to destination
    ; OUTPUT:
    ;  [ESP+4] - Pointer to first character in null-terminated concatenation of destination string + source string (excluding the null-terminator from the destination string)
    strglue PROC

        ; Step 1: Get a pointer to the null-terminator character in the destination string
        MOV EAX, DWORD PTR [ESP+4]
        ENDOFDEST:  
            INC EAX
            CMP BYTE PTR [EAX], 0
        JNZ ENDOFDEST

        ; Step 2: Copy the source string into the pointer to the end of dest, overwriting the null-terminator
        MOV ECX, DWORD PTR [ESP+8]
        STRCATLOOP:
            MOV DL, BYTE PTR [ECX]
            LEA EAX, [EAX+1]
            MOV BYTE PTR [EAX-1], DL
            LEA ECX, [ECX+1]

            ; If ZF=0  the null terminator in the source string has been reached; stop looping
            TEST DL, DL
        JNZ STRCATLOOP
        RET
    strglue ENDP
END
\$\endgroup\$
5
\$\begingroup\$

This strikes me as looking a little like code I'd expect to see from a compiler--code that works, but completely ignores both traditional x86 register usage, and the special instructions the x86 provides for the job.

Traditionally, you'd use edi to point to your destination string, and esi to point to your source string. You'd use a repnz scasb to find the end of the first string, and lodsb/stosb to copy bytes from the source to the destination (there is actually a movsb to copy bytes as well, but it doesn't allow you to check for the end of the string as you copy).

Note that this depends on the direction flag being 0, but every compiler of which I'm aware leaves it that way (and depend on your leaving it that way as well).

    xor eax, eax     ; set al to value we're looking for, 0 in this case.
    lea ecx, [eax-1] ; set ecx to 0ffffffffh

    mov edi, [esp+4] ; find end of dest string
    repnz scasb

    mov esi, [esp+8] ; we've found the end of the destination, now do the copy
    dec edi

copy_loop:
    lodsb
    stosb
    test al, al
    jnz  copy_loop
    ret

This is somewhat shorter. Under normal circumstances, we can expect either one to be memory bound, so speed probably isn't particularly relevant (but if we did care, it would depend on the exact processor in use--this would tend to work better on older processors, but would be about the same to possibly even a little slower on newer processors).

As far are readability goes, either one is fairly easy to read, but I think my code fits more closely with how somebody with experience writing code for an x86 would expect to see the registers used. As far as indentation goes, there doesn't seem to be much agreement about whether to indent assembly language. Your indentation certainly doesn't hurt anything, but it isn't like higher level languages where it's expected to the point that unindented code is clearly a problem. The way I've written the code above (instructions indented one stop, label(s) at the left margin) is almost certainly more common.

I should probably add that I'm a bit hesitant to post on this subject--while I think I wrote assembly language quite well at one time, it's been years since I wrote enough to notice. I do look forward to @Edward posting a review on this (though I'm not sure whether he writes much assembly code any more either).

\$\endgroup\$
  • \$\begingroup\$ Thanks for this review and advice! To be clear, the majority of my knowledge of x86 assembly language comes from looking at disassembled object files from high level languages. \$\endgroup\$ – Govind Parmar Apr 23 '16 at 18:57
  • \$\begingroup\$ Your code snippet has an error. When repnz scasb has found the null-terminator it leaves the EDI register past this address. Since the aim is to replace this null-byte with the first character from the source string, you need to add an dec edi instruction before going in the copy_loop. \$\endgroup\$ – Sep Roland May 22 '16 at 20:43
  • \$\begingroup\$ @SepRoland: Oops--well, at least I included a disclaimer that it's been years since I did this much... :-) \$\endgroup\$ – Jerry Coffin May 22 '16 at 21:00
2
\$\begingroup\$

The code looks ok. But there's instructions, movsb and rep, which basically can replace your second step. You could do use [esp + 8] instead in and do this after your first step:

sub eax, dword ptr [esp + 8]   ;get the length of the string
mov ecx, eax                   ;move the length of the string into ecx
mov esi, dword ptr [esp + 8]   ;move the source address into esi

You would use the original step 1 again to get the null-terminator of the destination and do this:

mov edi, eax                   ;move address of the destination null-terminator to edi, you could of course also replace eax with edi in your step 1
inc ecx                            ;don't forget to copy the null-terminator
rep movsb                      ;movsb: move the value at esi to the address at edi, increment esi and edi
                               ;rep:   decrement ecx and do the following instruction again if nonzero

For the documentation, look up the calling conventions. You could note what calling convention you are using, but other than that you do not need to explicitly note that eax, ecx and edx are used because they are parameters in most conventions. Other than that I think the documentation is not too verbose nor too little, except for the last comment.

Also note that you are using [esp + 4] and [esp + 8] as parameters, it would be better to follow the conventions and use ecx and edx or the stack instead. Also you should push the pointer to the data to the stack rather than return it in a fixed address.

Indentation is more a personal thing, except for a few things like don't use hard tabs (and you aren't using them so that's good). Most of the times a indentation with of 4 spaces is used. The only thing is that I personally leave the jump instruction at the end of a loop indented as well.

\$\endgroup\$
  • \$\begingroup\$ Your answer has an error. The calculation brings the length of the source string in ECX. If later you glue this many bytes to the destination string, the result will not be zero-terminated! You need to add an inc ecx instruction before doing the rep movsb. \$\endgroup\$ – Sep Roland May 22 '16 at 20:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.