Assembly - Trouble checking a string for non-numbers

Question

I'm trying to make a simple program that loops through a string and increments a register if any character isn't the in characters 0-9.

There are no syntax errors or anything, but the output is sporadic and changes each time I run it. So basically, it doesn't work because it doesn't output what is expected.

I'm using the 'asm' functionality of Delphi to write this, so variables are defined outside of the ASM code.

This is my code as I have it:

function checkInteger(numIn : String) : Boolean;
var isValid : Boolean;
  lengthOfString,incorrectCharacters,count: integer;
  buffer: integer;
begin

lengthOfString := length(numIn);
buffer := 0;

//BEGIN ASSEMBLY

asm
    xor edx,edx
    lea ecx,[numIn]
    mov [count],ecx

    @next:
    mov ecx,[count]
    movzx eax,byte[ecx]


    inc ecx
    mov [count],ecx

    cmp eax,'0'
    jb @increase_incorrect
    cmp eax,'9'
    ja @increase_incorrect

    jmp @skip

    @increase_incorrect:
    inc edx

    @skip:

    mov ecx,[lengthOfString]
    sub ecx,1
    mov [lengthOfString],ecx
    cmp ecx,0
    jg @next
    mov [incorrectCharacters],edx

end;

//END ASSEMBLY

if(incorrectCharacters > 0) then begin
  isValid := true;
end else begin
  isValid := false;
end;

checkInteger := isValid;
end;

The 'numIn' variable is just a user entered string containing any valid characters. As a note, the string will never be empty as I check for that earlier in the program.

Answer 1

I decided to throw my solution which takes full advantage of using ESI register in:

function HasNonDigits(const S: AnsiString): Boolean;
asm
        push    esi

        { ecx <- LStrLen(S) }
        mov     ecx, dword ptr [S-4]

        lea     esi, [S]
        cld               { for illustrative purpose, see the note }
@loop:
        lodsb             { al <- [esi]; esi <- esi+1; ecx <- ecx-1 }
        cmp     al, '0'
        jl      @true
        cmp     al, '9'
        jg      @true
        loop    @loop

        mov     al, 0     { Ord(False) } 
        jmp     @endp

@true:
        mov     al, 1     { Ord(True) }
@endp:
        pop     esi
end;

procedure TForm1.FormCreate(Sender: TObject);
const
  Tests: array[0..5] of AnsiString = (
    '12345',
    'foo',
    '700',
    'barbar',
    'streisand',
    'f33r'
  );
var
  I: Integer;
begin
  for I := Low(Tests) to High(Tests) do
    OutputDebugString(PChar(Format('%s -> %s', [
      Tests[I],
      BoolToStr(HasNonDigits(Tests[I]), True)
    ])));
end;

To recap, inline assembler code HAVE to comply with the following conventions:

Procedures and functions must preserve the EBX, ESI, EDI, and EBP registers, but can modify the EAX, EDX, and ECX registers. When implementing a constructor or destructor in assembler, be sure to preserve the DL register. Procedures and functions are invoked with the assumption that the CPU's direction flag is cleared (corresponding to a CLD instruction) and must return with the direction flag cleared.

Answer 2

Right off the bat, this is wrong:

lea ecx,[numIn]

That loads the address of the local variable numIn into the esi register. But you want the address of the buffer.

mov ecx,numIn

Of course, you'd need to add some code to deal with the empty string, for which numIn is nil.

And I think it is asking for trouble to mix Pascal and asm in one function. The compiler doesn't know what your asm code is doing to the registers, and vice versa. Don't mix Pascal and asm like that.

---

I wonder what this code is trying to achieve:

if(incorrectCharacters > 0) then begin
  isValid := true;
end else begin
  isValid := false;
end;

checkInteger := isValid;

Don't you mean:

checkInteger := incorrectCharacters > 0;

Do that and remove the isValid variable.

---

If you want the code to perform well, you should exit as soon as you find one invalid character. It is pointless to count how many there are. Once you've found one, you need to bail out.

I'd write it like this:

function checkInteger(const numIn : AnsiString) : Boolean;
var
  i: Integer;
begin
  for i := 1 to Length(numIn) do
    if (numIn[i]<'0') or (numIn[i]>'9') then
    begin
      Result := False;
      exit;
    end;
  Result := True;
end;

The compiler turns that into the following:

0041A150 56               push esi
0041A151 8BF0             mov esi,eax
0041A153 8BD6             mov edx,esi
0041A155 85D2             test edx,edx
0041A157 7405             jz $0041a15e
0041A159 83EA04           sub edx,$04
0041A15C 8B12             mov edx,[edx]
0041A15E 8BCA             mov ecx,edx
0041A160 85C9             test ecx,ecx
0041A162 7E1A             jle $0041a17e
0041A164 BA01000000       mov edx,$00000001
0041A169 0FB64416FF       movzx eax,[esi+edx-$01]
0041A16E 3C30             cmp al,$30
0041A170 7204             jb $0041a176
0041A172 3C39             cmp al,$39
0041A174 7604             jbe $0041a17a
0041A176 33C0             xor eax,eax
0041A178 5E               pop esi
0041A179 C3               ret 

That's a lot better than what you managed. A good exercise would be to start from here and try to improve it.