3

I have the following function that works in Delphi 2006, but under Delphi XE2 it gives either an access violation error or a privileged instruction error when processing RET.

function Q_TrimChar(const S: string; Ch: Char): string;
asm
        PUSH    ESI
        MOV     ESI,ECX
        TEST    EAX,EAX
        JE      @@qt
        MOV     ECX,[EAX-4]
        TEST    ECX,ECX
        JE      @@qt
        PUSH    EBX
        PUSH    EDI
        MOV     EBX,EAX
        MOV     EDI,EDX
        XOR     EDX,EDX
        MOV     EAX,ESI
        CALL    System.@LStrFromPCharLen
        MOV     EDX,EDI
        MOV     ECX,[EBX-4]
@@lp1:  CMP     DL,BYTE PTR [EBX]
        JNE     @@ex1
        INC     EBX
        DEC     ECX
        JNE     @@lp1
        MOV     EDX,[ESI]
        JMP     @@wq
@@ex1:  DEC     ECX
@@lp2:  CMP     DL,BYTE PTR [EBX+ECX]
        JNE     @@ex2
        DEC     ECX
        JMP     @@lp2
@@ex2:  MOV     EDI,[ESI]
        LEA     EDX,[EDI+ECX+1]
@@lp3:  MOV     AL,BYTE PTR [EBX+ECX]
        MOV     BYTE PTR [EDI+ECX],AL
        DEC     ECX
        JNS     @@lp3
@@wq:   MOV     EAX,[ESI]
        MOV     BYTE PTR [EDX],0
        SUB     EDX,EAX
        MOV     [EAX-4],EDX
        POP     EDI
        POP     EBX
        POP     ESI
        RET
@@qt:   MOV     EAX,ESI
        CALL    System.@LStrClr
        POP     ESI
end;

I don't know assembly very well. What is the problem?

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
There is no spoon
  • 1,775
  • 2
  • 22
  • 53
  • 1
    By the way, this asm code is just awfully coded. For instance, if the s string is only one char of Ch it will make an AV, IMHO. Forget about it, and use the 2nd pascal version supplied by Mike in his answer. – Arnaud Bouchez Feb 17 '12 at 08:22
  • 1
    I voted to close because this is so narrowly defined. Even though it's totally impressive that someone seems to have definitively answered it, the value of this kind of question to the site is like zero. – Warren P Feb 18 '12 at 00:12

2 Answers2

17

I completely agree with David's suggestion to simply code this in Pascal and have upvoted that answer. Unless profiling has indicated that this is a true bottleneck then there's really no need for the ASM. Here are two versions. The first is easier to read but the second is more efficient:

function Q_TrimChar(const S: string; Ch: Char): string;
begin
  result := S;
  while (result <> '') and (result[1] = Ch) do Delete(Result, 1, 1);
  while (result <> '') and (result[Length(Result)] = Ch) do Delete(Result, Length(Result), 1);
end;

function Q_TrimChar(const S: string; Ch: Char): string;
var
  First, Last : integer;
begin
  First := 1;
  Last := Length(S);
  while (First < Last) and (S[First] = Ch) do inc(First);
  while (Last >= First) and (S[Last] = Ch) do Dec(Last);
  Result := copy(S, First, Last-First+1);
end;
Mike W
  • 1,276
  • 8
  • 10
  • 1
    +1 well done for working what the code actually did. The second version is better. The obvious optimisation to that is not to call copy when First=1 and Last=Length(S). – David Heffernan Feb 17 '12 at 07:45
  • +1. Good compiler-generated string manipulation routines are hard to beat, especially in such simple cases. I don't think the original assembler is faster then the 2nd pascal variant provided here! – Cosmin Prund Feb 17 '12 at 10:01
  • @CosminPrund You are right, the 2nd version is less buggy than the original asm version, and will perform the same - even perhaps better since IMHO the asm version was buggy (e.g. if `S=Ch`). For a true optimized asm version of a close algorithm see `function Trim(const S: RawUTF8): RawUTF8;` in both asm and pascal in [this unit](http://synopse.info/fossil/finfo?name=SynCommons.pas) - I've used John O'Harrow's assembler, modified for Delphi 2009/2010/XE/XE2. – Arnaud Bouchez Feb 17 '12 at 12:26
6

Delphi 2006 uses single byte ANSI characters and so string is AnsiString, Char is AnsiChar. On Delphi 2009 and later, two byte Unicode characters are used. This function cannot possibly work on both compilers.

Even the standard hack of using AnsiString and AnsiChar does not work. Most likely the assumptions that this function makes about the RTL implementation are no longer valid in modern Delphi.

I would re-write this function in Pascal and let the compiler do the work. Not only will that be the quickest way to solve your current problem, it will also get you over the hurdle of 64-bit compilation should you ever choose to tackle that.

David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
  • 2
    The main issue in the supplied code was definitively not about string/AnsiString problem, but the whole asm block itself, which won't work at all in x64, due to diverse register layout *and* new signature of the `LStrFromPCharLen` APIs since Delphi 2009. – Arnaud Bouchez Feb 17 '12 at 12:17
  • 1
    @Arnaud I find that criticism rather weak. x64 is beside the point. OP is compiling 32 bit for sure. That code does not work in 32 bit XE2. Nor does it work in 32 bit 2010 or XE. Plus I commented on the x64 bit so I had that covered. And as for the main issue, I don't buy that at all. Any single problem with the code means it won't work. So there are multiple defects with the code. If an answer fails to identify every single problem does that make it worthy of a downvote? The blindingly obvious conclusion is that Pascal conversion is needed. No need to over-analyse the broken code. – David Heffernan Feb 17 '12 at 13:04
  • The main issue of the code, despite `AnsiString` use, is about `LStrFromPCharLen` API change - it expect the code page since Delphi 2009. – Arnaud Bouchez Feb 17 '12 at 15:02
  • @Arnaud So you think that if that change had not been made, then the asm would be fine. And that it would magically handle two byte chars? – David Heffernan Feb 17 '12 at 15:04