(From update of attachment 433581)
>+ current = (PRUint32 *)(((PRUint32)current + 7) & 0xfffffff8);
'~7' is clearer to me than '0xfffffff8'. I suppose that's subjective,
though.
>+ *((PRUint64*) current) = *dw;
I don't think that's valid in the non-EABI case where 'current' isn't
8-byte aligned. It will probably work anyway, but I think C assumes that
64-bit values have 64-bit alignment; LDRD and STRD had this restriction
on older architectures. I think the non-EABI and EABI cases should be
entirely contained in the #ifdef to avoid these issues.
>+ /* Wrap when reaching the end of the stack buffer */
>+ if (d == end) d = stk;
It might be wise to add a debug assertion here to check that 'd' is
within 'stk' and '&stk[end]'. That will catch some logic errors that can
be hard to spot, and easy to introduce in future patches.
> * The routine will issue calls to count the number of words
> * required for argument passing and to copy the arguments to
> * the stack.
We don't do this any more. Well, not explicitly anyway. Can you clobber
this bit of the comment please?
----
I like the look of this patch (with the 'Additional patch' on top), and it appears correct so I've marked it 'r+'. However, the N900 crash obviously needs to diagnosed & fixed before this is pushed.
(From update of attachment 433581) current + 7) & 0xfffffff8);
>+ current = (PRUint32 *)(((PRUint32)
'~7' is clearer to me than '0xfffffff8'. I suppose that's subjective,
though.
>+ *((PRUint64*) current) = *dw;
I don't think that's valid in the non-EABI case where 'current' isn't
8-byte aligned. It will probably work anyway, but I think C assumes that
64-bit values have 64-bit alignment; LDRD and STRD had this restriction
on older architectures. I think the non-EABI and EABI cases should be
entirely contained in the #ifdef to avoid these issues.
>+ /* Wrap when reaching the end of the stack buffer */
>+ if (d == end) d = stk;
It might be wise to add a debug assertion here to check that 'd' is
within 'stk' and '&stk[end]'. That will catch some logic errors that can
be hard to spot, and easy to introduce in future patches.
> * The routine will issue calls to count the number of words
> * required for argument passing and to copy the arguments to
> * the stack.
We don't do this any more. Well, not explicitly anyway. Can you clobber
this bit of the comment please?
----
I like the look of this patch (with the 'Additional patch' on top), and it appears correct so I've marked it 'r+'. However, the N900 crash obviously needs to diagnosed & fixed before this is pushed.
Thanks,
Jacob