This is the mail archive of the
libffi-discuss@sourceware.org
mailing list for the libffi project.
Re: [PATCH] Add variadic support
On 2011/2/24 01:20 AM, David Gilbert wrote:
> On 23 February 2011 16:56, Chung-Lin Tang <cltang@linaro.org> wrote:
>> On 2011/2/24 12:20 AM, David Gilbert wrote:
>>> On 23 February 2011 13:25, Chung-Lin Tang <cltang@linaro.org> wrote:
>>>> On 2011/2/23 09:11 PM, David Gilbert wrote:
>>>>> In ARM's ffi_prep_cif_machdep I have:
>>>>>
>>>>> +#ifdef __ARM_PCS_VFP
>>>>> + /* VFP uses the standard ABI for variadic functions */
>>>>> + if ((cif->abi == FFI_VFP) && (cif->nfixedargs != 0))
>>>>> + cif->abi = FFI_SYSV;
>>>>> +#endif
>>>>>
>>>>> and that's the only active change that's needed.
>>>>
>>>> I suggest removing the #ifdef __ARM_PCS_VFP #ifdef. We always have
>>>> support for both FFI_SYSV and FFI_VFP compiled in, so it doesn't make
>>>> sense to leave this bit of logic out when we are under __ARM_PCS.
>>>
>>> I could do, although cif->nfixedargs is also not present except in the
>>> VFP case, so
>>> it would still need an ifdef.
>>>
>>> I did that since I thought it best not to change the layout of the CIF
>>> structure for
>>> existing ARM builds.
>>
>> Sigh...you're right about the layout issue :(
>>
>> Still I think it's best we keep the full FFI_SYSV/VFP capabilities in,
>> or we'll have the weird case of "FFI_VFP with buggy variadics" under softfp.
>
> Wouldn't it make more sense to ban FFI_VFP under softfp?
Well, that would be simple :)
The current code paths already have support for both ABIs; since this
is a "foreign function interface", I thought it made sense to support
all ABIs on that platform, for the most flexibility, so I wrote it that way.
If we were to make softfp/hardfp a libffi build-time configuration,
we're already wasting some runtime cost now. In fact, we would have no
need for 'FFI_VFP' at all; FFI_SYSV would just mean "the ABI".
>> I suggest moving the nfixedargs field into the ARM FFI_EXTRA_CIF_FIELDS,
>> placed after the current VFP fields; this means doing away with the
>> FFI_TARGET_SPECIFIC_VARIADIC symbol, and probably having a
>> machine-specific ffi_prep_cif_var_machdep() function, analogous to the
>> current ffi_prep_cif_machdep().
>
> That would mean changing every back end to add that function or again
Okay, then we should keep the preprocessor symbol then; we can then use
the basic ffi_prep_cif() as the default ffi_prep_cif_var() implementation.
> ifdefing for it, and the belief is that storing the nfixedargs value
> is likely to
> be useful to some backends who have to do different calling conventions for
> variadics anyway.
The keyword is "some" :)
Some means it is not universally needed for supporting variadics, thus
probably should be in FFI_EXTRA_CIF_FIELDS.
Chung-Lin