cont_repaint_stack incorrectly calculates sp_safe in cont_util.cpp

[mattbradford83]

Basic Infos

• This issue complies with the issue POLICY doc.
• I have read the documentation at readthedocs and the issue is not addressed there.
• I have tested that the issue is present in current master branch (aka latest git).
• I have searched the issue tracker for a similar issue.
• If there is a stack dump, I have decoded it.
• I have filled out all fields below.

Platform

• Hardware: ESP-12E
• Core Version: 0x951aeffa
• Development Env: Arduino IDE
• Operating System: Windows 10

Settings in IDE

• Module: Nodemcu 1.0
• Flash Mode: qio
• Flash Size: 4MB
• lwip Variant: v2 Lower Memory
• Reset Method: nodemcu
• Flash Frequency: 40Mhz
• CPU Frequency: 80Mhz
• Upload Using: SERIAL
• Upload Speed: 115200

Problem Description

I am very grateful for the work of @igrr and his contribution of the cont_util functions. It appears though that cont_repaint_stack(cont_t *cont) (called by EspClass::resetFreeContStack() in Esp.cpp) does not behave as documented in the comments of cont_util.cpp.

The code documents that cont_repaint_stack should add 64 bytes adjacent to the current stack pointer before painting for safety reasons, however comparing the address pointed to by SP and the resulting address pointed to by sp_safe shows that there is much more than 64 bytes. If I'm correct, this results in not enough of the stack memory being painted, and functions like cont_get_free_stack(cont_t* cont) returning an inaccurate stack free amount.

If I have understood this correctly, this discrepancy is because, in part, the calculation of sp_safe doesn't appear to take into account that the operations are pointer arithmetic. I have provided one possible solution, assigned to variable sp_safe_pos below, which takes these into consideration. By subtracting the pointer of the head to the stack from the stack pointer, we are left with how many 4-byte entries are needing to be painted, which conveniently translates to the number of stack array entries in g_pcont we need to paint. Adding the 64 byte buffer to this is then trivial.

If I have misunderstood this problem, please advise.

MCVE Sketch

#include <cont.h>

void setup() {
  Serial.begin(115200);
  /* Get stack */
  extern cont_t* g_pcont;
  /* use renamed variable to ensure we can copy code from cont_util.h without change */
  cont_t* cont = g_pcont;

  /* Snippet from cont_util.cpp:73-76 */
  register uint32_t *sp asm("a1");
  // Ensure 64 bytes adjacent to the current SP don't get touched to endure
  // we don't accidentally trounce over locals or IRQ temps.
  uint32_t sp_safe = CONT_STACKSIZE/4 - ((sp - &cont->stack[0] - 64)/4);

  /* Compare Addresses */
  Serial.printf("\n\nSP: 0x%08X, sp_safe: %d, &cont->stack[sp_safe]: 0x%08X.\n",sp, sp_safe, &cont->stack[sp_safe]);
  Serial.printf("%d bytes between SP and &cont->stack[sp_safe].\n",(int)sp - (int)&cont->stack[sp_safe]);

  // Solution: 
  // Subtracting the stack pointer address from the stack head pointer address 
  // automatically results in correct stack index. 
  // Because this is pointer arithmetic, we subtract 16 to give us 64 bytes (64/4=16).
  // Add "_pos" so we don't confuse this with an actual memory address. This is an array index.
  uint32_t sp_safe_pos = sp - &cont->stack[0] - 16;

  /* Compare Addresses */
  Serial.printf("\n\nSP: 0x%08X, sp_safe_pos: %d, &cont->stack[sp_safe_pos]: 0x%08X.\n",sp, sp_safe_pos, &cont->stack[sp_safe_pos]);
  Serial.printf("%d bytes between SP and &cont->stack[sp_safe_pos].\n",(int)sp - (int)&cont->stack[sp_safe_pos]);

}

void loop() {
}

Code Output

SP: 0x3FFFFF90, sp_safe: 787, &cont->stack[sp_safe]: 0x3FFFFC0C.
900 bytes between SP and &cont->stack[sp_safe].


SP: 0x3FFFFF90, sp_safe_pos: 996, &cont->stack[sp_safe_pos]: 0x3FFFFF50.
64 bytes between SP and &cont->stack[sp_safe_pos].

[devyte]

cont_repaint_stack was recently implemented by @earlephilhower. Assigning to him for discussion.

[earlephilhower]

Arduino/cores/esp8266/cont_util.cpp

Line 76 in 167f39d

uint32_t sp_safe = CONT_STACKSIZE/4 - ((sp - &cont->stack[0] - 64)/4);

-edit- I re-read it and your suggestion above makes sense. Ignore my prior comment about the constant factor. It's off by 3x the delta...