[PATCH 1/1 v2] ARM: Thumb-2: Symbol manipulation macros for function body copying

[Dave Martin]

Hi again,

There's another problem which I hadn't spotted before:

In the Thumb case, we risk violating the alignment constraints of the
code which gets copied (actually, this is also true of the ARM case,
but less likely to bite).  This matters because the code may contain
literals and other data words -- quite likely given the requirement
for self-containedness.

In the simplest case, we could just require the caller to ensure that
there are is at least size+2 bytes of space at dest_buf: then we can
actually copy the function to dest_buf+2 if this is needed to preserve
word alignment.  But this does make the API harder to use.  For
Thumb-2, at least word alignment is always needed, even if the start
address of the function is only halfword-aligned, because of the way
literals are addressed.

There could also be a need for alignment greater than word alignment
in some cases; for example, where ldrd/strd or some hardware operation
needs an aligned buffer.  This would be rarer though; perhaps we could
get away without it.

We could provide a helper to determine how much space to request from
the allocator:

dest_size = fncpy_size(function, size_of_function, alignment);
dest_buf = allocate(dest_size);
fncpy(dest_buf, &function, size_of_function, alignment);

If we don't know what alignment guarantee is provided by the allocator
itself, we might have to define this conservatively, e.g.:

size_t fncpy_size(function, size_of_function, alignment)
{
    return size_of_function + alignment - 1;
}

...to guarantee the possibility of rounding the copied function
address up far enough to achieve the required alignment without
overrunning the buffer.  Of course, we can probably get simpler that
that...

Any thoughts?

Cheers
---Dave

On Fri, Jan 14, 2011 at 9:42 AM, Dave Martin <dave.martin at linaro.org> wrote:
> On Thu, Jan 13, 2011 at 5:55 PM, Russell King - ARM Linux
> <linux at arm.linux.org.uk> wrote:
>> On Thu, Jan 13, 2011 at 02:51:45PM -0600, Dave Martin wrote:
>>> +/* Cast function pointer to integer: */
>>> +#define __funcp_to_uint(funcp) ({                            \
>>
>> uint is confusing here - it suggests casting a pointer to an unsigned int,
>> rather than a uintptr_t.  Please use uintptr here.
>>
>>> +/*
>>> + * FSYM_REBASE: Determine the correct function pointer for funcp,
>>> + * after the function has been copied to dest_buf:
>>> + */
>>> +#define FSYM_REBASE(funcp, dest_buf)                                 \
>>> +     __uint_to_funcp((uintptr_t)(dest_buf) | FSYM_TYPE(funcp), funcp)
>>> +
>>> +/*
>>> + * FSYM_BASE: Determine the base address in memory of the function funcp
>>> + * FSYM_TYPE: Determine the instruction set type (ARM/Thumb) of funcp
>>> + * (both defined below)
>>> + */
>>> +
>>> +#ifdef CONFIG_THUMB2_KERNEL
>>> +#define FSYM_BASE(funcp) ((void *)(__funcp_to_uint(funcp) & ~(uintptr_t)1))
>>> +#define FSYM_TYPE(funcp) (__funcp_to_uint(funcp) & 1)
>>> +#else /* !CONFIG_THUMB2_KERNEL */
>>> +#define FSYM_BASE(funcp) ((void *)__funcp_to_uint(funcp))
>>> +#define FSYM_TYPE(funcp) 0
>>> +#endif /* !CONFIG_THUMB2_KERNEL */
>>
>> I'd really like to see these gone - otherwise they'll end up being used
>> in code inappropriately.  I like things to be kept as simple as possible
>> with as few opportunities for people to needlessly hook into internal
>> implementation details.
>>
>> If you expose implementation details, people will use them, and then if
>> you need to change the implementation, you've got a lot of code to deal
>> with.
>
> I guess I agree with that now ... with fncpy() implemented, there's
> little legitimate use for the other macros, including the casting
> macros.
>
> I'll fold it all into fncpy() and see how that looks.
>
>>
>> I don't think we need to make this conditional on THUMB2 either - we're
>> probably not wasting much by always clearing and copying the LSB.  And
>> this isn't particularly performance code.
>>
>
> Agreed.  I originally tried to avoid impacting the ARM case, but that
> adds complexity for little benefit.
>
> Cheers
> ---Dave
>