Bug in StrPCopy() also

dougwoodrow · 2013-01-18 20:47:40

The version of the StrPCopy function in SysUtils.pas has the same bug I reported for the StrLCopy function - it assumes the destination buffer already has a null terminator, and does not add one:

function StrPCopy(Dest: PChar; const Source: string): PChar;
asm // faster version by AB
    or edx,edx
    push eax
    xchg eax,edx
    jz @z
    mov ecx,[eax-4]
    inc ecx // copy last #0
    call move
@z: pop eax
end;

The solution is to correct the assembler above, or simply revert to the original Borland version of the function:

function StrPCopy(Dest: PChar; const Source: string): PChar;
begin
  Result := StrLCopy(Dest, PChar(Source), Length(Source));
end;

NB of course making sure you use the corrected version of StrLCopy I supplied in my last topic about StrLCopy().

ab · 2013-01-19 10:58:42

Thanks for the report.

I've added it to the direct unofficial link - http://synopse.info/files/SynopseRTLsources.zip

dougwoodrow · 2013-01-20 17:48:55

I notice you haven't updated StrLCopy though - have you tried running my test application illustrating the problem?

Incidentally, what is the purpose of the "assembler;" directive on the original Borland version of StrLCopy?

ab · 2013-01-20 18:02:55

"assembler" is a deprecated, and ignored keyword.

I think StrLCopy() as included in the zip should be fixed, isn't it?
Perhaps not.

dougwoodrow · 2013-01-20 18:09:49

ab wrote:

"assembler" is a deprecated, and ignored keyword.

Ah, thanks!

I think StrLCopy() as included in the zip should be fixed, isn't it?

It looks the same to me; it stops the copy at the first null in the source, but does not set a null value at the end of the destination buffer:

function StrLCopy(Dest: PChar; const Source: PChar; MaxLen: Cardinal): PChar;
asm // faster version by AB
    or edx,edx
    jz @z
    push eax
    push ebx
    xchg eax,edx
    mov ebx,ecx
    xor ecx,ecx
@1: cmp byte ptr [eax+ecx],0
    lea ecx,ecx+1 // copy last #0
    je @s
    cmp ecx,ebx
    jb @1
@s: pop ebx
    call Move
    pop eax
@z:
end;

ab · 2013-01-20 19:17:54

This version should work as you expect, I suppose:

function StrLCopy(Dest: PChar; const Source: PChar; MaxLen: Cardinal): PChar;
asm // faster version by AB: eax=Dest Source=edx MaxLen=ecx
    or edx,edx
    jz @z
    push ebx
    push esi
    xor esi,esi
@1: mov bl,[edx+esi]
    or  bl,bl
    mov [eax+esi],bl
    jz  @s
    lea esi,esi+1
    cmp esi,ecx
    jb @1
    mov byte ptr [eax+esi],0
@s: pop esi
    pop ebx
@z:
end;

dougwoodrow · 2013-01-20 23:30:26

This version should work as you expect, I suppose

Much better! But it's not correct when MaxLen is 0.

I wonder how this one compares, in terms of speed, with the version calling the Move routine? Probably depends on the size of the data being copied.

ab · 2013-01-21 08:51:08

OK, here it handles MaxLen=0.

function StrLCopy(Dest: PChar; const Source: PChar; MaxLen: Cardinal): PChar;
asm // faster version by AB: eax=Dest Source=edx MaxLen=ecx
    or edx,edx
    jz @z
    push ebx
    push esi
    xor esi,esi
    or ecx,ecx
    jz @2
@1: mov bl,[edx+esi]
    or  bl,bl
    mov [eax+esi],bl
    jz  @s
    lea esi,esi+1
    cmp esi,ecx
    jb @1
@2: mov byte ptr [eax+esi],0
@s: pop esi
    pop ebx
@z:
end;

About speed, I would not be surprised if this one will be faster, since it copies the content in one pass, whereas the previous version first checked the length (inlined StrLen code) then moved the data.
For small amount of text, it may not make big changes, but for big amount of text, it may make the difference.

dougwoodrow · 2013-01-21 10:03:05

Thank you Arnaud!
Although I assume John O'Harrow's version of Move() is highly optimised; do you think there is something wrong with my proposed solution?

function StrLCopy(Dest: PChar; const Source: PChar; MaxLen: Cardinal): PChar;
asm
    or edx,edx                // check Source
    jz @z                     // abort if nil
    mov byte ptr [eax+ecx],0  // null-terminate Dest
    push eax                  // save Dest
    xchg eax,edx              // swap Source & Dest for Move()
    call Move                 // do the move
    pop eax                   // restore Dest
@z:
end;

ab · 2013-01-21 10:20:17

You do not check the source end (searching any ending #0), which was the case in the original code (using StrLen ,if I remember well).

dougwoodrow · 2013-01-21 10:58:04

ab wrote:

You do not check the source end.

Ah, of course, thanks! I didn't pay attention to "at most" in the Delphi Help description of the function.

StrLCopy copies at most MaxLen characters from Source to Dest, then adds a null terminator to Dest and returns Dest.

mORMot Open Source

#1 2013-01-18 20:47:40

Bug in StrPCopy() also

#2 2013-01-19 10:58:42

Re: Bug in StrPCopy() also

#3 2013-01-20 17:48:55

Re: Bug in StrPCopy() also

#4 2013-01-20 18:02:55

Re: Bug in StrPCopy() also

#5 2013-01-20 18:09:49

Re: Bug in StrPCopy() also

#6 2013-01-20 19:17:54

Re: Bug in StrPCopy() also

#7 2013-01-20 23:30:26

Re: Bug in StrPCopy() also

#8 2013-01-21 08:51:08

Re: Bug in StrPCopy() also

#9 2013-01-21 10:03:05

Re: Bug in StrPCopy() also

#10 2013-01-21 10:20:17

Re: Bug in StrPCopy() also

#11 2013-01-21 10:58:04

Re: Bug in StrPCopy() also

Board footer