[PATCH 4/4] score: Ensure stack alignment requirement

[Gedare Bloom]

On Wed, Mar 3, 2021 at 10:56 AM Sebastian Huber
<sebastian.huber at embedded-brains.de> wrote:
>
>
> On 03/03/2021 17:04, Gedare Bloom wrote:
> >> +#if CPU_STACK_ALIGNMENT > CPU_HEAP_ALIGNMENT
> >> +  stack_align = CPU_STACK_ALIGNMENT;
> >> +  stack_begin = (char *) RTEMS_ALIGN_UP( (uintptr_t) stack_begin, stack_align );
> >> +  stack_end = (char *) RTEMS_ALIGN_DOWN( (uintptr_t) stack_end, stack_align );
> > Why align down here? Why align the end at all?
> Most stacks grow down, so the alignment of the end address is important.

OK, I think we had this discussion recently also on related patches.
It might be good to include a comment here that this code aims to
accommodate stacks growing either direction.

> > Is there a requirement
> > for the stack area to be a multiple of the stack alignment? I guess we
> > can lose some bytes and return less than config->stack_size as a
> > result of this?
>
> The CPU_STACK_ALIGNMENT should be set to the stack alignment required by
> the ABI (in SMP configurations, it should be probably at least cache
> aligned, but this is another topic). In this case all stack frames will
> be on such a boundary and you can't use these extra bytes. Independent
> of this, you should probably not calculate your stack size so that you
> actually use the last byte of your stack.
>
OK, but it's not necessary to waste those bytes, when we can align the
stack_begin area of the stack before we calculate the stack_end, if a
stack size is requested that is a multiple of the CPU_STACK_ALIGNMENT
(which is likely).  It's fine either way, pretty minor space
optimization/waste problem.

I could see someone doing something like putting an application CANARY
value at the end (bottom/begin) of their stack.

mystack[end - size] = CANARY;
that will give some unexpected results when the stack end has been
aligned down, and the size is larger than the end-begin due to
alignment.

Just a hunch. Maybe I'm making a big deal out of nothing.

> Also the changed _Stack_Extend_size() includes the alignment overhead.
>
> >
> > Shouldn't we better do...
> > #if CPU_STACK_ALIGNMENT > CPU_HEAP_ALIGNMENT
> >> +  stack_align = CPU_STACK_ALIGNMENT;
> >> +  stack_begin = (char *) RTEMS_ALIGN_UP( (uintptr_t) config->stack_area, stack_align );
> > #else
> > stack_begin = config->stack_area;
> No, I think the code in the patch is fine, however, I will add an assert
> after the block to ensure that begin and end are aligned.
ok

Is there no guarantee that stack_size == config->stack_size?

> > #endif
> > stack_end = stack_begin + config->stack_size;
> >
> > So we always return the requested stack size?
> >
> --
> embedded brains GmbH
> Herr Sebastian HUBER
> Dornierstr. 4
> 82178 Puchheim
> Germany
> email: sebastian.huber at embedded-brains.de
> phone: +49-89-18 94 741 - 16
> fax:   +49-89-18 94 741 - 08
>
> Registergericht: Amtsgericht München
> Registernummer: HRB 157899
> Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
> Unsere Datenschutzerklärung finden Sie hier:
> https://embedded-brains.de/datenschutzerklaerung/
>