Comment 39 for bug 488354

Revision history for this message
In , Jacob Bramley (jacob-bramley) wrote :

(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.

Thanks,
Jacob