View Issue Details

IDProjectCategoryView StatusLast Update
0037620FPCCompilerpublic2020-08-22 18:05
ReporterChristo Crause Assigned To 
PrioritynormalSeverityminorReproducibilityN/A
Status newResolutionopen 
Product Version3.3.1 
Summary0037620: Xtensa [patch] Modified call0 stack frame layout to simplify stack tracing
DescriptionThe current layout of the call0 stack frame makes it difficult to locate the previous frame pointer because it is not stored at a fixed offset in the stack frame. The attached patch changes the call0 stack frame so that saved registers are stored at negative offsets from the frame pointer (a15). This way the offsets are not dependent on the storage size of the params/temps area. The proposed layout is shown below in Additional Information. It is somewhat similar to the windowed layout, with the exception that all registers are stored below the frame pointer.

The stack helper functions in rtl/xtensa/xtensa.inc are updated for call0 and the new layout.

Note that the patch now always store register a0 - this is basically done for two reasons:
- to help with stack frame tracing
- to help solve 0036931 by creating a free register that can be clobbered when required
Additional Information-------- Top of frame ---------
temps/params/return
.
. <- FP (a15)
-------- Reg save area --------
a15 (caller FP)
a0 (return address)
other regs
.
. <- SP (a1)
--------- Below frame ---------
TagsNo tags attached.
Fixed in Revision
FPCOldBugId
FPCTarget
Attached Files

Activities

Christo Crause

2020-08-22 14:26

reporter  

call0-stackframe.patch (14,998 bytes)   
Index: compiler/xtensa/cgcpu.pas
===================================================================
--- compiler/xtensa/cgcpu.pas	(revision 46541)
+++ compiler/xtensa/cgcpu.pas	(working copy)
@@ -649,33 +649,60 @@
             case target_info.abi of
               abi_xtensa_call0:
                 begin
-                  if current_procinfo.framepointer<>NR_STACK_POINTER_REG then
-                    Include(regs,RS_A15);
-                  if pi_do_call in current_procinfo.flags then
-                    Include(regs,RS_A0);
+                { Configure stack as follows:
+
+                  -------- Top of frame ---------
+                  temps/params/return
+                  .
+                  .                         <- FP (a15)
+                  -------- Reg save area --------
+                  a15 (caller FP)
+                  a0  (return address)
+                  other regs
+                  .
+                  .                         <- SP (a1)
+                  --------- Below frame --------- }
+
+                  if (localsize<>0) then
+                    localsize:=align(localsize,current_settings.alignment.localalignmax);
+                  { Store caller FP (a15) at FP-4, return addresss (a0) at FP-8, followed by rest of regs }
+                  Include(regs,RS_A15);
+                  Include(regs,RS_A0);
+                  registerarea:=2*4;
                   if regs<>[] then
                      begin
-                       for r:=RS_A0 to RS_A15 do
+                       for r:=RS_A2 to RS_A14 do
                          if r in regs then
                            inc(registerarea,4);
+                       registerarea:=align(registerarea,current_settings.alignment.localalignmax);
                      end;
 
-                  inc(localsize,registerarea);
-                  if LocalSize<>0 then
+                  if (LocalSize<>0) or (registerarea <> 0) then
                     begin
-                      localsize:=align(localsize,current_settings.alignment.localalignmax);
-                      a_reg_alloc(list,NR_STACK_POINTER_REG);
-                      list.concat(taicpu.op_reg_reg_const(A_ADDI,NR_STACK_POINTER_REG,NR_STACK_POINTER_REG,-localsize));
+                      list.concat(taicpu.op_reg_reg_const(A_ADDI,NR_STACK_POINTER_REG,NR_STACK_POINTER_REG,-(localsize+registerarea)));
+                      list.concat(taicpu.op_reg_reg_const(A_S32i,NR_FRAME_POINTER_REG,NR_STACK_POINTER_REG,registerarea-4));
+                      list.concat(taicpu.op_reg_reg_const(A_S32i,NR_A0,NR_STACK_POINTER_REG,registerarea-8));
+                      list.concat(taicpu.op_reg_reg_const(A_ADDI,NR_FRAME_POINTER_REG,NR_STACK_POINTER_REG,registerarea));
+                      dec(registerarea, 2*4);
                     end;
+                  if regs<>[] then
+                    begin
+                      for r:=RS_A14 downto RS_A2 do
+                        if r in regs then
+                          begin
+                            dec(registerarea,4);
+                            list.concat(taicpu.op_reg_reg_const(A_S32I,newreg(R_INTREGISTER,r,R_SUBWHOLE),NR_STACK_POINTER_REG,registerarea));
+                          end;
+                    end;
 
                   reference_reset(ref,4,[]);
-                  ref.base:=NR_STACK_POINTER_REG;
+                  ref.base:=NR_FRAME_POINTER_REG;
                   ref.offset:=localsize;
                   if ref.offset>1024 then
                     begin
                       if ref.offset<=1024+32512 then
                         begin
-                          list.concat(taicpu.op_reg_reg_const(A_ADDMI,NR_A8,NR_STACK_POINTER_REG,ref.offset and $fffffc00));
+                          list.concat(taicpu.op_reg_reg_const(A_ADDMI,NR_A8,NR_FRAME_POINTER_REG,ref.offset and $fffffc00));
                           ref.offset:=ref.offset and $3ff;
                           ref.base:=NR_A8;
                         end
@@ -683,24 +710,6 @@
                         { fix me! }
                         Internalerror(2020031101);
                     end;
-
-                  if current_procinfo.framepointer<>NR_STACK_POINTER_REG then
-                    begin
-                      dec(ref.offset,4);
-                      list.concat(taicpu.op_reg_ref(A_S32I,NR_A15,ref));
-                      a_reg_alloc(list,NR_FRAME_POINTER_REG);
-                      list.concat(taicpu.op_reg_reg(A_MOV,NR_A15,NR_STACK_POINTER_REG));
-                    end;
-
-                  if regs<>[] then
-                    begin
-                      for r:=RS_A14 downto RS_A0 do
-                        if r in regs then
-                          begin
-                            dec(ref.offset,4);
-                            list.concat(taicpu.op_reg_ref(A_S32I,newreg(R_INTREGISTER,r,R_SUBWHOLE),ref));
-                          end;
-                    end;
                 end;
               abi_xtensa_windowed:
                 begin
@@ -757,7 +766,7 @@
          stackmisalignment : pint;
          regoffset : LongInt;
          stack_parameters : Boolean;
-         registerarea : PtrInt;
+         registersize, registeroffset : PtrInt;
          l : TAsmLabel;
          LocalSize: longint;
       begin
@@ -770,68 +779,43 @@
                 begin
                   LocalSize:=current_procinfo.calc_stackframe_size;
                   LocalSize:=align(LocalSize,4);
-                  stack_parameters:=current_procinfo.procdef.stack_tainting_parameter(calleeside);
-                  registerarea:=0;
+                  if LocalSize<>0 then
+                    localsize:=align(localsize,current_settings.alignment.localalignmax);
+
+                  registersize:=0;
                   regs:=rg[R_INTREGISTER].used_in_proc-paramanager.get_volatile_registers_int(pocall_stdcall);
-                  if current_procinfo.framepointer<>NR_STACK_POINTER_REG then
-                    Include(regs,RS_A15);
-                  if pi_do_call in current_procinfo.flags then
-                    Include(regs,RS_A0);
+                  Include(regs,RS_A15);
+                  Include(regs,RS_A0);
                   if regs<>[] then
                      begin
                        for r:=RS_A0 to RS_A15 do
                          if r in regs then
-                           inc(registerarea,4);
+                           inc(registersize,4);
+                       registersize:=align(registersize,current_settings.alignment.localalignmax);
                      end;
-                  inc(localsize,registerarea);
 
-                  if LocalSize<>0 then
+                  { Restore FP, PC }
+                  registeroffset:=registersize;
+                  dec(registeroffset,4);
+                  list.concat(taicpu.op_reg_reg_const(A_L32I,NR_A15,NR_STACK_POINTER_REG, registeroffset));
+                  dec(registeroffset,4);
+                  list.concat(taicpu.op_reg_reg_const(A_L32I,NR_A0,NR_STACK_POINTER_REG, registeroffset));
+                  { restore rest of registers }
+                  if regs<>[] then
                     begin
-                      localsize:=align(localsize,current_settings.alignment.localalignmax);
-                      // Determine reference mode required to access stack
-                      reference_reset(ref,4,[]);
-                      ref.base:=NR_STACK_POINTER_REG;
-                      ref.offset:=localsize;
-                      if ref.offset>1024 then
-                        begin
-                          if ref.offset<=1024+32512 then
-                            begin
-                              // allocation done in proc_entry
-                              //list.concat(taicpu.op_reg_reg_const(A_ADDMI,NR_A8,NR_STACK_POINTER_REG,ref.offset and $fffffc00));
-                              ref.offset:=ref.offset and $3ff;
-                              ref.base:=NR_A8;
-                            end
-                          else
-                            { fix me! }
-                            Internalerror(2020031102);
-                        end;
+                      for r:=RS_A14 downto RS_A2 do
+                        if r in regs then
+                          begin
+                            dec(registeroffset,4);
+                            list.concat(taicpu.op_reg_reg_const(A_L32I,newreg(R_INTREGISTER,r,R_SUBWHOLE),NR_STACK_POINTER_REG, registeroffset));
+                          end;
+                    end;
 
-                      // restore a15 if used
-                      if current_procinfo.framepointer<>NR_STACK_POINTER_REG then
-                        begin
-                          dec(ref.offset,4);
-                          list.concat(taicpu.op_reg_ref(A_L32I,NR_A15,ref));
-                          a_reg_dealloc(list,NR_FRAME_POINTER_REG);
-                        end;
-
-                      // restore rest of registers
-                      if regs<>[] then
-                        begin
-                          for r:=RS_A14 downto RS_A0 do
-                            if r in regs then
-                              begin
-                                dec(ref.offset,4);
-                                list.concat(taicpu.op_reg_ref(A_L32I,newreg(R_INTREGISTER,r,R_SUBWHOLE),ref));
-                              end;
-                        end;
-
-                      // restore stack pointer
-                      list.concat(taicpu.op_reg_reg_const(A_ADDI,NR_STACK_POINTER_REG,NR_STACK_POINTER_REG,localsize));
-                      a_reg_dealloc(list,NR_STACK_POINTER_REG);
-                    end;
-                  end;
+                  // restore stack pointer
+                  list.concat(taicpu.op_reg_reg_const(A_ADDI,NR_STACK_POINTER_REG,NR_STACK_POINTER_REG,localsize+registersize));
+                end;
               list.Concat(taicpu.op_none(A_RET));
-            end
+            end;
           else
             Internalerror(2020031403);
         end;
Index: rtl/xtensa/xtensa.inc
===================================================================
--- rtl/xtensa/xtensa.inc	(revision 46541)
+++ rtl/xtensa/xtensa.inc	(working copy)
@@ -31,11 +31,20 @@
     SysInitFPU;
 end;
 
-{$ifdef fpc_abi_windowed}
 const
-  // Minimum call8 calls to force register spilling to stack for caller of forceSpilledRegs
-  spillcount = 6;
+  {$ifdef fpc_abi_windowed}
+    { Minimum call8 calls to force register spilling to stack for caller of forceSpilledRegs }
+    spillcount = 6;
+    { Offsets relative to SP (a1) }
+    calleraddroffset = -16;
+    callerframeoffset = -12;
+  {$else fpc_abi_windowed}
+    { Offset relative to FP (a15) }
+    calleraddroffset = -8;
+    callerframeoffset = -4;
+  {$endif fpc_abi_windowed}
 
+{$ifdef fpc_abi_windowed}
 procedure forceSpilledRegs(n: uint32); assembler; public name 'forcespilledregs';
   label
     done, fin;
@@ -48,39 +57,36 @@
     movi a15, 0
     fin:
   end;
+{$endif fpc_abi_windowed}
 
 procedure fixCodeAddress(var addr: pointer);
   begin
-    // Check if valid code address
+    { Check if valid code address }
     if ptruint(addr) and $C0000000 >= $40000000 then
       begin
-        // Replace windowed call prefix
-        addr:=codepointer((ptruint(addr)and$00FFFFFF) or $40000000);
-        // Rewind to call instruction address
+        {$ifdef fpc_abi_windowed}
+          { Replace windowed call prefix }
+          addr:=codepointer((ptruint(addr)and$00FFFFFF) or $40000000);
+        {$endif fpc_abi_windowed}
+        { Rewind to call instruction address }
         dec(addr,3);
       end
     else
       addr:=nil;
   end;
-{$endif fpc_abi_windowed}
 
 {$IFNDEF INTERNAL_BACKTRACE}
   {$define FPC_SYSTEM_HAS_GET_FRAME}
   function get_frame:pointer;assembler;
-    label
-      done;
     asm
       {$ifdef fpc_abi_windowed}
-        // Force registers to spill onto stack
+        { Force registers to spill onto stack }
         movi a10, spillcount
         call8 forcespilledregs
-        // now get frame pointer of caller
-        addi a2, a1, -12
-        l32i a2, a2, 0
-        done:
-      {$else}
-        mov a2, a1
       {$endif}
+      { get frame pointer of caller }
+      addi a2, {$ifdef fpc_abi_windowed}a1{$else}a15{$endif}, callerframeoffset
+      l32i a2, a2, 0
     end;
 {$ENDIF not INTERNAL_BACKTRACE}
 
@@ -90,57 +96,52 @@
   begin
     {$ifdef fpc_abi_windowed}
       forceSpilledRegs(spillcount);
-      if (ptruint(framebp)>$3ff00000)and(ptruint(framebp)<$40000000) then
-        begin
-          get_caller_addr:=pointer((framebp-16)^);
-          fixCodeAddress(get_caller_addr);
-        end
-      else
-        get_caller_addr:=nil;
-    {$else}
+    {$endif fpc_abi_windowed}
+    if (ptruint(framebp)>$3ff00000)and(ptruint(framebp)<$40000000) then
+      begin
+        get_caller_addr:=pointer((framebp+calleraddroffset)^);
+        fixCodeAddress(get_caller_addr);
+      end
+    else
       get_caller_addr:=nil;
-    {$endif}
   end;
 
 {$define FPC_SYSTEM_HAS_GET_CALLER_FRAME}
 function get_caller_frame(framebp:pointer;addr:pointer=nil):pointer;
   begin
-    {$ifdef fpc_abi_windowed}
-      if (ptruint(framebp)>$3ff00000)and(ptruint(framebp)<$40000000) then
-        begin
+    if (ptruint(framebp)>$3ff00000)and(ptruint(framebp)<$40000000) then
+      begin
+        {$ifdef fpc_abi_windowed}
           forceSpilledRegs(spillcount);
-          get_caller_frame:=pointer((framebp-12)^);
-        end
-      else
-        get_caller_frame:=nil;
-    {$else}
+        {$endif fpc_abi_windowed}
+        get_caller_frame:=pointer((framebp+callerframeoffset)^);
+      end
+    else
       get_caller_frame:=nil;
-    {$endif}
   end;
 
 
-{$ifdef fpc_abi_windowed}
-  {$define FPC_SYSTEM_HAS_GET_CALLER_STACKINFO}
-  procedure get_caller_stackinfo(var framebp : pointer; var addr : codepointer);
-    begin
-      if (ptruint(framebp)>$3ff00000)and(ptruint(framebp)<$40000000) then
-        begin
+{$define FPC_SYSTEM_HAS_GET_CALLER_STACKINFO}
+procedure get_caller_stackinfo(var framebp : pointer; var addr : codepointer);
+  begin
+    if (ptruint(framebp)>$3ff00000)and(ptruint(framebp)<$40000000) then
+      begin
+        {$ifdef fpc_abi_windowed}
           forceSpilledRegs(spillcount);
-          addr:=codepointer((framebp-16)^);
-          framebp := pointer((framebp-12)^);
-          fixCodeAddress(addr);
-        end
-      else
-        begin
-          addr:=nil;
-          framebp:=nil;
-        end;
-    end;
-{$endif fpc_abi_windowed}
+        {$endif fpc_abi_windowed}
+        addr:=codepointer((framebp+calleraddroffset)^);
+        framebp := pointer((framebp+callerframeoffset)^);
+        fixCodeAddress(addr);
+      end
+    else
+      begin
+        addr:=nil;
+        framebp:=nil;
+      end;
+  end;
 
-
 {$define FPC_SYSTEM_HAS_SPTR}
-Function Sptr : pointer;assembler;
+Function Sptr : pointer; assembler;
   asm
     mov a2,a1
   end;
call0-stackframe.patch (14,998 bytes)   

Florian

2020-08-22 15:02

administrator   ~0125080

I did not check yet, but is this compliant with the official call0 abi?

Christo Crause

2020-08-22 18:05

reporter   ~0125083

The call0 ABI isn't very prescriptive regarding the stack frame layout
and the patch does not (or at least isn't supposed to) violate the requirements.

The ISA document mentions the following regarding call0:
Register a0 holds the return address upon entry to a function, but
unlike the windowed register ABI, it is not reserved for this purpose and may hold other
values after the return address has been saved.

Regarding stack frame:
The stack frame layout for the CALL0 ABI is the same as for the windowed register ABI,
except without the reserved register-spill areas. (Registers will need to be saved to the
stack, but there is no convention for where in the frame to place that storage.) Like the
windowed register ABI, the stack grows down and the stack pointer must be aligned to
16-byte boundaries. The optional stack-frame pointer is also used in the same way, but
it is placed in register a15 with the CALL0 ABI.

Issue History

Date Modified Username Field Change
2020-08-22 14:26 Christo Crause New Issue
2020-08-22 14:26 Christo Crause File Added: call0-stackframe.patch
2020-08-22 15:02 Florian Note Added: 0125080
2020-08-22 18:05 Christo Crause Note Added: 0125083