View Issue Details

IDProjectCategoryView StatusLast Update
0034409FPCCompilerpublic2019-12-02 12:24
ReporterMarģersAssigned ToFlorian 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionreopened 
Platformx86_64OSlinuxOS Version
Product Version3.2.0Product Build 
Target Version3.3.1Fixed in Version3.3.1 
Summary0034409: [regresion] mov ax, w - generate invalid assembler code
Descriptionw : word;

version 3.2.0
mov ax, w - generate: movw %edi,%ax

Error: Error while assembling exitcode 1

version 3.0.0
mov ax, w - generate: movw %di,%ax
Steps To Reproducecompile following code with parameters: -Rintel -O4 -a

function foo ( w : word):byte; assembler;
asm
     mov ax, w

end;

begin
     foo(3);
end.
TagsNo tags attached.
Fixed in Revision43617
FPCOldBugId
FPCTarget-
Attached Files
  • i34409.patch (5,149 bytes)
    Index: compiler/llvm/nllvmbas.pas
    ===================================================================
    --- compiler/llvm/nllvmbas.pas	(revision 40012)
    +++ compiler/llvm/nllvmbas.pas	(working copy)
    @@ -41,7 +41,7 @@
              fsymboldata: tfplist;
              function getllvmasmopindexforsym(sym: tabstractnormalvarsym): longint;
              function getllvmasmparasym(sym: tabstractnormalvarsym): tasmsymbol;
    -         procedure ResolveRef(const filepos: tfileposinfo; var op: toper); override;
    +         procedure ResolveRef(const filepos: tfileposinfo; var op: toper; OpSize: TOpSize = S_NO); override;
             public
              constructor create(p : TAsmList); override;
              destructor destroy; override;
    @@ -117,7 +117,7 @@
           end;
     
     
    -    procedure tllvmasmnode.ResolveRef(const filepos: tfileposinfo; var op: toper);
    +    procedure tllvmasmnode.ResolveRef(const filepos: tfileposinfo; var op: toper; OpSize: TOpSize);
           var
             sym: tabstractnormalvarsym;
             ref: treference;
    Index: compiler/ncgbas.pas
    ===================================================================
    --- compiler/ncgbas.pas	(revision 40012)
    +++ compiler/ncgbas.pas	(working copy)
    @@ -38,7 +38,7 @@
     
            tcgasmnode = class(tasmnode)
              protected
    -          procedure ResolveRef(const filepos: tfileposinfo; var op:toper); virtual;
    +          procedure ResolveRef(const filepos: tfileposinfo; var op:toper; OpSize: TOpSize = S_NO); virtual;
              public
               procedure pass_generate_code;override;
            end;
    @@ -123,7 +123,7 @@
     *****************************************************************************}
     
     
    -    procedure tcgasmnode.ResolveRef(const filepos: tfileposinfo; var op:toper);
    +    procedure tcgasmnode.ResolveRef(const filepos: tfileposinfo; var op:toper; OpSize: TOpSize);
           var
             sym : tabstractnormalvarsym;
     {$ifdef x86}
    @@ -133,6 +133,7 @@
             forceref,
             getoffset : boolean;
             indexreg : tregister;
    +        subreg : tsubregister;
             sofs : longint;
           begin
             if (op.typ=top_local) then
    @@ -210,8 +211,44 @@
                       else
                         begin
                           op.typ:=top_reg;
    -                      op.reg:=sym.localloc.register;
     
    +                      { i34409 - make sure the right sub-register is used in,
    +                        for example, raw "MOV AX, param" assembler instructions. [Kit] }
    +
    +                      if (OpSize in [S_NO, S_B, S_W, S_L, S_Q]) then
    +                        begin
    +                          case OpSize of
    +                            S_NO:
    +                              { Try to determine the correct register to use
    +                                (selecting one that's too large could cause a
    +                                buffer overflow when writing to a reference). [Kit] }
    +                              begin
    +                                case tcgsize2size[sym.localloc.size] of
    +                                  1: subreg := R_SUBL;
    +                                  2: subreg := R_SUBW;
    +                                  4: subreg := R_SUBD;
    +                                  8: subreg := R_SUBQ;
    +                                  else
    +                                    subreg := getsubreg(sym.localloc.register);
    +                                end;
    +                              end;
    +                            S_B: subreg := R_SUBL;
    +                            S_W: subreg := R_SUBW;
    +                            S_L: subreg := R_SUBD;
    +                            S_Q: subreg := R_SUBQ;
    +                            else
    +                              { Should never reach here }
    +                              InternalError(2018102400);
    +                          end;
    +
    +                          op.reg := newreg(
    +                            R_INTREGISTER,
    +                            getsupreg(sym.localloc.register),
    +                            subreg
    +                          )
    +                        end
    +                      else
    +                        op.reg := sym.localloc.register;
     {$ifdef avr}
                           case sofs of
                             1: op.reg:=cg.GetNextReg(op.reg);
    @@ -320,7 +357,7 @@
                            { fixup the references }
                            for i:=1 to taicpu(hp2).ops do
                             begin
    -                          ResolveRef(taicpu(hp2).fileinfo,taicpu(hp2).oper[i-1]^);
    +                          ResolveRef(taicpu(hp2).fileinfo,taicpu(hp2).oper[i-1]^, taicpu(hp2).opsize);
                               with taicpu(hp2).oper[i-1]^ do
                                begin
                                  case typ of
    @@ -369,7 +406,7 @@
                            { fixup the references }
                            for i:=1 to taicpu(hp).ops do
                              begin
    -                           ResolveRef(taicpu(hp).fileinfo,taicpu(hp).oper[i-1]^);
    +                           ResolveRef(taicpu(hp).fileinfo,taicpu(hp).oper[i-1]^, taicpu(hp).opsize);
     {$ifdef x86}
                                with taicpu(hp).oper[i-1]^ do
                                  begin
    
    i34409.patch (5,149 bytes)

Activities

Marģers

2018-10-12 16:06

reporter   ~0111383

version 3.3.1 Error while assembling
version 3.1.1 (2018/3/13) Error while assembling
version 3.0.4 Ok

look like this error is around for some time

J. Gareth Moreton

2018-10-13 13:27

developer   ~0111388

Last edited: 2018-10-13 13:32

View 3 revisions

I should be able to fix this one, given Intel assembly is my forte.

This bug does not seem to occur under win64. Compiling the code produces the expected "mov %cx,%ax" output.

J. Gareth Moreton

2018-10-24 06:33

developer   ~0111532

So the problem DOES actually occur under win64 when using the latest source:

.section .text.n_p$testfile_$$_foo$word$$byte,"x"
    .balign 16,0x90
.globl P$TESTFILE_$$_FOO$WORD$$BYTE
P$TESTFILE_$$_FOO$WORD$$BYTE:
.Lc1:
.seh_proc P$TESTFILE_$$_FOO$WORD$$BYTE
    pushq %rbp
.seh_pushreg %rbp
.Lc3:
.Lc4:
    movq %rsp,%rbp
.Lc5:
    leaq -16(%rsp),%rsp
.seh_stackalloc 16
.seh_endprologue
# CPU ATHLON64
    movw %ecx,%ax <--- ERROR!
# CPU ATHLON64
    leaq (%rbp),%rsp
    popq %rbp
    ret
.seh_endproc
.Lc2:

Watch this space!

J. Gareth Moreton

2018-10-25 05:03

developer  

i34409.patch (5,149 bytes)
Index: compiler/llvm/nllvmbas.pas
===================================================================
--- compiler/llvm/nllvmbas.pas	(revision 40012)
+++ compiler/llvm/nllvmbas.pas	(working copy)
@@ -41,7 +41,7 @@
          fsymboldata: tfplist;
          function getllvmasmopindexforsym(sym: tabstractnormalvarsym): longint;
          function getllvmasmparasym(sym: tabstractnormalvarsym): tasmsymbol;
-         procedure ResolveRef(const filepos: tfileposinfo; var op: toper); override;
+         procedure ResolveRef(const filepos: tfileposinfo; var op: toper; OpSize: TOpSize = S_NO); override;
         public
          constructor create(p : TAsmList); override;
          destructor destroy; override;
@@ -117,7 +117,7 @@
       end;
 
 
-    procedure tllvmasmnode.ResolveRef(const filepos: tfileposinfo; var op: toper);
+    procedure tllvmasmnode.ResolveRef(const filepos: tfileposinfo; var op: toper; OpSize: TOpSize);
       var
         sym: tabstractnormalvarsym;
         ref: treference;
Index: compiler/ncgbas.pas
===================================================================
--- compiler/ncgbas.pas	(revision 40012)
+++ compiler/ncgbas.pas	(working copy)
@@ -38,7 +38,7 @@
 
        tcgasmnode = class(tasmnode)
          protected
-          procedure ResolveRef(const filepos: tfileposinfo; var op:toper); virtual;
+          procedure ResolveRef(const filepos: tfileposinfo; var op:toper; OpSize: TOpSize = S_NO); virtual;
          public
           procedure pass_generate_code;override;
        end;
@@ -123,7 +123,7 @@
 *****************************************************************************}
 
 
-    procedure tcgasmnode.ResolveRef(const filepos: tfileposinfo; var op:toper);
+    procedure tcgasmnode.ResolveRef(const filepos: tfileposinfo; var op:toper; OpSize: TOpSize);
       var
         sym : tabstractnormalvarsym;
 {$ifdef x86}
@@ -133,6 +133,7 @@
         forceref,
         getoffset : boolean;
         indexreg : tregister;
+        subreg : tsubregister;
         sofs : longint;
       begin
         if (op.typ=top_local) then
@@ -210,8 +211,44 @@
                   else
                     begin
                       op.typ:=top_reg;
-                      op.reg:=sym.localloc.register;
 
+                      { i34409 - make sure the right sub-register is used in,
+                        for example, raw "MOV AX, param" assembler instructions. [Kit] }
+
+                      if (OpSize in [S_NO, S_B, S_W, S_L, S_Q]) then
+                        begin
+                          case OpSize of
+                            S_NO:
+                              { Try to determine the correct register to use
+                                (selecting one that's too large could cause a
+                                buffer overflow when writing to a reference). [Kit] }
+                              begin
+                                case tcgsize2size[sym.localloc.size] of
+                                  1: subreg := R_SUBL;
+                                  2: subreg := R_SUBW;
+                                  4: subreg := R_SUBD;
+                                  8: subreg := R_SUBQ;
+                                  else
+                                    subreg := getsubreg(sym.localloc.register);
+                                end;
+                              end;
+                            S_B: subreg := R_SUBL;
+                            S_W: subreg := R_SUBW;
+                            S_L: subreg := R_SUBD;
+                            S_Q: subreg := R_SUBQ;
+                            else
+                              { Should never reach here }
+                              InternalError(2018102400);
+                          end;
+
+                          op.reg := newreg(
+                            R_INTREGISTER,
+                            getsupreg(sym.localloc.register),
+                            subreg
+                          )
+                        end
+                      else
+                        op.reg := sym.localloc.register;
 {$ifdef avr}
                       case sofs of
                         1: op.reg:=cg.GetNextReg(op.reg);
@@ -320,7 +357,7 @@
                        { fixup the references }
                        for i:=1 to taicpu(hp2).ops do
                         begin
-                          ResolveRef(taicpu(hp2).fileinfo,taicpu(hp2).oper[i-1]^);
+                          ResolveRef(taicpu(hp2).fileinfo,taicpu(hp2).oper[i-1]^, taicpu(hp2).opsize);
                           with taicpu(hp2).oper[i-1]^ do
                            begin
                              case typ of
@@ -369,7 +406,7 @@
                        { fixup the references }
                        for i:=1 to taicpu(hp).ops do
                          begin
-                           ResolveRef(taicpu(hp).fileinfo,taicpu(hp).oper[i-1]^);
+                           ResolveRef(taicpu(hp).fileinfo,taicpu(hp).oper[i-1]^, taicpu(hp).opsize);
 {$ifdef x86}
                            with taicpu(hp).oper[i-1]^ do
                              begin
i34409.patch (5,149 bytes)

J. Gareth Moreton

2018-10-25 05:06

developer   ~0111544

This is probably not the cleanest way to do it, and I'm not sure if it doesn't introduce unforeseen side-effects, but I have added an extra parameter to the "ResolveRef" routine, namely the operation size. I'm guessing the compiler sets the reference register to 32 bits for the sake of performance reasons, so setting it to a smaller sub-register might cause problems elsewhere.

In the meantime, find attached the patch with the proposed fix. It fixes the issue under Windows and Linux, and the compiler builds and seems to perform as it should. Some extensive testing might be required though.

J. Gareth Moreton

2018-10-25 10:12

developer   ~0111552

Set to "feedback" since the patch hasn't actually been applied yet.

Florian

2019-11-30 21:40

administrator   ~0119562

I have chosen a shorter approach.

Marģers

2019-12-02 12:24

reporter   ~0119580

Thanks

Issue History

Date Modified Username Field Change
2018-10-12 13:56 Marģers New Issue
2018-10-12 16:06 Marģers Note Added: 0111383
2018-10-13 13:27 J. Gareth Moreton Note Added: 0111388
2018-10-13 13:28 J. Gareth Moreton Assigned To => J. Gareth Moreton
2018-10-13 13:28 J. Gareth Moreton Status new => assigned
2018-10-13 13:28 J. Gareth Moreton Note Edited: 0111388 View Revisions
2018-10-13 13:32 J. Gareth Moreton Note Edited: 0111388 View Revisions
2018-10-24 06:33 J. Gareth Moreton Note Added: 0111532
2018-10-25 05:03 J. Gareth Moreton File Added: i34409.patch
2018-10-25 05:06 J. Gareth Moreton Note Added: 0111544
2018-10-25 05:06 J. Gareth Moreton Status assigned => resolved
2018-10-25 05:06 J. Gareth Moreton Fixed in Version => 3.3.1
2018-10-25 05:06 J. Gareth Moreton Resolution open => fixed
2018-10-25 05:06 J. Gareth Moreton Assigned To J. Gareth Moreton => Florian
2018-10-25 10:12 J. Gareth Moreton Note Added: 0111552
2018-10-25 10:12 J. Gareth Moreton Status resolved => feedback
2018-10-25 10:12 J. Gareth Moreton Resolution fixed => reopened
2018-10-25 10:12 J. Gareth Moreton Target Version => 3.3.1
2019-11-30 21:40 Florian Status feedback => resolved
2019-11-30 21:40 Florian Fixed in Revision => 43617
2019-11-30 21:40 Florian FPCTarget => -
2019-11-30 21:40 Florian Note Added: 0119562
2019-12-02 12:24 Marģers Status resolved => closed
2019-12-02 12:24 Marģers Note Added: 0119580