[U-Boot] [PATCH v3] Fix board init code to use a valid C runtime environment

[Simon Glass]

Hi Albert,

On 14 November 2015 at 07:36, Albert ARIBAUD <albert.u.boot at aribaud.net> wrote:
> Hello Simon,
>
> On Fri, 13 Nov 2015 19:29:21 -0700, Simon Glass <sjg at chromium.org>
> wrote:
>> Hi Albert,
>>
>> On 13 November 2015 at 07:43, Albert ARIBAUD <albert.u.boot at aribaud.net> wrote:
>> > board_init_f_mem() alters the C runtime environment's
>> > stack it ls actually already using. This is not a valid
>> > C runtime environment.
>>
>> I think it is doing something like alloca() and reserving space on the
>> stack.
>
> AFAICT, board_init_f_mem does *not* "do something like alloca()",
> it does something that alloca() precisely does *not* do.
>
> What alloca() does is, it creates a local variable on the stack, only
> the size of that local is determined at run time; but that is still a
> *local* variable, that will be freed like all locals when the current
> function returns. The location of the alloca reservation is determined
> by the compiler, which knows how which way the stack works and how much
> of it is being used by the calling function already; and the resulting
> pointer cannot be accessed beyond the size that was allocated.
>
> What board_init_f_mem does is *completely* different: it wild-guesses
> how much of the stack it is using, it assumes which way the stack grows,
> and it purposefully accesses space that it was not given access to, and
> it expects that allocation to survive upon returning to the calling
> context.
>
> Now, if you look at alloca's definition in glibc, you'll see it is
> defined to be a gcc built-in, not a C function. There is a reason for
> this: stack allocation within a C function requires the compiler's help,
> because the compiler must be made aware of the alloca use in order to
> properly free the alloca()ed space on every exit point in the function.
> Only a built-in can get that level of compiler support.
>
> board_init_f_mem's case is worse yet, as, contrary to alloca() which
> only expects /local/ effects, board_init_f_mem expects the allocated
> space to survive the return to the calling context. Doing this would
> require *more* compiler help than alloca does, because it is not more
> a matter of managing the stack pointer; it is a matter of moving
> things around so that the allocated space ends up *above* the call
> frame for board_init_f.

OK. It doesn't matter much - I think we both understand what is going
on in the function.

>
>> It does not actually change the stack pointer in C code.
>
> Non, it does not indeed; it could not, in fact, do this at all without
> resorting to the same technique that is resorted to for implementing
> alloca(), that is, it would have to be a built-in of gcc, as alloca()
> is -- and, as I have just explained, it would require extensive help
> from the compiler, as all compiler-generated epilog code would have to
> account for that potential allocation.
>
>> It changes data that is below the stack pointer, and therefore by
>> definition not part of the stack, I think.
>
> It changes data that it /assumes/ is below the stack pointer and that it
> /assumes/ it can write to; but these two assumptions are unfounded, see
> below.
>
>> What actually goes wrong here? I read your info in the other thread
>> but I'm afraid I still don't understand it.
>
> At least three things are formally wrong here with board_init_f_mem:
>
> - it assumes the stack grows downward.
>
> - it assumes it never uses more than 0x40 bytes of the stack.
>
> - it assumes it can write to the stack beyond what it was allocated for
>   local variables, temporaries and arguments.
>
> Each of these assumptions is unfounded in code which is supposed to be
> architecture-agnostic. Not all stacks grow downward. There is no way to
> ensure that only 0x40 bytes are used by board_init_f_mem. There is no
> guarantee that a C function can write to its own stack beyond what it
> was allocated -- in fact, there is no guarantee that a C function can
> write to its stack anywhere else than its declared or alloca()ed
> locals. Such writes are undefined behavior.
>
> These assumptions are just as bad as assuming any pointer-to-long value
> is valid when we could ensure it is; and we would fix such a situation,
> would we not?
>
> Same here. This patch ensures that the code in board_init_f_mem becomes
> compliant with C runtime environment constraints. This requires
> splitting the C code in two functions so that the first one does not
> access its stack (except the allowed accesses generated by the
> compiler) and the second accesses an area which is in fact not the stack
> any more, thus is safe to access.

Reading between the lines, there isn't actually a problem with this in
practice, but it is doing things that C should not really be doing. Is
that right? To me it seems OK provided we can assume that the stack
needed to call the function is less than 64 bytes. But I'm sure there
is a spec somewhere that the current function violates. Your idea of
what constitutes the stack seems slightly different to mine - I think
of it as the data already written, whereas you think of it as data yet
to write, There is some associated uncertainty since the compiler is
free to fiddle with the stack pointer. In practice in a boot loader we
always make some architectural assumptions based on current operation.
But I agree we should minimise this.

Anyway, I think your solution only adds one more function and still
has most of the code in C rather than assembler, so that it is common
to all archs. That was my main goal with the change.

I prefer to set up gd in the C function if we can, to avoid the
oddness that gd has to be at the bottom and the associated 16-byte
alignment issue. But let's see how that goes. If we need to do it in
assembler, it might be better to require the caller to do it before
calling board_init_f_reserve().

Regards,
Simon