tasks.c is and should be agnostic about underlying ports and their specifics, it only knows about configurations/operations.
adding such code to it to the main branch would over complicate it and make it hard to read, understand and debug.
I think @rtel would agree with that statement ?
I believe a patch would be pretty nice too
@gedeonag: Could you include the patch to tasks.c in the posix port subdirectory so that one could use it if they want and it is part of the official posix port package?
We are working on a document for some pitfalls, I will discuss with the team to include it there, i will post it here as well, so you can give it a shot
I created a sample to modify the stack, port changes and application changes, as we can’t call static functions anymore (it is causing core dumps, with pthread_create, the stack manipulation is expectedly causing some trouble)
If you could give it a try to make sure that is exactly what you are expecting, it would be great
Hi @gedeonag:
I tested your change. There is 1 critical problem I ran into. There were 2 other issues that I was able to work around. But the other issues impose a trade-off on heap size allocation and heap overflow detection.
The critical problem is that by the time I enter the entry point of my task, the stack is already below pxStack. My stack size is 2KByte. When I enter my task function the esp register is 50K below pxStack. So any stack overflow checking that will look to see if pxStack was overwritten won’t trigger because my application code will use stack below pxStack. It appears that the data at pxStack is still 0xa5a5a5a5 so that is why the stack overflow checking code doesn’t trigger with pthread usage of the stack. Don’t know why pthread didn’t overwrite the location with pxStack when it allocated it?
50KByte seemed larger than 16KByte (PTHREAD_STACK_MIN) so I wondered how it could be that far down without a crash? But then I saw your code actually pvPortMalloc PTHREAD_STACK_MIN * sizeof(StackType_t), so it is actually allocating 64KByte for the stack. I’m curious where esp would be if only 16KByte was allocated but I don’t have time to try that. You probably only want to allocate 16KByte (PTHREAD_STACK_MIN) here and not multiply by sizeof(StackType_t)?
The first minor issue I ran into was that my idle thread is using a static stack of size 2KByte. This caused a segmentation fault when running with your patch. I don’t think your code change takes into account statically allocated stacks? When I increased the idle thread’s static stack to PTHREAD_STACK_MIN then no more segmentation fault. This is a compromise I could live with to have a larger idle stack in posix but not on target.
The 2nd minor issue I ran into was that I had to make a very big increase to my heap size. Since you are now doing a pvPortMalloc of PTHREAD_STACK_MIN*4 and I have 22 tasks, it required much more heap than what I had previously allocated. I was able to increase that but now I think that my posix sim’s ability to detect heap overflow may be compromised? I’d have to be very careful about exactly how much extra heap I configure to keep heap overflow detection working. I’d rather detect stack than heap overflow but seems one has to make a tradeoff with this code or allocate just the right amount of extra heap.
In the solution you originally said and I built upon, where the port.c malloc’s a new stack if it is too small then I would not have to increase the heap size and could continue to detect heap overflow. I also proposed that just before the task entry to change the pxStack in the TCB to what esp was (there is a gcc function to get the frame pointer). I think this would have solved my critical issue and the heap overflow issue. If we could add a function into tasks.c that allows one to overwrite the pxStack in the pxCurrentTCB that could be a way to implement this.
Thanks for supplying this patch but I think these issues need to be thought through.
Rob
oh I guess i made a bug here 
NP!, I’ll do some changes, and see whats going on.
1 Like