Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Jeff Law <law at redhat dot com>]

On 12/14/18 3:05 AM, Uecker, Martin wrote:
> 
> Am Donnerstag, den 13.12.2018, 16:35 -0700 schrieb Jeff Law:
>> On 12/12/18 11:12 AM, Uecker, Martin wrote:
> ...
>>>>> diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
>>>>> index 78e768c2366..ef039560eb9 100644
>>>>> --- a/gcc/c/c-objc-common.h
>>>>> +++ b/gcc/c/c-objc-common.h
>>>>> @@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If
>>>>> not see
>>>>>  
>>>>>  #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
>>>>>  #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
>>>>> +
>>>>> +#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
>>>>> +#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
>>>>>  #endif /* GCC_C_OBJC_COMMON */
>>>> I wonder if we even need the lang hook anymore.  ISTM that a
>>>> front-end
>>>> that wants to use the function descriptors can just set
>>>> FUNC_ADDR_BY_DESCRIPTOR and we'd use the function descriptor,
>>>> else we'll
>>>> use the trampoline.  Thoughts?
>>> The lang hook also affects the minimum alignment for function
>>> pointers via the FUNCTION_ALIGNMENT macro (gcc/default.h). This
>>> does
>>> not appear to change the default alignment on any architecture, but
>>> it causes a failure in i386/gcc.target/i386/attr-aligned.c when
>>> requesting a smaller alignment which is then silently ignored.
>> Ugh.  I didn't see that.
> The test is new (2019-11-29 Martin Sebor), but one could
> argue that we could simply remove this specific test as 'aligned'
> is only required to increase alignment. Martin?
The test is meant to test that we do the right thing consistently.  If
we're failing with your patch, then that needs to be addressed.

I read your note as the test would fail if you dropped the
CUSTOM_FUNCTION_DESCRIPTORS macro, not that it was failing with your
patch as-is.



> 
>>> I am not sure what the best approach is, but my preference
>>> would be to remove the lang hook and the FUNCTION_ALIGNMENT
>>> logic which will also fix the test case (the requested
>>> alignment will be applied).
>>>
>>> I would then instead add a warning (or error?) which triggers
>>> only with -fno-trampolines if the user requests an alignment
>>> which is too small for this mechanism to work.
>>> Does this sound reasonable?
>> So I'm thinking we should wrap the existing patch as-is for the trunk
>> (we're well into stage3 after all).  So leave the hook as-is for gcc-
>> 9.
>>
>> We can then tackle removal of the hook, including twiddling
>> FUNCTION_ALIGNMENT for gcc-10.
>>
>> Does that sound reasonable to you?
> This is fine with me. So just confirm: I should install the 
> patch despite the regression?
We need to address the regression.  Simply removing the test is probably
not the way to go.

jeff