View Issue Details

IDProjectCategoryView StatusLast Update
0029962FPCCompilerpublic2020-01-28 14:43
ReporterMarcin WiazowskiAssigned To 
PrioritynormalSeverityminorReproducibilityalways
Status newResolutionopen 
PlatformEmbeddedOSOS Version
Product Version3.1.1Product Build33415 
Target VersionFixed in Version 
Summary0029962: Stack checking functionality isn't properly initialized
DescriptionThere are some problems in the way, how the code for embedded targets initializes stack checking functionality. The stack checking feature can be enabled by using -Ct compiler command line parameter. Also fpc.cfg turns stack checking on, when -dDEBUG command line parameter is used.

When stack checking is used, compiler inserts a call to the "fpc_stackcheck" internal function at the beginning of each procedure/function code. "fpc_stackcheck" needs a "StackBottom" global variable (defined in "fpcsrc\rtl\inc\systemh.inc") to be set properly. The problem is that - due to three independent reasons - this variable is not set properly. Below I describe these problems and finally a patch, that works perfectly for me.


PROBLEM 1:
==========

Apart from very few exceptions, twenty seven of "system.pp"/"system.pas" files in Free Pascal (i.e. for all platforms) look similar to the code below; please note, that the only purpose of FPC_HAS_FEATURE_STACKCHECK is to enable "CheckInitialStkLen" forward declaration in "system.inc", if "CheckInitialStkLen" call is needed inside the included "thread.inc":

--------------------------------
unit System;


...
implementation


...
{$I system.inc} // Inside system.inc we have:
    //
    // If FPC_HAS_FEATURE_STACKCHECK is defined, thread.inc -
    // which is included below - uses "CheckInitialStkLen"
    // function, so a forward declaration for "CheckInitialStkLen"
    // must be defined here:
    //
    {$ifdef FPC_HAS_FEATURE_STACKCHECK}
    function CheckInitialStkLen(stklen : SizeUInt) : SizeUInt; forward;
    {$endif FPC_HAS_FEATURE_STACKCHECK}
    ...
    {$i thread.inc} // Inside thread.inc we have:
        //
        // If FPC_HAS_FEATURE_STACKCHECK is defined,
        // "CheckInitialStkLen" function is used:
        //
        {$ifdef FPC_HAS_FEATURE_STACKCHECK}

        StackLength:=CheckInitialStkLen(stkLen);
        StackBottom:=Sptr - StackLength;
        {$endif FPC_HAS_FEATURE_STACKCHECK}
...

//
// Now "CheckInitialStkLen" function is implemented:
//
function CheckInitialStkLen(stklen : SizeUInt) : SizeUInt;
begin
  ...
end;

begin
  ...
  ///
  // System unit initialization - "CheckInitialStkLen" function is
  // used to initialize some variables, that are required for proper
  // stack overflow checking by a "fpc_stackcheck" function
  //
  StackLength := CheckInitialStkLen(initialStkLen);
  StackBottom := initialstkptr - StackLength;
  ...
end.
--------------------------------

Important: "CheckInitialStkLen" function is defined and called unconditionally in the System unit's initialization section - so even if FPC_HAS_FEATURE_STACKCHECK is not defined, and the forward declaration for "CheckInitialStkLen" is not declared, the "CheckInitialStkLen" function is always implemented and called during the System unit's initialization - so "StackLength" and "StackBottom" are initialized.

But "fpcsrc\rtl\embedded\system.pp" unit behaves differently:

--------------------------------
unit System;
...
implementation
...
{$I system.inc}
...

{$ifdef FPC_HAS_FEATURE_STACKCHECK}
function CheckInitialStkLen(stklen : SizeUInt) : SizeUInt;inline;
begin
  result := stklen;
end;

var
  initialstkptr : Pointer; // external name '__stkptr';
{$endif FPC_HAS_FEATURE_STACKCHECK}

begin
  ...
{$ifdef FPC_HAS_FEATURE_STACKCHECK}
  StackLength := CheckInitialStkLen(initialStkLen);
  StackBottom := initialstkptr - StackLength;
{$endif FPC_HAS_FEATURE_STACKCHECK}
  ...
end.
--------------------------------

If FPC_HAS_FEATURE_STACKCHECK is not defined (it's not defined by default), "StackLength" and "StackBottom" variables are not initialized. So "{$ifdef FPC_HAS_FEATURE_STACKCHECK}" and "{$endif FPC_HAS_FEATURE_STACKCHECK}" statements (but not the code between them) should be removed, similarly to System units for other platforms.


PROBLEM 2:
==========

As can be seen above, the "initialstkptr" variable, used for "StackBottom" initialization, is declared as:

var
  initialstkptr : Pointer; // external name '__stkptr';

Reference to external symbol '__stkptr' has been commented out, so "initialstkptr" is not initialized and "StackBottom" gets a wrong value. But simple uncommenting is not a solution, because embedded targets are specific and '__stkptr' symbol is not defined for them. But a '_stack_top' symbol is defined by the linker instead, so "initialstkptr" can be declared as follows:

var
  _stack_top : record end; external name '_stack_top';
  initialstkptr : Pointer = @_stack_top;

Since '_stack_top' symbol is defined by the linker (not by the compiler, as most of symbols), it must be accessed by its address, not by its value; the linker assigns absolute addresses for symbols instead of values for them, so an "@" operator must be used to read the address of the '_stack_top' symbol, not its value.


PROBLEM 3:
==========

Also "initialstklen" variable must be properly initialized for proper "StackLength" and "StackBottom" initialization. It is declared in "fpcsrc\rtl\inc\system.inc" as:

var
  initialstklen : SizeUint;external name '__stklen';

The '__stklen' global symbol is created, when the compiler generates an assembler file; the generated assembler file also assigns a value to '__stklen'.

A value for '__stklen' is generated as follows: the compiler has a built-in set of different parameters for each compilation target. These parameters are defined in compiler sources, in "fpcsrc\compiler\systems\i_*.pas" files, as "tsysteminfo" records. Each "tsysteminfo" record, among others, defines a "stacksize" value; "stacksize" values may differ between compilation targets, but the same target will always get the same "stacksize" value. The compiler ("fpcsrc\compiler\ngenutil.pas") generates a '__stklen' global symbol in the generated assembler file and assigns the predefined "stacksize" value to it. The linker writes this value into the header of the generated executable file (PE, ELF). The "stacksize" value in the header is a request for the operating system when launching the executable, so operating system allocates the requested amount of stack or fails to launch the executable and returns an error. So if the application has been launched, it may blindly assume, that the stack of requested size has been allocated.

But embedded targets are different. There is no operating system to allocate the requested stack amount or to fail and return an error. There are no PE or ELF files to be launched, but binary files stored in FLASH memory instead. So the binary code itself must be responsible for proper stack handling. But the FPC assigns a predefined, constant value to the '__stklen' global symbol when building a binary file and this '__stklen' value is trustingly used by the compiled program code; as mentioned above, "initialstklen" (= '__stklen') variable is defined in "fpcsrc\rtl\inc\system.inc" - and "fpcsrc\rtl\embedded\system.pp" uses this value without any verification. What's more, every programmer may also access the '__stklen' global symbol independently, so proper value of the '__stklen' is vital to the proper application behavior.

Due to FPC internals, the RAM structure for embedded targets is organized as follows (from lower to higher addresses): initialized data area (initialization is performed by the program code, by copying from FLASH), uninitialized data area and unused area. This unused area becomes a stack area, so the stack size must be determined as "address of first byte after RAM end" minus "address of first byte after data end".

The proper stack size for embedded targets is not known at the time when the compiler itself is compiled, so "stacksize" value from the "tsysteminfo" record in "fpcsrc\compiler\systems\i_embed.pas" can't be used. Next, the compiler generates an assembly file and the proper stack size is still not known, because stack location is not determined yet. Next, the linker calculates (by using a script created by FPC) some global symbols like '_stack_top' or '_end', so the stack location and size can be now determined. But the linker, in opposite to the assembler, is not able to assign a value to already defined '__stklen' symbol - it is only able to overwrite an address of that symbol (take note of "@" operator when accessing "_stack_top" in description of PROBLEM 2). So the only chance to assign a proper value to '__stklen' global variable is during the compiled program's runtime - in the System unit's initialization code. Since '__stklen' global variable is represented by "initialstklen" variable in the System unit, this may look like:

var
  _stack_bottom : record end; external name '_end';
  _stack_top : record end; external name '_stack_top';

begin
  ...
  initialstklen := @_stack_top - @_stack_bottom;
  ...
end.


SUMMARY:
========
To solve all the problems described above, the following changes should be made in "fpcsrc\rtl\embedded\system.pp":

--------------------------------
{$ifdef FPC_HAS_FEATURE_STACKCHECK}

function CheckInitialStkLen(stklen : SizeUInt) : SizeUInt;inline;
begin
  result := stklen;
end;

var
  initialstkptr : Pointer; // external name '__stkptr';
{$endif FPC_HAS_FEATURE_STACKCHECK}
--------------------------------
should be changed to:
--------------------------------
function CheckInitialStkLen(stklen : SizeUInt) : SizeUInt;inline;
begin
  result := stklen;
end;

var
  _stack_bottom : record end; external name '_end';
  _stack_top : record end; external name '_stack_top';
  initialstkptr : Pointer = @_stack_top;
--------------------------------

and

--------------------------------
{$ifdef FPC_HAS_FEATURE_STACKCHECK}
  StackLength := CheckInitialStkLen(initialStkLen);
  StackBottom := initialstkptr - StackLength;
{$endif FPC_HAS_FEATURE_STACKCHECK}
--------------------------------
should be changed to:
--------------------------------
  initialstklen := @_stack_top - @_stack_bottom;
  StackLength := CheckInitialStkLen(initialstklen);
  StackBottom := initialstkptr - StackLength;
--------------------------------


Although not necessary, also "stacksize" values in "fpcsrc\compiler\systems\i_embed.pas" can be set to 0 to clearly show, that they are no longer in use:

--------------------------------
    const
       system_arm_embedded_info : tsysteminfo =
          (
            ...
            stacksize : 0;
            ...
          );

       system_avr_embedded_info : tsysteminfo =
          (
            ...
            stacksize : 0;
            ...
          );

       system_mipsel_embedded_info : tsysteminfo =
          (
            ...
            stacksize : 0;
            ...
          );

       system_i386_embedded_info : tsysteminfo =
          (
            ...
            stacksize : 0;
            ...
          );

       system_x86_64_embedded_info : tsysteminfo =
          (
            ...
            stacksize : 0;
            ...
          );
--------------------------------
Steps To ReproduceAll tests have been performed with STM32F107VC microcontroller (ARM Embedded compiler target).


test1.dpr (compilation: fpc -Ct -Tembedded -Parm -XParm-none-eabi- -Cparmv7m -Wpstm32f107xc test1.dpr):
--------------------------------
program Test1;

procedure TestProc;
begin // compiler inserts a call to "fpc_stackcheck" here (if -Ct option has been used)
  if StackBottom = nil then
    Halt;
end;

begin
  TestProc;
end.
--------------------------------

Before patching: "StackBottom" is not set, so "Halt" will be called.

After patching: "StackBottom" is set properly, so "Halt" won't be called.



test2.dpr (compilation: fpc -Ct -Tembedded -Parm -XParm-none-eabi- -Cparmv7m -Wpstm32f107xc test2.dpr):
--------------------------------
program Test2;

{$PUSH}
{$S-}
procedure Hardfault_interrupt; public name 'Hardfault_interrupt';
begin
end;
{$POP}

procedure TestProc;
begin // compiler inserts a call to "fpc_stackcheck" here (if -Ct option has been used)
  TestProc; // exhaust stack recursively
end;

begin
  TestProc;
end.
--------------------------------

Before patching: "fpc_stackcheck" will not detect the stack overflow (because "StackBottom" variable has not been initialized and is set to 0), so stack grows down until stack pointer points below beginning of the RAM area ($20000000 for ARM STM32F1). This causes a hard fault interrupt, but the interrupt handler also must use a bit of stack, which is impossible - this causes a nested hard fault interrupt, and so on, and so on.

After patching: "fpc_stackcheck" will detect the stack overflow in advance and won't allow to overwrite RAM variables below the stack bottom. "fpc_stackcheck" will call "fpc_handleerror" internally, which leads to an internal call to "Halt(202)".
TagsAVR
Fixed in Revision
FPCOldBugId
FPCTarget
Attached Files

Relationships

related to 0035332 closedJeppe Johansen AVR - incorrect stack error checking 

Activities

Marcin Wiazowski

2016-04-04 03:24

reporter   ~0091714

Additional note: linker scripts, generated in "fpcsrc\compiler\systems\t_embed.pas", define a '_stack_top' symbol for all embedded targets, but not a '_stack_bottom' symbol, so I used an '_end' symbol in the patch code above. But the best solution would be to define '_stack_bottom' symbol inside linker scripts and use it in the patch code, instead of a more general '_end' symbol.

This will make the scripts immune to potential memory layout reorganizations in the future - because of obvious '_stack_bottom' meaning, any person modifying the script will give the '_stack_bottom' symbol a proper value. Also for other embedded targets, that will be added in the future, the RAM area may be below the FLASH area, so '_end' symbol will be unusable for stack size calculation. So dedicated '_stack_bottom' symbol would be the best solution.

Issue History

Date Modified Username Field Change
2016-04-04 01:31 Marcin Wiazowski New Issue
2016-04-04 03:24 Marcin Wiazowski Note Added: 0091714
2019-04-26 19:19 Florian Relationship added related to 0035332
2020-01-28 14:43 Dimitrios Chr. Ioannidis Tag Attached: AVR